Hi, Thanks for the review.
On Thu, 30 Apr 2026 at 10:00, vignesh C <[email protected]> wrote: > > > Few comments: > 1) Since we are setting the variable to NULL for every sequence now, > this is not required: > @@ -246,6 +246,8 @@ get_and_validate_seq_info(TupleTableSlot *slot, > Relation *sequence_rel, > Form_pg_sequence local_seq; > LogicalRepSequenceInfo *seqinfo_local; > > + *sequence_rel = NULL; > + > *seqidx = DatumGetInt32(slot_getattr(slot, ++col, &isnull)); > Assert(!isnull); > I had added it as defence-in-depth, but yeah can be removed. > > 2) Creating a subscription is costly as it has more work to do as it > has to sync all relations and requires more processes to be started, > instead shall we use "ALTER SUBSCRIPTION ... SET CONNECTION" followed > by "ALTER SUBSCRIPTION ... REFRESH SEQUENCES" > +$node_subscriber->safe_psql( > + 'postgres', > + "CREATE SUBSCRIPTION regress_seq_sub_no_select CONNECTION > '$publisher_limited_connstr' PUBLICATION regress_seq_pub WITH > (disable_on_error = true)" > +); > Makes sense. > > 3) You can use sequence name as regress_s5 to be consistent with the > other sequence names nearby or alternatively you can change the privs > of an already existing sequence: > + CREATE ROLE regress_seq_repl LOGIN REPLICATION; > + CREATE SEQUENCE regress_no_select; > + GRANT USAGE ON SCHEMA public TO regress_seq_repl; > Sounds good. > > 4) I feel this is not required: > +$result = $node_subscriber->safe_psql('postgres', 'SELECT 1'); > +is($result, '1', > + 'subscriber remains running after publisher returns NULL > sequence data'); > Yeah, you are right. I've made these changes, attaching patch v4. Please review and let me know. Regards, Ayush
v4-0001-Fix-stale-relation-close-in-sequence-synchronization.patch
Description: Binary data
