Re: issue with synchronized_standby_slots

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;
}

--
With Regards,
Ashutosh Sharma.