On Wed, Aug 26, 2026 at 9:52 PM Noah Misch <[email protected]> wrote: > > commit 76e514e wrote: > > Author: Andrew Dunstan <> > > AuthorDate: Thu Mar 19 09:52:25 2026 -0400 > > Commit: Andrew Dunstan <[email protected]> > > CommitDate: Sun Apr 5 10:54:54 2026 -0400 > > > > Add pg_get_role_ddl() function > > I reviewed this commit. > > > Author: Mario Gonzalez <[email protected]> > > Author: Bryan Green <[email protected]> > > Co-authored-by: Andrew Dunstan <[email protected]> > > Co-authored-by: Euler Taveira <[email protected]> > > Reviewed-by: Japin Li <[email protected]> > > Reviewed-by: Quan Zongliang <[email protected]> > > Reviewed-by: jian he <[email protected]> > > Discussion: > > https://postgr.es/m/[email protected] > > Discussion: > > https://postgr.es/m/[email protected] > > > + /* > > + * We intentionally omit PASSWORD. There's no way to retrieve the > > + * original password text from the stored hash, and even if we could, > > + * exposing passwords through a SQL function would be a security > > issue. > > + * Users must set passwords separately after recreating roles. > > + */ > > pg_dumpall recreates password hashes without needing the original plaintext. > The first thread message said the use case is "dumping role definitions for > migration or backup purposes without needing pg_dumpall." Users expect their > passwords to be accepted after migration or restore from backup. > > I also don't see a security distinction arising merely because SQL is the > conduit. > > > + * pg_get_role_ddl_internal > > + * Generate DDL statements to recreate a role > > > + /* Build a fresh ALTER ROLE statement for this > > setting */ > > + resetStringInfo(&buf); > > + appendStringInfo(&buf, "ALTER ROLE %s", > > quote_identifier(rolname)); > > + > > + if (datname != NULL) > > + appendStringInfo(&buf, " IN DATABASE %s", > > + > > quote_identifier(datname)); > > This doesn't deal with dependencies. To migrate, you need to dump roles > first, then databases (potentially owned by roles), then IN DATABASE ... SET > statements (which depend on both). By putting IN DATABASE in the same payload > as CREATE ROLE, it's not conducive to restoring from an empty cluster. The > caller would need to break apart the payload and do its own dependency > analysis, which substantially defeats the point of having $SUBJECT. > > The word "depend" appears nowhere on the threads or in this commit. For a > feature aiming for an outcome like pg_dump, I think dependency handling needs > to be foundational in the design. > > > + /* > > + * Scan pg_auth_members for role memberships. We look for rows where > > + * member = roleid, meaning this role has been granted membership in > > other > > + * roles. > > The corresponding step in pg_dumpall is much more complicated; see this > comment in dumpRoleMembership(): > > /* > * We can't dump these GRANT commands in arbitrary order, because a > role > * that is named as a grantor must already have ADMIN OPTION on the > role > * for which it is granting permissions, except for the bootstrap > * superuser, who can always be named as the grantor. > * > * We handle this by considering these grants role by role. For each > role, > * we initially consider the only allowable grantor to be the > bootstrap > * superuser. Every time we grant ADMIN OPTION on the role to some > user, > * that user also becomes an allowable grantor. We make repeated > passes > * over the grants for the role, each time dumping those whose > grantors > * are allowable and which we haven't done yet. Eventually this > should let > * us dump all the grants. > */ > > If the backend version achieves the right outcomes without that complexity, it > should have a comment about how it achieves that.
[RMT hat] We are concerned that there has been no reply addressing these reported issues. We urge the feature authors to reply and determine whether or not this feature needs to be fixed (or reverted). - Melanie
