On Wed, Apr 1, 2026 at 11:12 PM Peter Smith <[email protected]> wrote: > > Hi Sawada-San > > Some review comments for v8-0001. > > ====== > Commit message > > 1. > The existing a VARIADIC array form of pg_get_publication_tables() is > preserved for backward compatibility. Tablesync workers use the new > two-argument form when connected to a publisher running PostgreSQL 19 > or later. > > ~ > > Typo? "The existing a VARIADIC" > > ====== > src/backend/catalog/pg_publication.c > > is_publishable_table: > > 2. > + /* > + * Sequences are publishable according to is_publishable_class() so > + * explicitly exclude here. > + */ > + if (relform->relkind != RELKIND_SEQUENCE && > + is_publishable_class(tableoid, relform)) > + { > + ReleaseSysCache(tuple); > + return true; > + } > > It seemed strange to say that "sequences are publishable according to > is_publishable_class() so explicitly exclude", but then you proceed to > call is_publishable_class() anyway. > > Maybe using a variable, and a different comment could be a better way > of expressing this? > > SUGGESTION > bool ret; > ... > /* Sequences are not tables, so this function returns false. */ > if (relform->relkind == RELKIND_SEQUENCE) > ret = false; > else > ret = is_publishable_class(tableoid, relform); > > ReleaseSysCache(tuple); > return ret; > > ~~~ > > is_table_publishable_in_publication: > > 3. > + * A helper function for pg_get_publication_tables() to check whether the > + * table of the given relid is published for the specified publication. > > /table of the given relid/table with the given relid/ > > /is published for the/is published in the/ > > ~~~ > > pg_get_publication_tables: > > 4. > + * If filter_by_relid is true, only the row for target_relid is returned; > + * if target_relid does not exist or is not part of the publications, zero > + * rows are returned. If filter_by_relid is false, rows for all tables > + * within the specified publications are returned and target_relid is > + * ignored. > > Should that say "only the row(s) for target_relid", e.g. possibly > plural, because if same table is in multiple publications then there > are be multiple result rows, right? > > ====== > src/include/catalog/pg_proc.dat > > 5. > Missed my previous [1] review comment #4? > > First arg of pg_get_publication_tables_a should be plural 'pubnames', > same as first arg of pg_get_publication_tables_b. > > ====== > src/test/regress/sql/publication.sql > > 6. > +CREATE PUBLICATION pub_all_except_no_viaroot FOR ALL TABLES EXCEPT > (TABLE tbl_parent, gpt_test_sch.tbl_sch) WITH > (publish_via_partition_root = true); > > Why is this publication called '...no_viaroot' when > publish_via_partition_root = true? > > ~~~ > > 7. > +-- test for the EXCLUDE clause > > Typo? /EXCLUDE clause/EXCEPT clause/ >
Thank you for the comments! I've pushed the patch after incorporating these points. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
