RE: [PATCH] Support automatic sequence replication

On Friday, February 27, 2026 7:07 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> Few comments:
> =============
> 1.
> + oldctx = MemoryContextSwitchTo(SequenceSyncContext);
> 
> - initStringInfo(&app_name);
> - appendStringInfo(&app_name, "pg_%u_sequence_sync_" UINT64_FORMAT,
> - MySubscription->oid, GetSystemIdentifier());
> + /* Process sequences */
> + sequence_copied = copy_sequences(conn, seqinfos);
> 
> - /*
> - * 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("sequencesync worker for subscription \"%s\" could not connect to
> the publisher: %s",
> -    MySubscription->name, err));
> -
> - pfree(app_name.data);
> -
> - copy_sequences(LogRepWorkerWalRcvConn);
> + MemoryContextSwitchTo(oldctx);
> 
> It is better to switch to SequenceSyncContext at the caller of
> LogicalRepSyncSequences similar to what we are doing for
> ApplyMessageContext.

Agreed. I changed as suggested.

> 
> 2.
> @@ -4221,6 +4221,7 @@ LogicalRepApplyLoop(XLogRecPtr last_received)
>   ProcessConfigFile(PGC_SIGHUP);
>   }
> 
> +
>   if (rc & WL_TIMEOUT)
> 
> Spurious line addition.
> 
> 3. Apart from above, the attached patch contains comments and cosmetic
> changes.

Thanks for the patch, I have merged them into 0001.

Here is the V8 patch set addressing the previous review comments:

- For 0001, I noticed that the GetSequence() function added in the patch
  fetches the local sequence value without any privilege check. This
  allows the worker to read sequence data even without proper SELECT
  privilege, which seems unsafe. I've added a SELECT privilege check
  before fetching the sequence value. Additionally, I've updated several
  comments, made cosmetic changes, commit message update, and run
  pgindent on all patches.

- 0002 includes the changes to synchronize sequences directly in the
  REFRESH SEQUENCES command

Best Regards,
Hou zj