On Wed, 20 May 2026 at 19:39, Peter Smith <[email protected]> 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.
