pg_get_*_ddl() needs a redesign
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253564psql -h localhost -U postgresBuilt from patchset v25 (message #26), September 13, 2026 at 06:40 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253564_25 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253564_25 && git checkout t253564_25Patchset v25 (message #26) is on t253564_25
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 -0400Add 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.
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.
Attachments:
ddlutils-defect-tests-v0.patchtext/plain; charset=utf-8Download+358-0
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 -0400Add 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
On 2026-09-09 We 1:34 PM, Tom Lane wrote:
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).
I first raised the idea of these functions on the mailing list in July
2025, although the idea was much older. [1]/messages/by-id/945db7c5-be75-45bf-b55b-cb1e56f2e3e9@dunslane.net Among other things, you
(Tom) said [2]/messages/by-id/758258.1755354170@sss.pgh.pa.us:
Insisting that this feature support pg_dump is a good way
of making sure that nothing useful will emerge at all.
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." After they were committed there were various comments on the way they were done, and I addressed them. Now, months later, we are more or less told that this whole enterprise has been misguided from the start. Is it any wonder that people often find the process of contributing to Postgres discouraging? And that would include people who have been working on more important members of this set for release 20, particularly the one that most people want, namely pg_get_table_ddl() that Akshay has been working on. I don't feel badly for myself, but I do feel badly for the contributors.
I believe reverting now almost certainly means this proposal or anything like it is probably dead for the foreseeable future.
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."
cheers
andrew
[1]: /messages/by-id/945db7c5-be75-45bf-b55b-cb1e56f2e3e9@dunslane.net
[2]: /messages/by-id/758258.1755354170@sss.pgh.pa.us
--
Andrew Dunstan
EDB:https://www.enterprisedb.com
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
Import Notes
Reply to msg id not found: ccd76a86-51c0-4395-b6b2-2e7214c2fbf2@dunslane.net
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
On Wed, Sep 09, 2026 at 03:12:26PM -0400, Andrew Dunstan wrote:
On 2026-09-09 We 1:34 PM, Tom Lane wrote:
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).I first raised the idea of these functions on the mailing list in July 2025,
although the idea was much older. [1] Among other things, you (Tom) said
[2]:Insisting that this feature support pg_dump is a good way
of making sure that nothing useful will emerge at all.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." After they were committed there were various comments on the way they were done, and I addressed them. Now, months later, we are more or less told that this whole enterprise has been misguided from the start. Is it any wonder that people often find the process of contributing to Postgres discouraging? And that would include people who have been working on more important members of this set for release 20, particularly the one that most people want, namely pg_get_table_ddl() that Akshay has been working on. I don't feel badly for myself, but I do feel badly for the contributors.
Going forward, feel free to add me to the To: line of a mailing list post and
explicitly ask me for my guess about the project's overall prognosis, likely
ways of failing, etc. My guess may be brief and/or wrong, but I'll provide
one.
When it's four months after feature freeze and I find non-subtle, unreported
defects in the feature, the development process wasn't adequate for the
subject matter. To be transparent, observing this outcome changed my view of
code duplication. Duplication was always a smell, but it seemed like a
plausible compromise tactic in 2025. Now I find the duplication gave wrong
code too much place to hide.
I believe reverting now almost certainly means this proposal or anything like it is probably dead for the foreseeable future.
I'm prepared to accept that outcome.
Import Notes
Reply to msg id not found: ccd76a86-51c0-4395-b6b2-2e7214c2fbf2@dunslane.net
On 2026-09-09 We 6:25 PM, Andrew Dunstan wrote:
Thanks for reviewing. I hope to have some proposals tomorrow for
addressing these issues as well as those raised by Noah.
I have run out of time, energy and brain cells. I have some working
patches, but a) they need a little polish and b) I think they are too
invasive for consideration now. The major difference in actual code (as
opposed to testing) is in how we deal with permissions. I spent quite a
lot of time yesterday and today on that, and came to the conclusion that
it needs to be reworked along these lines: we should allow the user to
see the information the functions provide if and only if they can see it
by directly selecting from the catalog. Code changes to implement this
by directly running ACL checks on the relevant catalogs are
straightforward but also substantial. My proposed changes to testing are
also substantial, being both more efficient and providing better
coverage, especially w.r.t pg_dump/all compatibility.
I therefore suggest that we revert for release 19, but not in master,
and I will continue to polish the patches I mentioned and publish them
in a few days.
I can make the revert happen in the next day or so.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On Thu, Sep 10, 2026 at 12:00 AM Noah Misch <noah@leadboat.com> wrote:
Going forward, feel free to add me to the To: line of a mailing list post and
explicitly ask me for my guess about the project's overall prognosis, likely
ways of failing, etc. My guess may be brief and/or wrong, but I'll provide
one.
I don't think this is how this mailing list works. The volume is
overwhelming for me, too, but patch authors aren't obliged to cc
anyone specifially.
When it's four months after feature freeze and I find non-subtle, unreported
defects in the feature, the development process wasn't adequate for the
subject matter. To be transparent, observing this outcome changed my view of
code duplication. Duplication was always a smell, but it seemed like a
plausible compromise tactic in 2025. Now I find the duplication gave wrong
code too much place to hide.
I think we would do well to confine all of our present conversations,
insofar as it is possible to do so, to whether any given feature
contains defects incompatible with shipping it. The desirability of
this particular feature is, as Andrew says, something that would have
been better litigated six or more months ago. Granted, we're all
overwhelmed, and that makes it more understandable that people who had
concerns about desirability did not raise them at the time, but we
will not succeed in producing awesome software by forcing last-minute
reverts because we've changed our mind about whether the feature is a
good idea in principle. There's more than enough to revert on the
basis of whatever is broken in practice.
That said, if this is coming out of v19 anyway, then IMHO it is fair
game to reopen the question of whether we want it in v20. But if we're
going to do that, we should hash that out now rather than in six
months from now or twelve months from now. Personally, I have mixed
feelings about this feature. On the one hand, I am sure it will get
some use, and I have wanted it myself. On the other hand, it will get
far less use, and far less testing, than the equivalent code in
pg_dump, so it's more likely to have bugs now and in the future, and
the fact that there's no reasonable way (at least, not that I can see)
to handle version skew within a facility like this is a major
disadvantage vs. pg_dump. Nevertheless, on balance, I'm unconvinced
that there's sufficient reason to categorically block a
sufficiently-well-implemented feature of this type. But not even the
most desirable feature should remain in tree if the implementation
isn't of sufficient quality.
--
Robert Haas
Robert Haas <robertmhaas@gmail.com> writes:
That said, if this is coming out of v19 anyway, then IMHO it is fair
game to reopen the question of whether we want it in v20. But if we're
going to do that, we should hash that out now rather than in six
months from now or twelve months from now. Personally, I have mixed
feelings about this feature. On the one hand, I am sure it will get
some use, and I have wanted it myself. On the other hand, it will get
far less use, and far less testing, than the equivalent code in
pg_dump, so it's more likely to have bugs now and in the future, and
the fact that there's no reasonable way (at least, not that I can see)
to handle version skew within a facility like this is a major
disadvantage vs. pg_dump.
I'm a bit confused why you say that. To my mind, if we were going
to proceed in this direction, version skew would ideally be solved
like this:
1. Server-side DDL-reconstruction functions only need to work with
their particular server version; necessarily so, since that's the
only catalog data they'll ever see.
2. pg_dump just calls the source server's DDL-reconstruction functions
and doesn't have to worry about version skew.
The difficulty is how do we get to #2 from where we are. Part of that
could be handled by saying "well, pg_dump calls the server functions
when source server version >= N, and otherwise does it locally",
and expecting that we could eventually retire the "locally" code.
Since the "locally" code would only need to work with frozen server
versions, that doesn't seem too unmaintainable. The hard parts as
I see it are
(A) pg_dump is optimized to gather data across the entire database
at once. Converting it to call functions that produce info about
a single object at a time would almost certainly be catastrophic
for performance on large databases.
(B) pg_dump slices and dices the data in ways that don't necessarily
make sense for standalone usage. For example it doesn't want to
mention a primary key when creating a table, rather it wants to add
that constraint (and build the index) after loading data.
I do not feel that these problems are necessarily insoluble, but
they do require some hard work to define the server-side functionality
properly.
regards, tom lane
On Fri, Sep 11, 2026 at 12:19:15AM -0400, Robert Haas wrote:
On Thu, Sep 10, 2026 at 12:00 AM Noah Misch <noah@leadboat.com> wrote:
Going forward, feel free to add me to the To: line of a mailing list post and
explicitly ask me for my guess about the project's overall prognosis, likely
ways of failing, etc. My guess may be brief and/or wrong, but I'll provide
one.I don't think this is how this mailing list works. The volume is
overwhelming for me, too, but patch authors aren't obliged to cc
anyone specifially.
To clarify, that text was me offering Andrew a new option, not suggesting that
he has any new duty.
When it's four months after feature freeze and I find non-subtle, unreported
defects in the feature, the development process wasn't adequate for the
subject matter. To be transparent, observing this outcome changed my view of
code duplication. Duplication was always a smell, but it seemed like a
plausible compromise tactic in 2025. Now I find the duplication gave wrong
code too much place to hide.I think we would do well to confine all of our present conversations,
insofar as it is possible to do so, to whether any given feature
contains defects incompatible with shipping it.
The defects are the reason not to ship this implementation. One of those
defects, the inability to restore in dependency order, cannot be fixed without
redesigning the current SQL-facing API. That is the reason not to ship this
design. See my original post of this thread for details.
On Fri, Sep 11, 2026 at 1:27 AM Noah Misch <noah@leadboat.com> wrote:
I think we would do well to confine all of our present conversations,
insofar as it is possible to do so, to whether any given feature
contains defects incompatible with shipping it.The defects are the reason not to ship this implementation. One of those
defects, the inability to restore in dependency order, cannot be fixed without
redesigning the current SQL-facing API. That is the reason not to ship this
design. See my original post of this thread for details.
I saw that, but that seems like you're questioning the feature's
choice of scope as much as its actual merits. I suppose the idea was
that this feature is for getting the definition of an individual
object for possible cloning into this database or some other where the
prerequisites are already present. If the feature had the intention of
allowing for full-database restoration and failed to do so, that's a
code defect. Otherwise, it isn't. If the feature could reasonably have
been designed in such a way that it would allow for full-database
restoration but was not actually designed in such a way, that's a
design defect. It's not obvious to me that this is the case, but it
might be. If there's no reasonable way to make a feature of this type
suitable for that use case, this isn't a defect at all, just code that
does something that you may not find entirely worthwhile. Regardless,
I'm not saying that you have to like this feature -- I don't think
it's great either, although it seems I dislike it less than you -- but
I think fairness demands that we not label as code defects issues that
are properly categorized otherwise.
--
Robert Haas
On Fri, Sep 11, 2026 at 08:24:50AM -0400, Robert Haas wrote:
On Fri, Sep 11, 2026 at 1:27 AM Noah Misch <noah@leadboat.com> wrote:
I think we would do well to confine all of our present conversations,
insofar as it is possible to do so, to whether any given feature
contains defects incompatible with shipping it.The defects are the reason not to ship this implementation. One of those
defects, the inability to restore in dependency order, cannot be fixed without
redesigning the current SQL-facing API. That is the reason not to ship this
design. See my original post of this thread for details.I saw that, but that seems like you're questioning the feature's
choice of scope as much as its actual merits.
That is not the case.
I suppose the idea was
that this feature is for getting the definition of an individual
object for possible cloning into this database or some other where the
prerequisites are already present. If the feature had the intention of
allowing for full-database restoration and failed to do so, that's a
code defect. Otherwise, it isn't.
The first discussion linked from the 76e514e commit message, [1]/messages/by-id/4c5f895e-3281-48f8-b943-9228b7da6471@gmail.com, described
the "main use case" as "dumping role definitions for migration or backup
purposes without needing pg_dumpall". My review findings are about failures
that arise when using the feature as part of a migration.
[1]: /messages/by-id/4c5f895e-3281-48f8-b943-9228b7da6471@gmail.com
On Fri, Sep 11, 2026 at 9:35 AM Noah Misch <noah@leadboat.com> wrote:
The first discussion linked from the 76e514e commit message, [1], described
the "main use case" as "dumping role definitions for migration or backup
purposes without needing pg_dumpall". My review findings are about failures
that arise when using the feature as part of a migration.[1] /messages/by-id/4c5f895e-3281-48f8-b943-9228b7da6471@gmail.com
I think you're absolutizing this to a greater degree than seems fair
to me. I agree with you that the justification for not including
passwords in the output doesn't make sense; IMHO, not including them
is just incorrect behavior. The dependency handling thing I do not
find so black-and-white. I think that point merits discussion, rather
than conclusory statements.
--
Robert Haas
Hi,
On 2026-09-11 00:55:54 -0400, Tom Lane wrote:
I'm a bit confused why you say that. To my mind, if we were going
to proceed in this direction, version skew would ideally be solved
like this:1. Server-side DDL-reconstruction functions only need to work with
their particular server version; necessarily so, since that's the
only catalog data they'll ever see.2. pg_dump just calls the source server's DDL-reconstruction functions
and doesn't have to worry about version skew.
Isn't the problem with that that we occasionally need to adapt the syntax
between versions to some degree? I think it has become less common, but I'm a
bit sceptical that it's wise to decide we won't ever again need that...
Binary upgrade mode is probably where we hit this most often these days.
I wonder if the right thing here would be to separate the "metadata
collection" aspect from the "SQL generation" aspect. I.e. have a small library
that can generate DDL statements from its inputs, without any catalog
accesses. We'd still end up - annoyingly - with two versions of the metadata
collection (one from syscaches for the get_ddl* SQL functions, one via queries
for pg_dump), but at least the SQL generation aspect would be deduplicated.
I think such a separation could be useful for other things as well,
e.g. logical replication of DDLs.
(A) pg_dump is optimized to gather data across the entire database
at once. Converting it to call functions that produce info about
a single object at a time would almost certainly be catastrophic
for performance on large databases.
Hm. I don't think I really understand that:
From what I can see the main benefits of entire-database-at-once isn't catalog
lookup efficiency, it's the avoidance of per-object roundtrips and avoiding
osme serialization cost.
Sure, there's also some benefits of turning more things into sequential scans,
rather than index lookups (via sycaches), but there's also a lot of overhead
in using full blown joins for everything, and in doing redundant lookups for
data that's repeated in almost every query (e.g. pg_namespace, name of owner,
etc).
For comparable performance I think you'd obviously have to call the DDL
generating functions for all objects of a class in one go, but I don't think
that'd be hard.
(B) pg_dump slices and dices the data in ways that don't necessarily
make sense for standalone usage. For example it doesn't want to
mention a primary key when creating a table, rather it wants to add
that constraint (and build the index) after loading data.
Yep, that seems nontrivial.
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
On 2026-09-11 00:55:54 -0400, Tom Lane wrote:
I'm a bit confused why you say that. To my mind, if we were going
to proceed in this direction, version skew would ideally be solved
like this:1. Server-side DDL-reconstruction functions only need to work with
their particular server version; necessarily so, since that's the
only catalog data they'll ever see.2. pg_dump just calls the source server's DDL-reconstruction functions
and doesn't have to worry about version skew.
Isn't the problem with that that we occasionally need to adapt the syntax
between versions to some degree? I think it has become less common, but I'm a
bit sceptical that it's wise to decide we won't ever again need that...
I think we try very very hard to avoid making changes that would break
existing dump files. There are certainly times where we change what
pg_dump prefers to output, but the server should continue to accept
what pg_dump used to output (at least for quite a few years). Users
don't always have the luxury of being able to follow the advice to
"use the newest pg_dump to make your dump".
I wonder if the right thing here would be to separate the "metadata
collection" aspect from the "SQL generation" aspect. I.e. have a small library
that can generate DDL statements from its inputs, without any catalog
accesses. We'd still end up - annoyingly - with two versions of the metadata
collection (one from syscaches for the get_ddl* SQL functions, one via queries
for pg_dump), but at least the SQL generation aspect would be deduplicated.
Hmm...
regards, tom lane
On Fri, Sep 11, 2026 at 10:23 AM Andres Freund <andres@anarazel.de> wrote:
2. pg_dump just calls the source server's DDL-reconstruction functions
and doesn't have to worry about version skew.Isn't the problem with that that we occasionally need to adapt the syntax
between versions to some degree? I think it has become less common, but I'm a
bit sceptical that it's wise to decide we won't ever again need that...
+1.
(B) pg_dump slices and dices the data in ways that don't necessarily
make sense for standalone usage. For example it doesn't want to
mention a primary key when creating a table, rather it wants to add
that constraint (and build the index) after loading data.Yep, that seems nontrivial.
The kind of scenario Tom mentions here is exactly why I'm unwilling to
take Noah's criticisms purely at face value. Dealing with dependencies
means splitting up the creation of single objects into multiple DDL
commands in weird ways. But if you just want the definition of one
single object expressed in the most straightforward way, you are going
to want the primary key as part of the object definition, not a
separate, broken-out statement. There are use cases for both things.
If we judge the facility that was built here as a substitute for
pg_dump, it's fatally flawed and should come straight out of the tree
and be deposited directly and permanently into the circular file,
which seems to be what Noah wants. But if we judge it as having a
separate purpose, then the question is how well it serves that
purpose. And the phrase Tom uses, "standalone usage," is at least
sketch of a purpose where dependency handling is irrelevant and
perhaps even undesirable.
--
Robert Haas
