pg_get_*_ddl() needs a redesign - Mailing list pgsql-hackers

commit 76e514e wrote:
> Author:     Andrew Dunstan <>
> AuthorDate: Thu Mar 19 09:52:25 2026 -0400
> Commit:     Andrew Dunstan <andrew@dunslane.net>
> CommitDate: Sun Apr 5 10:54:54 2026 -0400
> 
>     Add pg_get_role_ddl() function

I reviewed this commit.

>     Author: Mario Gonzalez <gonzalemario@gmail.com>
>     Author: Bryan Green <dbryan.green@gmail.com>
>     Co-authored-by: Andrew Dunstan <andrew@dunslane.net>
>     Co-authored-by: Euler Taveira <euler@eulerto.com>
>     Reviewed-by: Japin Li <japinli@hotmail.com>
>     Reviewed-by: Quan Zongliang <quanzongliang@yeah.net>
>     Reviewed-by: jian he <jian.universality@gmail.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.


>  src/backend/utils/adt/ddlutils.c       | 361 +++++++++++++++++++++++++++++++++

Separate from the above correctness problems, I object to having two
independent implementations in the tree for translating catalog state into SQL
that recreates that state.  pg_dump support is already a key friction source
for implementing most new catalog-stored features.  I don't want such features
to face updating both a src/bin/pg_dump implementation and an independent
backend implementation, each with its own bugs.

Wrapping pg_dump and pg_dumpall is already a reasonable implementation of
these use cases, so I think the bar for introducing another implementation is
high.  A shared implementation used by both could potentially clear that bar;
this commit doesn't start in that direction.  PostgreSQL should not carry two
independent implementations of this logic.


I also ran an Opus 4.8 review.  It found some minor issues that aren't worth
addressing before the above.  I'm attaching it for completeness.