Re: pg_get_*_ddl() needs a redesign

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