On Thu, 30 Apr 2026 at 10:23, Ayush Tiwari <ayushtiwari.slg01@gmail.com> wrote:
>
> 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.
Thanks for the updated patch. After this error occurs, it currently
emits the message 'missing sequence on publisher
("public.regress_no_select")'. This can be misleading, as it suggests
the sequence does not exist, whereas the actual cause may be
insufficient privileges. As a result, users may find it difficult to
diagnose and resolve the issue.
Consider updating the error reported in report_sequence_errors to
convey that the failure could be due to either a missing sequence or
insufficient privileges (e.g., "missing sequence or insufficient
privilege on publisher"). While making this change, also update the
missing_seqs_idx variable name and the associated comments to
accurately reflect the broader scope of the condition.
Regards,
Vignesh
