MIME-Version: 1.0
References: <CAFPTHDZXX9WQ_X1ZfEvS248T+pKuk6SmCnXcvgPM059N1xPUfA@mail.gmail.com>
 <CAJpy0uDLUEjHHME8om1vAf6qkXCeRR6cBvkpK8yWBAC=T0ZFLA@mail.gmail.com> <CAFPTHDZg1JrunGgOj332hr+gUuH_Jm7skqPpYSvd-QE3yEdRDQ@mail.gmail.com>
In-Reply-To: <CAFPTHDZg1JrunGgOj332hr+gUuH_Jm7skqPpYSvd-QE3yEdRDQ@mail.gmail.com>
From: Peter Smith <smithpb2250@gmail.com>
Date: Fri, 6 Feb 2026 18:38:55 +1100
Message-ID: <CAHut+PuNZWTr41pw8Xi1fhFmCaNUSixxdM9dnjrjyUQvwsZrqQ@mail.gmail.com>
Subject: Re: [PATCH] Support automatic sequence replication
To: Ajin Cherian <itsajin@gmail.com>
Cc: shveta malik <shveta.malik@gmail.com>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CAHut%2BPuNZWTr41pw8Xi1fhFmCaNUSixxdM9dnjrjyUQvwsZrqQ%40mail.gmail.com>
Precedence: bulk

Hi Ajin.

Some review comments for patch v2-0001.

======
.../replication/logical/sequencesync.c

copy_sequences:

1.
+ return drift_detected;

This seems a bit strange. And it is not doing quite what the function
comment says it does. I felt you should have another variable like
'sequences_copied', which is set to true only when that
'batch_succeeded_count++' is incremented. This is what you ultimately
want to return. IMO, the variable 'drift_detected' isn't needed at
all.

======
src/test/subscription/t/036_sequences.pl

2.
##########
## ALTER SUBSCRIPTION ... REFRESH PUBLICATION should cause sync of new
# sequences of the publisher.
##########

# Create a new sequence 'regress_s2', and update existing sequence 'regress_s1'
$node_publisher->safe_psql(5.
'postgres', qq(
CREATE SEQUENCE regress_s2;
INSERT INTO regress_seq_test SELECT nextval('regress_s2') FROM
generate_series(1,100);

-- Existing sequence
INSERT INTO regress_seq_test SELECT nextval('regress_s1') FROM
generate_series(1,100);
));

~

IIUC, you are no longer sync of testing "existing sequences" in this
test part, so you might also want to remove that comment and INSERT
for 'regress_s1'.

======
Kind Regards,
Peter Smith.
Fujitsu Australia