Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate away from Pg{Everything} everywhere #1454

Open
23 tasks
workingjubilee opened this issue Dec 28, 2023 · 2 comments
Open
23 tasks

Migrate away from Pg{Everything} everywhere #1454

workingjubilee opened this issue Dec 28, 2023 · 2 comments
Labels
deprecation refactor Changes would be considered a refactor of existing functionality.

Comments

@workingjubilee
Copy link
Member

workingjubilee commented Dec 28, 2023

While it is appropriate to disambiguate in some cases, pgrx is effectively always used in the context of Postgres. Everything about it is a Pg-something-something. Postgres adopts the style of using the PG_ prefix because it is written in a language which doesn't believe namespacing is a thing, where all inclusions are global, whereas Rust has... you know. Modules. Import scopes?

As a consequence, we should be... more cautious about bandying about PgWhatever names in our source code, except when the conflict is obvious and inevitable with e.g. the stdlib. The excessive reuse of the Pg prefix is prone to making one go cross-eyed because one's eeys wlil natraully gravtiate toawrds the begnining and enidng of wrods as anchor points when reading them. This is partly because we are used to performing error correction on words, especially long ones, and so we're mostly expecting a word that has about the right number of characters, and mostly the right characters, with a few clear points "standing out". That means making everything have the same prefix can make it notably harder to read, as it forces us to slow down and examine every single character inside, and not in a "reduces the risk of making errors of confusion" way, but in a "raises the risk of confusion and lowers speed, to no benefit" way.

Obviously, the contents of pgrx-pg-sys itself that are emitted by bindgen or interact with FFI directly are effectively exempt from this: they have to be the right names. And that means we shouldn't get too cute about this in general: pgrx::list::List and pgrx::pg_sys::List is a decision of mine that I question somewhat, but I think it is acceptable because the pgrx version is basically "this, but with a generic, because Rust".

The main way to do this for existing types that we want to keep is to rename things and then leave a type alias behind for the previous type with a deprecation notice, so that it doesn't break anything. However, it will sometimes be the case that we want to simply do this as part of a breaking replacement anyways (e.g. the only reason I haven't deprecated PgList is because it is used in places like hooks still). Also, this kinda doesn't work for traits, if memory serves.

Instances of this to formally either deprecate or reject deprecating:

  • PgAtomic
  • PgBox
    • this is a good example of a type which shouldn't drop the prefix
    • not checking it yet because I want to revisit it post-MemCx
  • PgHeapTuple
  • PgHeapTupleError
  • PgList
  • PgLwLock
  • PgLwLockShareGuard
  • PgMemoryContexts
  • PgNode
  • PgOid
  • PgQualifiedNameBuilder
  • PgRelation
  • PGRXSharedMemory
  • PgSharedMemoryInitialization
  • PgSharedMem
  • PgSpinLock
  • PgSpinLockGuard
  • PgSqlErrorCode
  • PgTrigger
  • PgTryBuilder
  • PgVarlena
  • PgVarlenaInOutFuncs
  • PgXactCallbackEvent
@workingjubilee workingjubilee added refactor Changes would be considered a refactor of existing functionality. deprecation labels Dec 28, 2023
@workingjubilee
Copy link
Member Author

workingjubilee commented Dec 28, 2023

Also to expound slightly further: we are redeemed somewhat in the specific case of pgrx-pg-sys name clashes, as the "sin" of things like pg_sys::List coexisting with list::List is helped by people who adopt pgrx tending to copy our "house style", which is to use the pg_sys:: prefix fairly relentlessly, without fully importing things in that case.

This can't be relied on, exactly, but it does help. That is part of why I think path-qualified names should be considered in a fairly separate manner: Programmers are more than just compilers, but they sure do tend to mentally embed what amounts to a lexer, and so a path-qualified name is easily discernable as "this type from this module" in a way that is hard to do for distinguishing the various PgPrefixed names.

@workingjubilee
Copy link
Member Author

workingjubilee commented Jan 7, 2024

Traits need to be imported in order to use them in the syntactically-expected way, so I think they should still be qualified. Otherwise we wind up with the fmt::Write and io::Write problem:

use std::fmt::Write;
use std::io::Write;
//~^ error[E0252]: the name `Write` is defined multiple times

This has an even more unsatisfying resolution, to my mind, than use std::io::Result as IoResult, so I think we should accept constructions like trait PgNode. We can continue that pattern for any trait that doesn't define a Rust-level or pgrx-level concept but rather is a marker for "something from Postgres that behaves in a particular way". This also gets us ahead of any name conflicts that might look like:

trait Node {}
struct Node {}
//~^ error[E0428]: the name `Node` is defined multiple times

This can also help distinguish traits like trait Enlist, for which the bound is more arbitrary rather than hard: we implement it for *mut ffi::c_void, but we could also implement it for *mut pg_sys::Node (for instance). It's a notion defined in Rust, in other words.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation refactor Changes would be considered a refactor of existing functionality.
Projects
None yet
Development

No branches or pull requests

1 participant