Re: Logical Replication of sequences

Few comments:

1)
The message of patch001 says:
----
When a sequence is synchronized to the subscriber, the page LSN of the
sequence from the publisher is also captured and stored in
pg_subscription_rel.srsublsn. This LSN will reflect the state of the
sequence at the time of synchronization. By comparing the current LSN
of the sequence on the publisher (via pg_sequence_state()) with the
stored LSN on the subscriber, users can detect if the sequence has
advanced and is now out-of-sync. This comparison will help determine
whether re-synchronization is needed for a given sequence.
----

I am unsure if pg_subscription_rel.srsublsn can help diagnose thatseq
is out-of-sync. The page-lsn can be the same but the sequence-values
can still be unsynchronized. Same page-lsn does not necessarily mean
synchronized sequences.

patch002:
2)
+ if (schemaidlist && (pubform->puballtables || pubform->puballsequences))
+ {
+ if (pubform->puballtables && pubform->puballsequences)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("publication \"%s\" is defined as FOR ALL TABLES, ALL SEQUENCES",
+ NameStr(pubform->pubname)),
+ errdetail("Schemas cannot be added to or dropped from FOR ALL
TABLES, ALL SEQUENCES publications."));
+ else if (pubform->puballtables)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("publication \"%s\" is defined as FOR ALL TABLES",
+ NameStr(pubform->pubname)),
+ errdetail("Schemas cannot be added to or dropped from FOR ALL TABLES
publications."));
+ else
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("publication \"%s\" is defined as FOR ALL SEQUENCES",
+ NameStr(pubform->pubname)),
+ errdetail("Schemas cannot be added to or dropped from FOR ALL
SEQUENCES publications."));
+ }

Do you think we can make it as:

if (schemaidlist && (pubform->puballtables || pubform->puballsequences))
{
ereport(ERROR,
errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("Schemas cannot be added to or dropped from publication
defined for ALL TABLES, ALL SEQUENCES, or both"));
}

IMO, a generic message such as above is good enough.
Same is applicable to the next 'Tables or sequences' message.

patch003:
3)
/*
* Return whether the subscription currently has any relations.
*
* Note: Unlike HasSubscriptionRelations(), this function relies on cached
* information for subscription relations. Additionally, it should not be
* invoked outside of apply or tablesync workers, as MySubscription must be
* initialized first.
*/
bool
HasSubscriptionRelationsCached(void)
{
/* We need up-to-date subscription tables info here */
return FetchRelationStates(NULL);
}

a) The comment mentions old function name HasSubscriptionRelations()
b) I think this function only worries about tables as we are passing
has_pending_sequences as NULL.

So does the comment and function name need amendments from relation to table?

patch005:
4)
+ * root partitioned tables. This is not applicable for FOR ALL SEQEUNCES
+ * publication.

a) SEQEUNCES --> SEQUENCES

b) We may say (omit FOR):
This is not applicable to ALL SEQUENCES publication.

5)
* If getting tables and not_ready is false get all tables, otherwise,
* only get tables that have not reached READY state.
* If getting sequences and not_ready is false get all sequences,
* otherwise, only get sequences that have not reached READY state (i.e. are
* still in INIT state).

Shall we rephrase to:
/*
* If getting tables and not_ready is false, retrieve all tables;
* otherwise, retrieve only tables that have not reached the READY state.
*
* If getting sequences and not_ready is false, retrieve all sequences;
* otherwise, retrieve only sequences that are still in the INIT state
* (i.e., have not reached the READY state).
*/

Reviewing further..

thanks
Shveta