On Thu, 30 Oct 2025 at 16:20, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Oct 30, 2025 at 8:12 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > 3.
> > +static void
> > +report_sequence_errors(StringInfo insuffperm_seqs, StringInfo mismatched_seqs,
> > + StringInfo insuffperm_seqs)
> >
> > Perhaps this function should also have an assertion:
> >
> > Assert(insuffperm_seqs->len || mismatched_seqs->len || insuffperm_seqs->len);
> >
>
> I don't think such an assertion is helpful. As of now, this is called
> from only one place where we ensure that one of the conditions
> mentioned in assert is true but in future even if we call that without
> any condition being true, it will just be a harmless call.
>
> Other review comments:
> ===================
> 1. In copy_sequences, we start a transaction before getting the
> changes from the publisher and keep using it till we copy all
> sequences. If we don't need to retain lock while querying from a
> publisher, then can't we even commit the xact before that and start a
> new one for the copy_sequence part? Is it possible that we fetch the
> required sequence information in LogicalRepSyncSequences()?
Change it to fetching it in LogicalRepSyncSequences
> 2. Isn't it better to add some comments as to why we didn't decide to
> retain locks on required sequences while querying the publisher, and
> rather re-validate them later?
Added comments for the same
> 3. To avoid a lot of parameters in get_remote_sequence_info and
> validate_sequence(), can we have one function call say
> get_and_validate_seq_info()? It will retrieve only the parameters
> required by copy_sequence.
Modified
> 4.
> validate_sequence()
> {
> ...
> + /* Sequence was concurrently invalidated? */
> + if (!seqinfo->entry_valid)
> + {
> + ReleaseSysCache(tup);
> + return COPYSEQ_SKIPPED;
> + }
>
> So, if the sequence is renamed concurrently, we get the cause as
> SKIPPED but shouldn't it be COPYSEQ_MISMATCH? Is it possible to
> compare the local sequence name with the remote name in
> validate_sequence() after taking lock on sequence relation? If so, we
> can return the state as COPYSEQ_MISMATCH.
Modified
> Apart from the above, I have modified comments at various places in
> the patch, see attached.
Thanks, I have merged them.
The attached v20251030 version patch has the changes for the same.
I have also addressed Peter's comments from [1] and Hou-san's comments from [2].
[1] - https://www.postgresql.org/message-id/CAHut%2BPt73A8XNnBSiF5wEfqM4mT6ofVwW9BXw3oUjbGsaYQN_g%40mail.gmail.com
[2] -
https://www.postgresql.org/message-id/TY4PR01MB16907CAA300EE41232FB0BA8194FAA%40TY4PR01MB16907.jpnprd01.prod.outlook.com
Regards,
Vignesh
