Re: [PATCH] Fix stale relation close in sequence synchronization

Hi,

Thanks for the review.

On Thu, 30 Apr 2026 at 10:00, vignesh C <vignesh21@gmail.com> 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