Re: Logical Replication of sequences

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.
~~~~

--
Thanks,
Nisha