Re: issue with synchronized_standby_slots

From: Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com>
To: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
Cc: Alexander Kukushkin <cyberdemn(at)gmail(dot)com>, Rahila Syed <rahilasyed90(at)gmail(dot)com>, Fabrice Chapuis <fabrice636861(at)gmail(dot)com>, "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: issue with synchronized_standby_slots
Date: 2025-09-11 05:30:02
Message-ID: CANhcyEUeD0yO=XnmvyOCKAm_8vpFobqZoAX-M4QHGowTyOXr-w@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Thu, 11 Sept 2025 at 09:20, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Sep 11, 2025 at 9:02 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Wed, Sep 10, 2025 at 5:23 PM Alexander Kukushkin <cyberdemn(at)gmail(dot)com> wrote:
> > >
> > > On Wed, 10 Sept 2025 at 13:34, Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
> > >>
> > >> I think we should also add a parsing check for slot names specified in
> > >> the GUC synchronize_standby_slots as suggested by Amit in [1].
> > >> I made the changes in the above for the same and attached the updated patch.
> > >
> > >
> > > I agree, validating that list contains valid replication slot names is a good idea.
> > > However, you used ReplicationSlotValidateName() function, which is not a good fit for it, especially when it is called with elevel=ERROR in postmaster.
> > >
> >
> > Can you please explain why you think so? And what is your proposal for the same?
> >
>
> You are right and I think we should use WARNING here as is used in
> check_primary_slot_name() for the same function call. For ERROR
> reporting, we need to use GUC_check_* functions. Also, probably the
> ERROR during startup could lead to shutdown.
>
I tested by setting elevel=ERROR and elevel=WARNING in the function
ReplicationSlotValidateName.

For elevel=ERROR,
After hitting ereport inside function ReplicationSlotValidateName, the
PG_CATCH() in 'call_string_check_hook' and process is terminated.
Server logs are
2025-09-11 10:01:17.909 IST [1995206] FATAL: replication slot name
"myslot1*" contains invalid character
2025-09-11 10:01:17.909 IST [1995206] HINT: Replication slot names
may only contain lower case letters, numbers, and the underscore
character.

For level=WARNING,
Even after hitting the ereport, it is continuing with the normal flow of code.
Server logs are:
2025-09-11 10:27:30.195 IST [2013341] WARNING: replication slot name
"myslot1*" contains invalid character
2025-09-11 10:27:30.195 IST [2013341] HINT: Replication slot names
may only contain lower case letters, numbers, and the underscore
character.
2025-09-11 10:28:13.863 IST [2013341] LOG: invalid value for
parameter "synchronized_standby_slots": "myslot1*"
2025-09-11 10:28:13.863 IST [2013341] FATAL: configuration file
"/home/ubuntu/Project/inst/pg_11_9_tmp_4/bin/primary/postgresql.conf"
contains errors

I think we can use ReplicationSlotValidateName with elevel=WARNING here.
I have attached an updated patch with this change.

Thanks,
Shlok Kyal

Attachment Content-Type Size
v3-0001-Remove-the-validation-from-the-GUC-check-hook-and.patch application/octet-stream 2.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Chao Li 2025-09-11 05:30:20 Re: Fix missing EvalPlanQual recheck for TID scans
Previous Message Tom Lane 2025-09-11 05:22:43 Re: someone else to do the list of acknowledgments