Add pg_get_publication_ddl function

Hi,

I haven't tested your patch yet, but I noticed a few points that may need adjustment:

Based on your code logic, the pub parameter passed here can never be NULL — if the corresponding PUBLICATION does not exist, an error should have already been thrown earlier in the code flow.
Therefore, the following code block can be removed, and the usage of return (Datum) NULL; in this block is also incorrect:
```
if (pub == NULL)
return (Datum) NULL;
```
pubtuple is not being freed — please use heap_freetuple to release it.

The format string "%sALL SEQUENCE" should be corrected to "%sALL SEQUENCES".

--
Regards,
Man Zeng
www.openhalo.org

Hi Jonathan,

Here are some initial review comments for patch v1-0001.

(I haven't applied or run anything -- this is all just by inspection).

======
1. GENERAL - typos

There are many typos and upper/lower capitalisation mistakes in the
comments / commit message/docs.

I won't list them all, but below is a sample. Consider using a
spelling/grammar checker to discover all of them.

typo /for a giving PUBLICATION./
typo /thuse/
typo /Comprhensive/
typo /CREATE statement for a giving PUBLICATION/
typo /DDL fro a PUBLICATION/
typo /The sequence are/
typo /publication an have relations/
typo /If no null/
typo /at the end make sense to/
typo /create publation/

======

doc/src/sgml/func/func-info.sgml

2.
+     <tbody>
+      <row>
+       <entry role="func_table_entry"><para role="func_signature">
+        <indexterm>
+         <primary>pg_get_publication_ddl</primary>
+        </indexterm>
+        <function>pg_get_publication_ddl</function> (
<parameter>publication</parameter> <type>text</type> or
<type>oid</type> )
+        <returnvalue>text</returnvalue>
+       </para>

Maybe say "name" instead of "text" for the parameter; otherwise, there
is no indication of what the text should represent.

~~~

3.
+       <para>
+        Recreate the CREATE statement for a giving PUBLICATION.
+        The result is a complete <command>CREATE
PUBLICATION</command> statement.
+       </para></entry>
+      </row>
+     </tbody>

SUGGESTION
Returns the <command>CREATE PUBLICATION</command> command that would
create this publication.

======
src/backend/utils/adt/ruleutils.c

pg_do_publication_ddl:

4.
+ if (pub == NULL)
+ return (Datum) NULL;

Was a cast needed?

~~~

5.
+ publications = GetPublicationRelations(pub->oid, PUBLICATION_PART_ALL);

AFAIK, this returns relids of the published tables. So this variable
name 'publications' is very misleading.

~~~

6.
+ /*
+ * we add the publication name, this isn't covered by the schema, yet?
+ */

What does that "covered by the schema" part mean? Pubnames are not
schema-qualified (??)

~~~

7.
+ /*
+ * having all tables or all sequence means there's not publications per
+ * table
+ */

This might be true for now, but it might be wise to avoid making this
assumption if it is possible to do so, because one day, "FOR ALL
SEQUENCES, FOR TABLE t1" syntax may be added, and then your assumption
will be broken.

IOW, would it be better not to say 'else'?

~~~

8.
+ if (pub->allsequences)
+ appendStringInfo(buf,
+ "%sALL SEQUENCE",
+ pub->alltables ? ", " : " ");

This should say "ALL SEQUENCES", not "ALL SEQUENCE"

~~~

9.
+ /*
+ * publication an have relations of tables, in schema or current
+ * schema
+ *
+ */

9a.
Unnecessary newline.

~

9b.
Why are you mentioning FOR TABLES IN SCHEMA here? That seems
unnecessary. That clause gets handled separately later.

~~~~

10.
+ /* If no null, means we have a list of columns to publish */
+ if (!cols_nulls)
+ {
+ ArrayType  *arr;
+ int ncolumns;
+ int16    *cols;
+ bool fcol = true;
+
+ arr = DatumGetArrayTypeP(columns);
+ ncolumns = ARR_DIMS(arr)[0];
+ cols = (int16 *) ARR_DATA_PTR(arr);
+
+ appendStringInfoChar(buf, '(');
+ for (int i = 0; i < ncolumns; i++)
+ {
+ appendStringInfo(buf, "%s%s",
+ fcol ? "" : ", ",
+ get_attname(rel->rd_id, cols[i], true));
+ fcol = false;
+ }
+ appendStringInfoChar(buf, ')');
+ }

10a.
SUGGESTION
Handle any Column List for this table

~

10b.
Variable 'fcol' is unnecessary; just check i == 0.

~~~

11.
+ /*
+ * If there's a condition goes after the columns but if there's no
+ * column we can have conditions too
+ */

For everything related to Anum_pg_publication_rel_prattrs and
Anum_pg_publication_rel_prqual, the well-known terminology is "Column
Lists" and "Row Filters", so all the variables and comments using
those should be calling them by the proper names.

IIUC, Row Filters are independent of Column Lists, so that code
comment is not very helpful.

SUGGESTION
Handle any Row Filter for this table

~~~

12.
+ appendStringInfo(buf, " WHERE %s ", str);

Why the trailing space? You didn't have one if there was only a Column
List, but no Row Filter.

~~~

13.
+ /* if we have schemas, for sure this will go right before the WITH */

Overcomplicated comment, you don't need to say stuff like "for sure
this will go right before the WITH" because the DDL order of clauses
is obvious from the logic.

SUGGESTION
Handle FOR TABLES IN SCHEMA

~~~

14.
+
+
+ /* always add the WITH options */
+ appendStringInfo(buf, " WITH (");

Double blank line.

~~~

15.
+ /* publish string */
+ appendStringInfo(buf, "publish='");

SUGGESTION
Parameter: 'publish'

~~~

16.
+ /*
+ * by precedence we know that the insert will always be first, no need
+ * to check previous values
+ */

There is no "precedence". The 'publish' values are in this order only
because you chose to put them in this order. This whole comment seems
unnecessary.

~~~

17.
+ /* publish_generated_columns string */
+ appendStringInfo(buf, "publish_generated_columns='%s', ",
+ pub->pubgencols_type == PUBLISH_GENCOLS_NONE ? "none" : "stored");

SUGGESTION
Parameter: 'publish_generated_columns'

~~~

18.
+ /* publish_via_partition_root value */
+ appendStringInfo(buf, "publish_via_partition_root='%s')",
+ pub->pubviaroot ? "true" : "false");

18a
SUGGESTION
Parameter: 'publish_via_partition_root'

~

18b.
Putting the true/false in single quotes is unnecessary, and it looks a
bit strange

======
src/test/regress/sql/publication_ddl.sql

19. GENERAL

I have not checked the tests in detail, but here are some general comments:

19a. There is a mix of upper/lowercase -- e/g/sometimes you say
"create publication", other times you say "ALTER PUBLICATION". Be
consistent.

~

19b.
Some tests are missing comments to introduce them. The spacing
grouping the tests is also inconsistent.

~

19c.
Use proper terminology of "Column Lists" and "Row Filters" instead of
just calling them columns and conditions

~

19d.
All of those cleanup DROPs can be combined; You don't need a separate
DROP statement for every object.

~~~

20.
+
+-- a new schema for multiple schemas
+CREATE SCHEMA pub_schema_test_ddl_2;
+CREATE TABLE pub_schema_test_ddl_2.schema_tbl1 (foo int, bar int);

This could've been done earlier same place where you created the other
test schema/tables.

======

FYI, last year I wrote a blog [1]https://www.postgresql.fastware.com/pzone/tailoring-create-publication-for-logical-replication-a-complete-syntax-guide claiming to be the "complete syntax
guide" for CREATE PUBLICATION. Perhaps it is useful as a reference?

======
[1]: https://www.postgresql.fastware.com/pzone/tailoring-create-publication-for-logical-replication-a-complete-syntax-guide

Kind Regards,
Peter Smith.
Fujitsu Australia

The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested

Hello

+# run the retail DDL tests at the end make sense to not interrupt with other
+# tests in case the object names are the same to other objects previously used.
+test: publication_ddl

retail DDL tests is unclear to me. Might as well remove the comment.

+	if (pub->alltables || pub->allsequences)
+	{
+		appendStringInfo(buf, " FOR ");
+
+		if (pub->alltables)
+			appendStringInfo(buf, "ALL TABLES");
+
+		/* The sequence are published only on versions 19+ */
+		if (pub->allsequences)
+			appendStringInfo(buf,
+							 "%sALL SEQUENCE",
+							 pub->alltables ? ", " : " ");
+	}

Critical bug / typo here, I think you meant ALL SEQUENCES instead
of ALL SEQUENCE.

+ if (pub == NULL)
+ return (Datum) NULL;

use PG_RETURN_NULL() instead like everywhere else for consistency.

Typos in your commit message and everywhere else in code.. For
example:

"giving" → "given", "thuse" → "thus",
"Comprhensive" → "Comprehensive", "fro" → "for"

Need to run the patch through a spelling / typo check tool

Also, you should test pg_get_publication_ddl() with non-existent
publication as well.

Best regards

Cary Huang
Highgo Software

Hi,

Thanks for the patch. I reviewed v1 and found a few issues that need
to be addressed before commit.

1. In pg_do_publication_ddl(), schemaname is initialized to NULL before
the loop but never reset per iteration. When the first table is in a
non-default schema and the second is in public, the public table
incorrectly inherits the previous schema prefix:
```
CREATE SCHEMA s1;
CREATE TABLE s1.t1 (id int);
CREATE TABLE public.t2 (id int);
CREATE PUBLICATION p FOR TABLE s1.t1, public.t2;
SELECT pg_get_publication_ddl('p');
-- Got: ... FOR TABLE s1.t1, s1.t2 ← WRONG
-- Want: ... FOR TABLE s1.t1, t2
```

2. FOR TABLE ONLY is silently dropped, changing semantics for publications
on tables with inheritance or partitioning:
```
CREATE PUBLICATION p FOR TABLE ONLY t (col) WHERE (col > 0);
SELECT pg_get_publication_ddl('p');
-- Got: FOR TABLE t(col) WHERE (col > 0) WITH ...
-- Want: FOR TABLE ONLY t(col) WHERE (col > 0) WITH ...

```

As already mentioned there are also typos in the code. Otherwise, the idea and way lgtm.

Regards.

Hello all!!

I'm attaching the v2 of this patch! There's a lot of changes but I'll
try to mention the biggest ones:

* Adopted the new structure following the previous committed patches of
tablespaces and databases.

* Added the options for pretty and owner, also added the statement that
will alter the publication to have the owner. I didn't know if it was
required since the CREATE PUBLICATION doesn't have a way to add the
OWNER, but since all the other functions were doing the same, I think
it's the way to do it.

* Some improvements on managing the string splits, there's really
useful functions for bitmapsets and lists so I use them to decide if we
were in the first element or not.

* I decided to use the cache instead of opening the relation with a
shared lock, I hope it was the right decision since it didn't make
sense to me to have a lock for a DDL.

* I took all the comments and check the grammar

* Even more tests were added since many corner cases were not detected
in the first version, the regress tests helped a lot on finding corner
cases

* Added the EXCEPT clause that was added after the first version.

Thank you for your reviews!

--
Jonathan Gonzalez V.
EDB: https://www.enterprisedb.com

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.

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

On Wed, 20 May 2026 at 19:39, Peter Smith <smithpb2250@gmail.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.

On Wed, May 20, 2026 at 7:39 PM Peter Smith <smithpb2250@gmail.com> wrote:
...

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?

Later, I learned that this is a character added by psql to indicate
rows that span multiple lines. So, please ignore this comment.

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