Re: Logical Replication of sequences

On Wed, 28 May 2025 at 20:52, Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
>
> On Thu, May 22, 2025 at 10:42 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> >
> > The attached v20250522 patch has the changes for the same.
> >
>
> Thank you for the patches, please find comments for patch-0004.
>
> 1)
> +/*
> + * report_error_sequences
> + *
> + * Logs a warning listing all sequences that are missing on the publisher,
> + * as well as those with value mismatches relative to the subscriber.
> + */
> +static void
> +report_error_sequences(StringInfo missing_seqs, StringInfo mismatched_seqs)
>
> The function description should be updated to reflect the recent
> changes, as it now raises an error instead of issuing a warning.
>
> 2)
> + ereport(ERROR, errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("%s", combined_error_msg->data));
> +
> + destroyStringInfo(combined_error_msg);
> +}
>
> I think we can remove destroyStringInfo() as we will never reach here
> in case of error.
>
> 3)
> + * we want to avoid keeping this batch transaction open for extended
> + * periods so it iscurrently limited to 100 sequences per batch.
> + */
>
> typo : iscurrently / is currently
>
> 4)
> + HeapTuple tup;
> + Form_pg_sequence seqform;
> + LogicalRepSequenceInfo *seqinfo;
> +
> [...]
> + Assert(seqinfo);
>
> Since there's an assertion for 'seqinfo', it would be safer to
> initialize it to NULL to avoid any unexpected behavior.
>
> 6)
> + if (missing_seqs->len || mismatched_seqs->len)
> + report_error_sequences(missing_seqs, mismatched_seqs);
>
> I think it would be helpful to add a comment for this check, perhaps
> something like:
> /*
> * Report an error if any sequences are missing on the remote side
> * or if local sequence parameters don't match with the remote ones.
> */
> Please rephrase if needed.

These comments are handled in the attached v2025029 version patch.

Regards,
Vignesh