Re: pg_get_*_ddl() needs a redesign

On 2026-09-09 We 4:12 PM, Andres Freund wrote:
> 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.

Quite so, but the time frame in this case was pretty large.

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

Well, I have a proposal at least for that, namely that we test the
equivalence with a TAP test. I don't know how comprehensive we can make
it, but it seems like s atart at least.

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

Thanks for reviewing. I hope to have some proposals tomorrow for
addressing these issues as well as those raised by Noah.

cheers

andrew

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