On Mon, Dec 22, 2025 at 4:41 AM Peter Smith <[email protected]> wrote: > > Hi Vignesh, > > A couple of review comments for v2-0001 > > ====== > src/backend/catalog/pg_publication.c > > pg_get_publication_tables: > > 1. > if (pub_elem->alltables) > pub_elem_tables = GetAllPublicationRelations(RELKIND_RELATION, > pub_elem->pubviaroot); > - else > + else if (!pub_elem->allsequences) > { > List *relids, > *schemarelids; > @@ -1203,8 +1203,13 @@ pg_get_publication_tables(PG_FUNCTION_ARGS) > table_infos = lappend(table_infos, table_info); > } > > - /* At least one publication is using publish_via_partition_root. */ > - if (pub_elem->pubviaroot) > + /* > + * At least one publication is using publish_via_partition_root. > + * Skip sequences only publications, as publish_via_partition_root > + * is applicable only to table publications. > + */ > + if (pub_elem->pubviaroot && !PUB_HAS_SEQUENCES_ONLY(pub_elem->allsequences, > + pub_elem->alltables)) > viaroot = true; > > Won't it be simpler to check this up-front and then just 'continue'? > Then you wouldn't have to handle "sequence only" for the rest of the > loop logic.
+1. It will simplify the code. > e.g. > > pub_elem = ... > > /* Skip this publication if no TABLES are published. */ > if (PUB_HAS_SEQUENCES_ONLY(pub_elem->allsequences, pub_elem->alltables) > continue; > > if (pub_elem->alltables) > ... > else > ... > > ====== > src/backend/commands/publicationcmds.c > > 2. > - if (!pubform->puballtables && publish_via_partition_root_given && > - !publish_via_partition_root) > + if (!pubform->puballtables && !pubform->puballsequences && > + publish_via_partition_root_given && !publish_via_partition_root) > > I felt this modified condition ought to be expressed as: > > if (!PUB_HAS_SEQUENCES_ONLY(...) && <original condition> > > ====== > Kind Regards, > Peter Smith. > Fujitsu Australia
