Horton

Re: Logical Replication of sequences

Peter Smith <smithpb2250@gmail.com>

View thread
From: Peter Smith <smithpb2250@gmail.com>
To: vignesh C <vignesh21@gmail.com>
Cc: Shlok Kyal <shlok.kyal.oss@gmail.com>, Chao Li <li.evan.chao@gmail.com>, shveta malik <shveta.malik@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, Shinya Kato <shinya11.kato@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, Dilip Kumar <dilipbalaut@gmail.com>, Nisha Moond <nisha.moond412@gmail.com>, Peter Eisentraut <peter@eisentraut.org>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Euler Taveira <euler@eulerto.com>, Michael Paquier <michael@paquier.xyz>, "Jonathan S. Katz" <jkatz@postgresql.org>
Date: 2025-12-21T23:10:51Z
Lists: pgsql-hackers 
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.

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