Re: Logical Replication of sequences

On Mon, 28 Jul 2025 at 13:26, Hayato Kuroda (Fujitsu)
<kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
>
> Dear Vignesh,
>
> Here are remained comments for v20250723 0003-0005. I've not checked the latest version.
>
> 01.
> ```
> * PostgreSQL logical replication: common synchronization code
> ```
>
> How about: "Common code for synchronizations"? Since this file locates in
> replication/logical, initial part is bit trivial.

I felt this is ok as tablesync.c has a similar file header.

> 02.
>
> How do you feel to separate header file into syncutils.h file? We can put some
> definitions needed for synchronizations.

Currently we only have 5 functions here, we can add it if more
functions get added here.

> 03.
> ```
> -extern void getSubscriptionTables(Archive *fout);
> +extern void getSubscriptionRelations(Archive *fout);
> ```
>
> Assuming that this function obtains both tables and sequences. I'm now wondering
> we can say "relation" for both the tables and sequences in the context. E.g.,
> getTableData()->makeTableDataInfo() seems to obtain both table and sequence data,
> and dumpSequenceData() dumps sequences. How about keep getSubscriptionTables, or
> getSubscriptionTablesAndSequences?

Retained it to getSubscriptionTables as earlier. As in case of pg_dump
code it is used like that, that is function having table name not only
handle table but other relation object too.

> 04.
> ```
> /*
> * dumpSubscriptionTable
> * Dump the definition of the given subscription table mapping. This will be
> * used only in binary-upgrade mode for PG17 or later versions.
> */
> static void
> dumpSubscriptionTable(Archive *fout, const SubRelInfo *subrinfo)
> ```
>
> If you rename getSubscriptionTables, dumpSubscriptionTable should be also renamed.

Since we are not renaming getSubscriptionTables, nothing to do here

> 05.
> ```
> /*
> * Gets list of all relations published by FOR ALL SEQUENCES publication(s).
> */
> List *
> GetAllSequencesPublicationRelations(void)
> ```
>
> It looks very similar with GetAllTablesPublicationRelations(). Can we combine them?
> I feel we can pass the kind of target relation and pubviaroot.

Modified

> 06.
> ```
> --- a/src/backend/catalog/pg_subscription.c
> +++ b/src/backend/catalog/pg_subscription.c
> @@ -27,6 +27,7 @@
> #include "utils/array.h"
> #include "utils/builtins.h"
> #include "utils/fmgroids.h"
> +#include "utils/memutils.h"
> ```
>
> I can build without the header.

Removed this

> 07.
> ```
> + /*
> + * XXX: If the subscription is for a sequence-only publication, creating a
> + * replication origin is unnecessary because incremental synchronization
> + * of sequences is not supported, and sequence data is fully synced during
> + * a REFRESH, which does not rely on the origin. If the publication is
> + * later modified to include tables, the origin can be created during the
> + * ALTER SUBSCRIPTION ... REFRESH command.
> + */
> ReplicationOriginNameForLogicalRep(subid, InvalidOid, originname, sizeof(originname));
> replorigin_create(originname);
> ```
>
> The comment is bit misleading because currently we create the replicaton origin
> in the case. Can you clarify the point like:
> ```
> XXX: Now the replication origin is created for all the cases, but it is unnecessary
> when the subcription is for a sequence-only publicaiton....
> ```

Modified

> 08.
> ```
> + * XXX: If the subscription is for a sequence-only publication,
> + * creating this slot is unnecessary. It can be created later
> + * during the ALTER SUBSCRIPTION ... REFRESH PUBLICATION or ALTER
> + * SUBSCRIPTION ... REFRESH PUBLICATION SEQUENCES command, if the
> + * publication is updated to include tables.
> ```
>
> Same as above.

Modified

> 09.
> ```
> + * Assert copy_data is true.
> + * Assert refresh_tables is false.
> + * Assert refresh_sequences is true
> ```
>
> IIUC assertions are rarely described the comment stop function. If you want to
> add, we should say like:
> ```
> In Assert enabled builds, we verify that parameters are passed correctly...
> ```

Removed these

> 10.
> ```
> +#ifdef USE_ASSERT_CHECKING
> + /* Sanity checks for parameter values */
> + if (resync_all_sequences)
> + Assert(copy_data && !refresh_tables && refresh_sequences);
> +#endif
> ```
>
> How about below, which does not require ifdef.
>
> ```
> Assert(!resync_all_sequences ||
> (copy_data && !refresh_tables && refresh_sequences));
>
> ```

Modified

> 11.
> ```
> + bool issequence;
> + bool istable;
> ```
>
> Isn't it enough to use istable?

Removed both of them add added relkind variable, which will also help
next comment

> 12.
> ```
> + /* Relation is either a sequence or a table */
> + issequence = get_rel_relkind(subrel->srrelid) == RELKIND_SEQUENCE;
> ```
>
> How about adding an Assert() to ensure the relation is either of table or sequence?

Modified

> 13.
> ```
> + * not_ready:
> + * 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
> + * still in INIT state).
> ```
> Two parts decribe mostly same point. How about:
> If true, this function returns only the relations that are not in a ready state.
> Otherwise returns all the relations of the subscription.

Since we have get_tables and get_sequences, the existing version is more clear

> 14.
> ```
> + char table_state;
> ```
>
> It should be `relation_state`.

Modified

> 15.
> ```
> +#define SEQ_LOG_CNT_INVALID 0
> ```
>
> Can you add comment how we use it?

We have the following comment in the SetSequence function:
* log_cnt is currently used only by the sequence syncworker to set the
* log_cnt for sequences while synchronizing values from the publisher.

Here all others other than sequence syncworker will pass
SEQ_LOG_CNT_INVALID to set log_cnt to 0.

I felt this is enough.

> 16.
> ```
> +
> + TimestampTz last_seqsync_start_time;
> ```
>
> I can't find the user of this attribute, is it needed?

This is used in check_and_launch_sync_worker to check in case of
failures if it has elapsed the wal_retrieve_retry_interval and start
the sequence sync worker.

> 17.
> ```
> + FetchRelationStates(&has_pending_sequences);
> + ProcessSyncingTablesForApply(current_lsn);
> + if (has_pending_sequences)
> + ProcessSyncingSequencesForApply();
> ```
>
> IIUC we do not always call ProcessSyncingSequencesForApply() because it would acquire
> the LW lock. Can you clariy it as comments?

FetchRelationStates will indicate if there are any sequences to be
synchronized or not by setting has_pending_sequences. If there are no
sequences to be synchronized, no point in calling
ProcessSyncingSequencesForApply. I felt no need to add any comments
for this. Thoughts?

> 18.
> ```
> + case WORKERTYPE_SEQUENCESYNC:
> + /* Should never happen. */
> + Assert(0);
> ```
>
> Should we call elog(ERROR) instead of Assert(0) like another case?

Modified

> 19.
> ```
> /* Find the leader apply worker and signal it. */
> logicalrep_worker_wakeup(MyLogicalRepWorker->subid, InvalidOid);
> ```
>
> Do we have to signal to the leader even when the sequence worker exits?

Consider the case when the last run apply worker could not allocate an
tablesync worker because there was no worker that time. Now after the
sequence sync worker signals apply worker, apply worker can check and
see if it can be alloted for it, also it can check for any new
sequences that need to be synced because of refresh publication.

Thanks for the comments, the attached v20250728 version patch has the
changes for the same.

Regards,
Vignesh