pg_get_*_ddl() needs a redesign

On Wed, Aug 26, 2026 at 9:52 PM Noah Misch <noah@leadboat.com> wrote:

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: /messages/by-id/4c5f895e-3281-48f8-b943-9228b7da6471@gmail.com
Discussion: /messages/by-id/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

On 2026-09-09 We 11:47 AM, Melanie Plageman wrote:

[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).

Sorry, my bad. Euler and I have been working on fixes, which I hope to
publish today. There was a slight delay due to the issue addressed by my
commit a12600b762c from an hour or so ago.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Wed, Sep 09, 2026 at 12:24:32PM -0400, Andrew Dunstan wrote:

On 2026-09-09 We 11:47 AM, Melanie Plageman wrote:

[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).

Sorry, my bad. Euler and I have been working on fixes, which I hope to
publish today. There was a slight delay due to the issue addressed by my
commit a12600b762c from an hour or so ago.

I want to clarify one thing before you spend much time on fixes. I don't see
"fixes" addressing this thread as a whole, unless you plan to dispute the
premise that the feature needs a redesign. In other words, I think the thread
outcome should be one of these:

- My review erred in determining that the feature needs a redesign. The
design is sound and should proceed, possibly with some fixes.

- Revert the feature.

Noah Misch <noah@leadboat.com> writes:

I want to clarify one thing before you spend much time on fixes. I don't see
"fixes" addressing this thread as a whole, unless you plan to dispute the
premise that the feature needs a redesign. In other words, I think the thread
outcome should be one of these:

- My review erred in determining that the feature needs a redesign. The
design is sound and should proceed, possibly with some fixes.

- Revert the feature.

FWIW, I agree with Noah's point that having two independent
implementations of SQL reverse compilation is not really a place
we want to be: it will be a major PITA for future development.

Unfortunately, I see no practical way to have pg_dump share code with
the backend for most of this stuff; not least because pg_dump has to
support dumping from old backends that won't have any such feature.
So we either swallow that duplication or decide that we're never
going to support this in the backend. Neither choice is palatable,
but I don't think I want to be forced into the duplicative approach
when we've not explored other alternatives (such as refactoring
pg_dump to expose its internal logic in some way).

regards, tom lane

On Wed, Sep 09, 2026 at 01:34:59PM -0400, Tom Lane wrote:

[...] (such as refactoring pg_dump to expose its internal logic in some
way).

Over the years, I've encountered a few cases where a pg_dump library
would be rather handy. That's probably a huge and difficult project,
though.

--
nathan

Hi,

On 2026-09-09 15:12:26 -0400, Andrew Dunstan wrote:

Concrete patches began to appear around October and November 2025. I don't
recall anyone coming along and saying "No, we don't want to do that."

Given how busy the list is at times, I don't think we can really expect
everyone to make all objections known early enough, unfortunately. Otherwise
nobody will ever have time to actually do work on their own.

To me it is beyond stupid that a postgresql server doesn't have enough
introspection to be able to produce the DDL for its own objects. Telling
people that they have to call pg_dump/pg_dumpall to generate the DDL
programmatically is awful. So, do I think this is worth the possible
maintenance burden? Yes. I might take a suggestion of "needs redesign" more
seriously if there had been a hint of what that might look like. But without
that it looks to me like a way of saying "I don't like the feature or think
we need it."

I don't really have a clear opinion on this. I agree that not having a way to
get the DDL in a simple way is bad and that invoking pg_dump for it isn't an
actual answer. But I also agree that having two codepaths for this is pretty
bad too.

I do have some concerns though:

- I don't understand the permission concept:

/* User must have connect privilege for target database. */
aclresult = object_aclcheck(DatabaseRelationId, dbid, GetUserId(), ACL_CONNECT);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, OBJECT_DATABASE,
get_database_name(dbid));

Here the code seems to intend to check the permissions of the to-be-dumped
object.

But then there's stuff like:

/* User must have SELECT privilege on pg_tablespace. */
if (pg_class_aclcheck(TableSpaceRelationId, GetUserId(), ACL_SELECT) != ACLCHECK_OK)
{
ReleaseSysCache(tuple);
aclcheck_error(ACLCHECK_NO_PRIV, OBJECT_TABLESPACE, spcname);
}

Which is a right everyone has, and that is not specific to the to-be-dumped
object. Why are the permissions for pg_tablespace checked, but not e.g. the
permissions for pg_database?

There's also a permission check for pg_authid:
/* User must have SELECT privilege on pg_authid. */
if (pg_class_aclcheck(AuthIdRelationId, GetUserId(), ACL_SELECT) != ACLCHECK_OK)
{
ReleaseSysCache(tuple);
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("permission denied for role %s", rolname)));
}

This afaict makes basically makes pg_get_role_ddl() unusable for
non-superusers?

And the error message indicates that the permission failure is on the user,
but in fact the user's permissions were never checked.

- Has somebody thought about the locking semantics that are needed to make the
ddl functions actually safe under concurrency?

It's not yet too bad with the kinds of objects supported, but it'll be more
important once tables etc are added.

Even now I think these really ought to take appropriate locks?

Once locking is done properly, I think this also ought to make sure to not
lock objects that the user does not have permissions to.

- duplication between *_ddl_* functions:

Why do pg_get_database_ddl(), pg_get_tablespace_ddl_srf(), pg_get_role_ddl()
basically have the same contents? Why populate a list that then is returned
via the SRF_PERCALL mechanism instead of just populating the tuplestore
once?

- The code is quite verbose. With just roles, tablespaces and databases
supported, we're already at ~1k lines. Once more is supported, we're talking
a substantial amount of duplicated code that needs to be maintained
indefinitely.

I think this ought to undergo a fair bit refactoring to reduce the verbosity
/ repetition. It e.g. really can't make sense that we have the ~same ~10
lines for OWNER, CONNECTION LIMIT, IS_TEMPLATE, ALLOW_CONNECTIONS.

There's also stuff like copies of timestamptz_out(), which seems ... not the
right thing to have?

- There are no crosschecks that pg_get_*ddl actually produce the same result
as the SQL generated by pg_dump. Without some automated crosschecking
between them, I think it's pretty much guaranteed that we will have
divergence.

- The tap test is pointlessly expensive, forking 10s of psqls for something
that could - perhaps leaving some locale related filtering aside - as a
single pg_regress style test isn't awesome.

Greetings,

Andres Freund