On Thu, Sep 11, 2025 at 11:07 AM Ashutosh Sharma <[email protected]> wrote:
> > On Thu, Sep 11, 2025 at 11:00 AM Shlok Kyal <[email protected]> > wrote: > > > > On Thu, 11 Sept 2025 at 09:20, Amit Kapila <[email protected]> > wrote: > > > > > > On Thu, Sep 11, 2025 at 9:02 AM Amit Kapila <[email protected]> > wrote: > > > > > > > > On Wed, Sep 10, 2025 at 5:23 PM Alexander Kukushkin < > [email protected]> wrote: > > > > > > > > > > On Wed, 10 Sept 2025 at 13:34, Shlok Kyal < > [email protected]> 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.
