Horton

Thread

  1. Re: Add pg_get_publication_ddl function

    Jonathan Gonzalez V. <jonathan.abdiel@gmail.com> — 2026-05-17T21:27:15Z

    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

  2. Re: Add pg_get_publication_ddl function

    Peter Smith <smithpb2250@gmail.com> — 2026-05-20T09:39:14Z

    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

  3. Re: Add pg_get_publication_ddl function

    Japin Li <japinli@hotmail.com> — 2026-05-20T10:00:56Z

    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.

  4. Re: Add pg_get_publication_ddl function

    Peter Smith <smithpb2250@gmail.com> — 2026-05-20T22:27:15Z

    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