Re: Logical Replication of sequences

On Tue, Jun 24, 2025 at 3:07 PM Nisha Moond <nisha(dot)moond412(at)gmail(dot)com> wrote:
>
> On Sun, Jun 22, 2025 at 8:05 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> >
> > Thanks for the comment, the attached v20250622 version patch has the
> > changes for the same.
> >
>
> Thanks for the patches, please find my review comments for patches 001 and 002:
>

Please find my further comments on patches 004 and 005:
(I've no comments for 006)

patch-004:

5) The new fetch_sequence_list() function should be guarded with version checks.
Without this, CREATE SUBSCRIPTION will always fail when a newer
subscriber (>=PG19) attempts to create a subscription to an older
publisher (<PG19).
e.g., pub1 is a publication on without-patch node, with only tables in it.

postgres=# create subscription sub_oldpub connection 'dbname=postgres
host=localhost port=8841' publication pub1;
ERROR: could not receive list of sequences from the publisher: ERROR:
relation "pg_catalog.pg_publication_sequences" does not exist
LINE 2: FROM pg_catalog.pg_publication_sequences ...
~~~

6)
+ * not_ready:
+ * If getting tables, if not_ready is false get all tables, otherwise
+ * only get tables that have not reached READY state.
+ * If getting sequences, if not_ready is false get all sequences,
+ * otherwise only get sequences that have not reached READY state (i.e. are
+ * still in INIT state).

I feel the above comment could be reworded slightly for better clarity.
Suggestion:

* If getting tables and not_ready is false, get all tables, otherwise,
* only get tables that have not reached READY state.
* If getting sequences and not_ready is false, get all sequences,
* otherwise, only get sequences that have not reached READY state (i.e. are
~~~

patch-005:

7)
+ /*
+ * Establish the connection to the publisher for sequence synchronization.
+ */
+ LogRepWorkerWalRcvConn =
+ walrcv_connect(MySubscription->conninfo, true, true,
+ must_use_password,
+ app_name.data, &err);
+ if (LogRepWorkerWalRcvConn == NULL)
+ ereport(ERROR,
+ errcode(ERRCODE_CONNECTION_FAILURE),
+ errmsg("could not connect to the publisher: %s", err));

The error message should mention the specific process or worker that
failed to connect, similar to how it's done for other workers like
slotsync or tablesync.

Suggestion:
errmsg("sequencesync worker for subscription \"%s\" could not connect
to the publisher: %s", MySubscription->name, err));
~~~

8)
+ CommitTransactionCommand();
+
+ copy_sequences(LogRepWorkerWalRcvConn, remotesequences, subid);
+
+ list_free_deep(sequences_not_synced);

Should we also free the 'remotesequences' list here?
~~~

--
Thanks,
Nisha