Re: Logical Replication of sequences

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