On Fri, Oct 31, 2025 at 7:34 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> ======
> .../replication/logical/sequencesync.c
>
> 2.
> + * A single sequencesync worker is responsible for synchronizing all sequences
> + * in INIT state in pg_subscription_rel. It begins by retrieving the list of
> + * sequences flagged for synchronization. These sequences are then processed
> + * in batches, allowing multiple entries to be synchronized within a single
> + * transaction. The worker fetches the current sequence values and page LSNs
> + * from the remote publisher, updates the corresponding sequences on the local
> + * subscriber, and finally marks each sequence as READY upon successful
> + * synchronization.
> + *
>
> Those first 2 sentences seem repetitive because AFAIK "in INIT state"
> and "flagged for synchronization" are exactly the same thing.
>
> SUGGESTION
> A single sequencesync worker is responsible for synchronizing all
> sequences. It begins by retrieving the list of sequences that are
> flagged for needing synchronization (e.g. those with INIT state).
>
/e.g/i.e. We don't have multiple such states, so let's be specific.
> ~~~
>
> 4.
> Now that this function is in 2 parts, I think each part should be
> clearly identified with comments, something like:
>
> PART 1:
> /*
> * 1. Extract sequence information from the tuple slot received from the
> * publisher
> */
>
> PART 2:
> /*
> * 2. Compare the remote sequence definition to the local sequence definition,
> * and report any discrepancies.
> */
>
I don't see the need for such explicit comments as the same is
apparent from the code.
> ~~~
>
> 5.
> + seqinfo_local = (LogicalRepSequenceInfo *) list_nth(seqinfos, seqidx);
> + seqinfo_local->found_on_pub = true;
> + *seqinfo = seqinfo_local;
>
> Is this separate `seqinfo_local` variable needed? It seems always
> unconditionally assigned to the parameter, so you might as well just
> do without the extra variable. Maybe just rename the parameter as
> `seqinfo_local`?
>
We can do without a local variable as well but it appears neat to
modify and use local variable. I think it is a matter of personal
choice, so either way is fine but I would prefer using local variable
for this.
>
> 12.
> There is still a yet-to-be-implemented test combination as previously
> reported [1, comment #3], right?
>
I don't think adding similar negative tests adds value. So, we can skip those.
--
With Regards,
Amit Kapila.
