Re: Logical Replication of sequences

On Wed, 17 Sept 2025 at 10:06, shveta malik <shveta.malik@gmail.com> wrote:
>
> 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.

Currently we don't WAL log every sequence change, it happens once in
32 changes. I felt this was fine. Do you want anything additionally to
be included?

> 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.

I'm ok with the generic error message, modified accordingly.

> 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?

Modified

> 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.

Modified

> 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).
>  */

Modified

Thanks for the comments. The attached patches has the changes for the
same. Also Shlok's comments from [1] have also been addressed.

[1] - https://www.postgresql.org/message-id/CANhcyEUHS%2BkjS0AQhVEgLF0Yf0dEZkxczEriN4su5mQqZnxU8g%40mail.gmail.com

Regards,
Vignesh