Re: issue with synchronized_standby_slots

On Thu, Sep 11, 2025 at 11:07 AM Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com>
wrote:

>
> On Thu, Sep 11, 2025 at 11:00 AM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com>
> wrote:
> >
> > 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.
> >
>
> I would suggest getting rid of the "*ok*" flag, it’s probably not needed.
> I’d rather rewrite validate_sync_standby_slots() like this:
>
> static bool
> validate_sync_standby_slots(char *rawname, List **elemlist)
> {
> /* Verify syntax and parse string into a list of identifiers */
> if (!SplitIdentifierString(rawname, ',', elemlist))
> GUC_check_errdetail("List syntax is invalid.");
> else
> {
> foreach_ptr(char, name, *elemlist)
> {
> if (!ReplicationSlotValidateName(name, false, WARNING))
> return false;
> }
> }
>
> return true;
> }
>
>
Apart from this change, if you agree, then you may also add a test-case
that sets synchronized_standby_slots to some reserved name like
"pg_conflict_detection" to verify for the WARNING message.

--
With Regards,
Ashutosh Sharma.