RE: [PATCH] Support automatic sequence replication
Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
From: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
To: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, Amit Kapila <amit.kapila16@gmail.com>, Ajin Cherian <itsajin@gmail.com>
Cc: shveta malik <shveta.malik@gmail.com>, Ashutosh Sharma <ashu.coek88@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2026-03-04T04:20:47Z
Lists: pgsql-hackers
Dear Hou,
Thanks for updating the patch. Here are my comments.
01.
```
<para>
A <firstterm>sequence synchronization worker</firstterm> will be started
- after executing any of the above subscriber commands, and will exit once the
- sequences are synchronized.
+ after executing any of the above subscriber commands. The worker will
+ remain running for the life of the subscription, periodically
+ synchronizing all published sequences.
</para>
```
I think it's not accurate, because REFRESH SEQUENCE command does not need the
sequencesync worker anymore.
02.
```
void
GetSequence(Relation seqrel, int64 *last_value, bool *is_called)
```
I think GetSequence() itself should conitan the permission check like SetSequence().
My idea is to set NULL for last_value and is_called in this case.
03.
```
/*
* Verify that the current user has SELECT privilege on the sequence.
* This is required to read the sequence state below.
*/
aclresult = pg_class_aclcheck(seqoid, GetUserId(), ACL_SELECT);
if (aclresult != ACLCHECK_OK)
return COPYSEQ_INSUFFICIENT_PERM;
/* Get current local sequence state */
GetSequence(sequence_rel, &local_last_value, &local_is_called);
```
If you accept above comment, this part can be simplified.
04.
```
/*
* get_and_validate_seq_info
*
* Extracts remote sequence information from the tuple slot received from the
* publisher, and validates it against the corresponding local sequence
* definition.
*/
static CopySeqResult
get_and_validate_seq_info(TupleTableSlot *slot, Relation *sequence_rel,
LogicalRepSequenceInfo **seqinfo, int *seqidx,
List *seqinfos)
```
It can return COPYSEQ_SUCCESS, but it might be misleading; copying is not
happened yet here. How about returning boolean and add another argument to
indicate the reason if the validation is failed?
05.
LogicalRepSyncSequences() starts the transaction and read sequences every time.
Can we cache the seqinfos to reuse in the next iteration? My idea is to introduce
a syscache callback for the pg_subscription_relto invalidate the cached list.
How about measuring performance once and considering it's a good improvement?
Best regards,
Hayato Kuroda
FUJITSU LIMITED
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Don't include wait_event.h in pgstat.h
- 868825aaeb40 19 (unreleased) cited
-
Don't include proc.h in shm_mq.h
- a2c89835f512 19 (unreleased) cited
-
Fix unsafe RTE_GROUP removal in simplify_EXISTS_query
- 77c7a17a6e5f 19 (unreleased) cited
-
Add sequence synchronization for logical replication.
- 5509055d6956 19 (unreleased) cited