Re: Logical Replication of sequences - Mailing list pgsql-hackers

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