On Mon, Dec 22, 2025 at 4:41 AM Peter Smith <smithpb2250@gmail.com> 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
