Re: Add pg_get_publication_ddl function

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