Re: pg_get_*_ddl() needs a redesign

On Wed, Aug 26, 2026 at 9:52 PM Noah Misch <noah(at)leadboat(dot)com> wrote:
>
> commit 76e514e wrote:
> > Author: Andrew Dunstan <>
> > AuthorDate: Thu Mar 19 09:52:25 2026 -0400
> > Commit: Andrew Dunstan <andrew(at)dunslane(dot)net>
> > CommitDate: Sun Apr 5 10:54:54 2026 -0400
> >
> > Add pg_get_role_ddl() function
>
> I reviewed this commit.
>
> > Author: Mario Gonzalez <gonzalemario(at)gmail(dot)com>
> > Author: Bryan Green <dbryan(dot)green(at)gmail(dot)com>
> > Co-authored-by: Andrew Dunstan <andrew(at)dunslane(dot)net>
> > Co-authored-by: Euler Taveira <euler(at)eulerto(dot)com>
> > Reviewed-by: Japin Li <japinli(at)hotmail(dot)com>
> > Reviewed-by: Quan Zongliang <quanzongliang(at)yeah(dot)net>
> > Reviewed-by: jian he <jian(dot)universality(at)gmail(dot)com>
> > Discussion: https://postgr.es/m/4c5f895e-3281-48f8-b943-9228b7da6471@gmail.com
> > Discussion: https://postgr.es/m/e247c261-e3fb-4810-81e0-a65893170e94@dunslane.net
>
> > + /*
> > + * 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