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()?
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?
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.
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.
Apart from the above, I have modified comments at various places in
the patch, see attached.
--
With Regards,
Amit Kapila.
