Re: Add pg_get_publication_ddl function

On Wed, 20 May 2026 at 19:39, Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> Hi.
>
> Some review comments for the v2-0001 patch.
>
> ======
> doc/src/sgml/func/func-info.sgml
>
> (9.28.13. Get Object DDL Functions)
>
> 1.
> + <para role="func_signature">
> + <function>pg_get_publication_ddl</function>
> + ( <parameter>publication</parameter> <type>text</type>
> + <optional>, <literal>VARIADIC</literal> <parameter>options</parameter>
> + <type>text</type> </optional> )
> + <returnvalue>setof text</returnvalue>
> + </para>
>
> I think "pubname" might be a more meaningful name for the first parameter.
>
> ~~~
>
> 2.
> + <para>
> + Reconstructs the <command>CREATE PUBLICATION</command> statement for
> + the specified publication (by OID or name), followed by an
> + <command>ALTER PUBLICATION ... OWNER TO</command> statement (the
> + <command>CREATE PUBLICATION</command> grammar has no
> + <literal>OWNER</literal> clause). Each statement is returned as a
> + separate row. An error is raised if no publication with the supplied
> + OID or name exists. When the publication was created with
> + <literal>FOR ALL TABLES, ALL SEQUENCES</literal>, the emitted
> + statement always lists <literal>ALL TABLES</literal> before
> + <literal>ALL SEQUENCES</literal> regardless of the original order.
> + The following options are supported:
> + <literal>pretty</literal> (boolean) for formatted output and
> + <literal>owner</literal> (boolean) to include
> + <literal>OWNER</literal>.
> + </para></entry>
>
> 2a.
> That "CREATE PUBLICATION" should <link> back to the CREATE PUBLICATION
> docs page.
>
> ~
>
> 2b.
> It is overkill to mention about the potential reordering of ALL TABLES
> and ALL SEQUENCES.
>
> Apart from being unnecessary, there are many other things can also be
> rearranged which are not mentioned:
> - TABLES and ALL TABLES IN SCHEMA clauses might be different order
> than specified
> - The publication parameters might be in a different order than specified
> - The values of 'publish' parameter might be different order than specified
> - etc.
>
> ~~~
>
> GENERAL
>
> 3.
> It would be better if the the rows of "Table 9.96" were in alphabetical order.
>
> ======
> src/backend/utils/adt/ddlutils.c
>
> pg_get_publication_ddl_internal:
>
> 4.
> + if (pub->allsequences)
> + appendStringInfo(buf,
> + "%sALL SEQUENCES",
> + pub->alltables ? ", " : "");
>
> Maybe better to avoid tricky format strings.
>
> SUGGESTION
> if (pub->allsequences)
> {
> if (pub->alltables)
> appendStringInfo(buf, ", ");
>
> appendStringInfo("ALL SEQUENCES");
> }
>
> ~~~
>
> 5.
> + if (pub_incl_relids != NIL)
> + {
> + ListCell *pub_cell;
> + char *schemaname = NULL;
> + char *tablename;
> +
> + append_ddl_option(buf, pretty, 4, "FOR TABLE ");
> +
> + /*
> + * Publication can have table relations
> + */
> + foreach(pub_cell, pub_incl_relids)
>
> Maybe that comment belongs earlier (above the if).
>
> ~~~
>
> 6.
> + appendStringInfo(buf, "%s%s",
> + foreach_current_index(pub_cell) > 0 ? ", " : "",
> + quote_qualified_identifier(schemaname, tablename));
>
> Another place where avoiding a tricky format string may be tidier.
>
> SUGGESTION
> if (foreach_current_index(pub_cell) > 0)
> appendStringInfo(buf, ", ");
>
> appendStringInfo(buf, "%s", quote_qualified_identifier(schemaname, tablename));
>
> ~~~
>
> 7.
> + pubtuple = SearchSysCache2(PUBLICATIONRELMAP, ObjectIdGetDatum(relid),
> + ObjectIdGetDatum(pub->oid));
> +
> + if (!HeapTupleIsValid(pubtuple))
> + elog(ERROR,
> + "cache lookup failed for publication relation %u in publication %u",
> + relid, pub->oid);
>
> 7a.
> Maybe blank line here is not wanted.
>
> ~
>
> 7b.
> Don't need to say "publication" 2x.
>
> /publication relation/relation/
>
> ~~~
>
> 8.
> + /* If non-null, we have a list of columns to publish */
> + if (!cols_nulls)
>
> SUGGESTION FOR THE COMMENT
> Does this table specify a column-list?
>
> ~~~
>
> 9.
> + appendStringInfo(buf, "%s%s",
> + bms_member_index(attmap, attnum) ? ", " : "",
> + quote_identifier(get_attname(relid, attnum, true)));
>
> Another place where avoiding a tricky format string may be tidier.
>
> (change similar to previous review comments)
>
> ~~~
>
> 10.
> + /*
> + * If there is a condition it goes after the columns. We can have
> + * conditions without columns as well.
> + */
> + if (!condition_nulls)
>
> 10a.
> The earlier assignment to 'conditions' should be moved to be directly
> above here.
>
> ~
>
> 10b.
> BTW, it is called a "row filter" so maybe it is better to refer to
> that in the comments/vars instead of the generic sounding "condition".
>
> ~~~
>
> 11.
> + /* If we have schemas, they will go right before the WITH */
>
> The kind of comments that just say "this-goes-after-that" or
> "this-goes-after-that" are not very useful, because it is obvious from
> the code logic that some appendStringInfo comes before or after
> another one.
>
> ~~~
>
> 12.
> + /*
> + * Schemas can be preceded by a list of tables. When they are, the
> + * "TABLES IN SCHEMA" stays inline as a continuation of the existing
> + * FOR clause; otherwise it starts the FOR clause on its own line in
> + * pretty mode.
> + */
>
> IMO it would be better for the FOR TABLE IN SCHEMA to come *before*
> the specific tables in FOR TABLE.
>
> e.g. For the case when there are specified tables "absorbed" into the
> same named schemas I think it is more natural to see the schemas
> first.
> CREATE PUBLICATION mypub FOR TABLES IN SCHEMA s, TABLE s.t1;
>
> ~~~
>
> 13.
> + appendStringInfo(buf, "%s %s",
> + foreach_current_index(schema_cell) > 0 ? "," : "",
> + quote_identifier(nspname));
>
> Another place where avoiding a tricky format string may be tidier.
>
> (change similar to previous review comments)
>
> ~~~
>
> 14.
> + if (pub_excl_relids != NIL)
> + {
> + ListCell *excl_cell;
> + char *schemaname = NULL;
> +
> + appendStringInfoString(buf, " EXCEPT (TABLE ");
>
> The EXCEPT clause is currently permitted only with FOR ALL TABLES, so
> it would be better moving this to earlier in this function where
> pub->alltables was handled.
>
> ~~~
>
> 15.
> + appendStringInfo(buf, "%s%s",
> + foreach_current_index(excl_cell) > 0 ? ", " : "",
> + quote_qualified_identifier(schemaname, NameStr(reltup->relname)));
>
> Another place where avoiding a tricky format string may be tidier.
>
> (change similar to previous review comments)
>
> ~~~
>
> 16.
> + /*
> + * We need to know if we're the second permission added to prefix with a
> + * ", " string
> + */
> + if (pub->pubactions.pubinsert)
> + {
> + /*
> + * By precedence we know that the insert will always be first, no need
> + * to check previous values
> + */
> + appendStringInfoString(buf, "insert");
>
> Both these comments are doing little more than just saying the same as
> the code. IMO they are not needed.
>
> ~~~
>
> 17.
> + if (pub->pubactions.pubinsert)
> + {
> + /*
> + * By precedence we know that the insert will always be first, no need
> + * to check previous values
> + */
> + appendStringInfoString(buf, "insert");
> + first_perm = false;
> + }
> +
> + if (pub->pubactions.pubupdate)
> + {
> + appendStringInfo(buf, "%supdate", first_perm ? "" : ", ");
> + first_perm = false;
> + }
> + if (pub->pubactions.pubdelete)
> + {
> + appendStringInfo(buf, "%sdelete", first_perm ? "" : ", ");
> + first_perm = false;
> + }
> +
> + if (pub->pubactions.pubtruncate)
> + {
> + appendStringInfo(buf, "%struncate", first_perm ? "" : ", ");
> + }
> +
>
> 17a.
> There are some random blank lines that seem unnecessary.
>
> ~
>
> 17b.
> IMO it is tidier to simply append the string you want, instead of
> using a trick format string.
>
> SUGGESTION (compare with patch)
>
> if (pub->pubactions.pubinsert)
> {
> appendStringInfoString(buf, "insert");
> first_perm = false;
> }
> if (pub->pubactions.pubupdate)
> {
> appendStringInfo(buf, first_perm ? "update" : ",update");
> first_perm = false;
> }
> if (pub->pubactions.pubdelete)
> {
> appendStringInfo(buf, first_perm ? "delete" : ",delete");
> first_perm = false;
> }
> if (pub->pubactions.pubtruncate)
> {
> appendStringInfo(buf, first_perm ? "truncate" : ",truncate");
> }
>
> ======
> src/test/regress/expected/publication_ddl.out
>
> 18.
> + CREATE PUBLICATION testpub_ddl_1
> +
> + WITH (publish='insert, update, delete, truncate',
> publish_generated_columns='none', publish_via_partition_root='false');
>
> ~
>
> This "pretty" output appears to have "+" garbage in it. What's that
> about -- it looks like some sort of line continuation character? Can
> it be removed?
>
> ~~~
>
> 19.
> The "pretty" output might be improved if each of the publication
> options was on a new line.
>
> ~~~
>
> 20.
> The generated boolean values (e.g. 'true'/'false') do not need to be quoted.
>
> ======
> src/test/regress/sql/publication_ddl.sql
>
> (Here are lots of test review comments; the first group are are
> general so might apply to multiple test cases).
>
> 21.
> I think you can create all the necessary schema and tables together
> up-front instead of scatering them through the file.
>
> ~~~
>
> 22.
> Make use of the proper publication teminology like "Column Lists" and
> "Row Filters" instead of vague
> "columns" and "conditions".
>
> ~~~
>
> 23.
> Here is an idea:
>
> Instead of having dozens of test publications, just have 1 test
> publication, which you CREATE/DROP for each test case.
>
> Then, since there is a fixed name publication (e.g. "mypub") for
> everything, you can make a subroutine to encapsulate the common code:
>
> +SELECT pg_get_publication_ddl('mypub');
> +SELECT pg_get_publication_ddl((SELECT oid FROM pg_publication WHERE
> pubname='mypub'));
> +SELECT pg_get_publication_ddl('mypub', 'pretty', 'true');
>
> It means your test .sql file can become much shorter/simpler I think.
>
> ~~~
>
> 24.
> There is duplication of some tests:
>
> e.g.
> +-- columns in publication must be quoted
> and
> +-- identifiers that require quoting: publication, schema, table and column
>
> ~~~
>
> 25.
> It is not needed to quote the booleans 'true'/'false' for the options.
>
> //////
>
> 26.
> +-- create base table to test basic table publication
>
> What does "basic table publication" mean? I expect it means different
> things to different people. Better to be explicit about what this is
> really testing.
>
> ~~~
>
> 27.
> +-- create publication for one table with two columns and a condition
> with an expression
>
> What does "with an expression" mean? All Row-Filters are expressions
> aren't they?
>
> ~~~
>
> 28.
> +-- create a publication for a list of tables
>
> Not really describing what this test is doing, which is mixing FOR
> TABLE and FOR TABLES IN SCHEMA.
>
> ~~~
>
> 29.
> +CREATE PUBLICATION testpub_ddl_part3 FOR TABLE testpub_ddl_part WITH
> (publish_via_partition_root='true');
>
> I guess it make no difference since these are only DDL syntax tests,
> but it didn't really make much sense to set
> publish_via_partition_root=true when testpub_ddl_part is the ROOT
> anyway.
>
> ~~~
>
> 30.
> +-- create publication for all tables except two tables
>
> Actually this is also combining with an ALL SEQUENCES test.
>
> ~~~
>
> 31.
> +-- cleanup publications
> +DROP PUBLICATION testpub_ddl_1;
> +DROP PUBLICATION testpub_ddl_2;
> +DROP PUBLICATION testpub_ddl_3;
> +DROP PUBLICATION testpub_ddl_4;
> +DROP PUBLICATION testpub_ddl_5;
> +DROP PUBLICATION testpub_ddl_6;
> +DROP PUBLICATION testpub_ddl_7;
> +DROP PUBLICATION testpub_ddl_8;
> +DROP PUBLICATION testpub_ddl_9;
> +DROP PUBLICATION testpub_ddl_10;
> +DROP PUBLICATION testpub_ddl_schema_1;
> +DROP PUBLICATION testpub_ddl_schema_2;
> +DROP PUBLICATION testpub_ddl_schema_3;
> +DROP PUBLICATION testpub_ddl_schema_4;
> +DROP PUBLICATION testpub_ddl_schema_5;
> +DROP PUBLICATION testpub_ddl_part1;
> +DROP PUBLICATION testpub_ddl_part2;
> +DROP PUBLICATION testpub_ddl_part3;
> +DROP PUBLICATION testpub_ddl_part4;
> +DROP PUBLICATION "Quoted Pub";
> +DROP PUBLICATION testpub_ddl_except1;
> +DROP PUBLICATION testpub_ddl_except2;
>
> As per earlier review comment IMO it would make the test code simpler
> to have just 1 publication that you CREATE/DROP om the fly.
>
> ~~~
>
> 32.
> +-- cleanup tables in schemas
>
> Not sure why this is done separately. Probably easier just to drop the
> schemas with CASCADE so their tables will be auto-deleted.
>

+ buf = makeStringInfo();

I have one more comment: where possible, we should use stack-allocated
StringInfoData objects, as was done in commits a63bbc811d4 and 6d0eba66275.

> ======
> Kind Regards,
> Peter Smith.
> Fujitsu Australia

--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.