Hi Jonathan, I tested v3 fairly thoroughly and it holds up well. I checked that the generated DDL round-trips (get the DDL, drop the publication, replay it, and confirm the output is identical) across: empty publications, publish='', column lists (including reordered and after a dropped column), row filters, column+filter combinations, quoted schema/table/column names, FOR TABLE + TABLES IN SCHEMA, FOR ALL TABLES EXCEPT, ALL SEQUENCES + ALL TABLES EXCEPT, generated columns, publish_via_partition_root, and partitioned tables. The variadic option parsing (unknown/duplicate/odd/NULL options, owner=false) behaves correctly and consistently with pg_get_database_ddl(). No functional issues found.
One thing from your original post I wanted to close out -- the worry about this leaking table/column/row-filter information to users without rights on those tables. I don't think there's a new exposure here: pg_publication and pg_publication_rel have no REVOKE and are world-readable like the other system catalogs (unlike pg_subscription, which REVOKEs because of subconninfo). So the membership, column lists (prattrs) and row filters (prqual) are already visible to any user via the catalogs; pg_get_publication_ddl() just formats what is already readable. This also matches the existing pg_get_*def family (pg_get_viewdef, pg_get_indexdef, pg_get_functiondef, ...): none of them perform a privilege check. I confirmed this -- given the object's OID, those functions return the full definition even to a role that has no access to the object at all (the only gate is name resolution, i.e. schema USAGE, which an OID argument bypasses). So I don't think a separate privilege check is needed here either, and adding one would be inconsistent both with those functions and with the catalogs being readable. A couple of smaller comments: 1. src/test/regress/sql/publication_ddl.sql: the 'owner' option isn't exercised anywhere -- there's no pg_get_publication_ddl(..., 'owner', 'false') case verifying the OWNER statement is omitted. Given the polarity is easy to get wrong, a test for it would help. 2. src/backend/utils/adt/ddlutils.c: minor, there's a double blank line after appendStringInfoString(&buf, "publish='") before the pubinsert block. Thanks, Rui
