public inbox for [email protected]  

help / color / mirror / Atom feed

From: Hayato Kuroda (Fujitsu) <[email protected]>
To: 'Ajin Cherian' <[email protected]>
To: shveta malik <[email protected]>
Cc: Ashutosh Sharma <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Amit Kapila <[email protected]>
Subject: RE: [PATCH] Support automatic sequence replication
Date: Mon, 23 Feb 2026 01:26:10 +0000
Message-ID: <OS9PR01MB12149D9054CC7F2DC3F0D26A1F577A@OS9PR01MB12149.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CAFPTHDZiWYXoKoo4VcBYNH9a=gxDZhfkcBeXt5w6cLw4_ysyKw@mail.gmail.com>
References: <CAFPTHDZXX9WQ_X1ZfEvS248T+pKuk6SmCnXcvgPM059N1xPUfA@mail.gmail.com>
	<CAJpy0uDLUEjHHME8om1vAf6qkXCeRR6cBvkpK8yWBAC=T0ZFLA@mail.gmail.com>
	<CAFPTHDZg1JrunGgOj332hr+gUuH_Jm7skqPpYSvd-QE3yEdRDQ@mail.gmail.com>
	<CAJpy0uBz7MCSUkvFJD9ij65vBahNmY+bfCgdGKRqXovYs+K_TA@mail.gmail.com>
	<CAJpy0uDsuNqjWd-TmGBxqSS1rnVCJ3B8=SYrtxQ=Vs8kb71QFA@mail.gmail.com>
	<CAJpy0uAMWg3KcXtVBS7B0rnchLNrCCVYBByJCzAp=u5LERgtfA@mail.gmail.com>
	<CAFPTHDZwEhxhDAeqcPi0GuYN6xBs8gFXHOMUnbg3u2Xigcz4Zg@mail.gmail.com>
	<CAE9k0PmTyCU1A9YEf+MRgfeZ8yK1bAYJu=o0bH8DNUTzXejQyQ@mail.gmail.com>
	<CAA4eK1L6czEzG4mLNZSyjYC5nX0FMSjjk3csKuxPD3Ph5-7Yvw@mail.gmail.com>
	<CAJpy0uAhGQJ=msVsn2GsqWXr+YESJK6x9NBvrUtKvtvp1OVuKQ@mail.gmail.com>
	<CAJpy0uAOuu-M6wobH2wHOdTymm-cX9+MqwPyRNoOt=sPKBdCew@mail.gmail.com>
	<CAFPTHDZiWYXoKoo4VcBYNH9a=gxDZhfkcBeXt5w6cLw4_ysyKw@mail.gmail.com>

Dear Ajin,

Thanks for updating the patch. Here are my comments.

01. start_sequence_sync()
```
   /* Need to start transaction for cache lookup */
   StartTransactionCommand();
```

Here, we must check additional parameter changes, such as conninfo and passwordrequired.
Also, it's inefficient because transactions are started each time.

Can we re-use maybe_reread_subscription() here? Some parameters do not take
effect for the sequence sync worker, but it is OK to exit even if they
are changed. If we use the function, no need to include "storage/ipc.h".

02. match_previous_words

No need to remove "REFRESH SEQUENCES" anymore.

03. CopySeqResult
```
 COPYSEQ_NOWORK,
```

It describes why the copying is skipped. How about "COPYSEQ_NO_DRIFT"?

04. LogicalRepSyncSequences()

```
  oldctx = MemoryContextSwitchTo(ApplyContext);

  seq = palloc0_object(LogicalRepSequenceInfo);
  seq->localrelid = subrel->srrelid;
  seq->nspname = get_namespace_name(RelationGetNamespace(sequence_rel));
  seq->seqname = pstrdup(RelationGetRelationName(sequence_rel));
  seq->relstate = relstate;
  seqinfos = lappend(seqinfos, seq);

  MemoryContextSwitchTo(oldctx);
```

ISTM they are palloc'd but not pfree'd.

Since the sequencesync worker now has a long lifetime, we must take care of the
memory allocation/freeing more carefully. How about introducing per-interaction
memory context like ApplyMessageContext?

05. LogicalRepApplyLoop()

MaybeLaunchSequenceSyncWorker() should be called more; otherwise, the sequencesync
worker won't be launched if the worker always receives messages and WL_TIMEOUT does
not happen. Can you add most of the places under maybe_advance_nonremovable_xid()?
Personally considered, no need to add within `else if (c == PqReplMsg_PrimaryStatusUpdate)`
because it just consumes status updates from the primary.

06.
Not sure if the issue should be discussed here, but I found that sequences more
likely to go backward if users use sequences on the subscriber side.
Previously, the sync could happen based on the request, and users could understand
the risk. But now everything would be done automatically, thus they may be
surprised more.

Should we consider some ratchet mechanisms, or retain it now because it's not
expected usage?

E.g., `nextval()` is called three times, and synchronization occurs between them.

```
subscriber=# SELECT nextval('seq');
 nextval
---------
       2
(1 row)

subscriber=# SELECT nextval('seq');
 nextval
---------
       3
(1 row)

subscriber=# -- synchronization happened
subscriber=# SELECT nextval('seq');
 nextval
---------
       1
(1 row)
```

07.
Question: Can we introduce an intermediate state, such as SYNC, to clarify
whether synchronization is proceeding?

Best regards,
Hayato Kuroda
FUJITSU LIMITED

view thread (58+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: RE: [PATCH] Support automatic sequence replication
  In-Reply-To: <OS9PR01MB12149D9054CC7F2DC3F0D26A1F577A@OS9PR01MB12149.jpnprd01.prod.outlook.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox