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
