Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix GUC check_hook validation for synchronized_standby_slots.

  2. Make invalid primary_slot_name follow standard GUC error reporting.

  1. issue with synchronized_standby_slots

    Fabrice Chapuis <fabrice636861@gmail.com> — 2025-09-04T13:26:59Z

    Hi,
With PG 17.5 and using logical replication failover slots.
When trying to change the value of synchronized_standby_slots, node2 was
not running then the error * invalid value for parameter
"synchronized_standby_slots": "node1,node2"  *was generated.
The problem is that statement were affected by this and they can't execute.

STATEMENT:  select service_period,sp1_0.address_line_1 from tbl1  where
sp1_0.vn=$1 order by sp1_0.start_of_period
2025-08-24 13:14:29.417 CEST [848477]: [1-1] user=,db=,client=,application=
ERROR:  invalid value for parameter "synchronized_standby_slots":
"node1,node2"
2025-08-24 13:14:29.417 CEST [848477]: [2-1] user=,db=,client=,application=
DETAIL:  replication slot "s029054a" does not exist
2025-08-24 13:14:29.417 CEST [848477]: [3-1] user=,db=,client=,application=
CONTEXT:  while setting parameter "synchronized_standby_slots" to
"node1,node2"
2025-08-24 13:14:29.418 CEST [777453]: [48-1]
user=,db=,client=,application= LOG:  background worker "parallel worker"
(PID 848476) exited with exit code 1
2025-08-24 13:14:29.418 CEST [777453]: [49-1]
user=,db=,client=,application= LOG:  background worker "parallel worker"
(PID 848477) exited with exit code 1

Is this issue already observed

Thanks for your feedback

Fabrice

  2. RE: issue with synchronized_standby_slots

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2025-09-05T04:07:32Z

    On Thursday, September 4, 2025 9:27 PM Fabrice Chapuis <fabrice636861@gmail.com>  wrote:
> With PG 17.5 and using logical replication failover slots. When trying to
> change the value of synchronized_standby_slots, node2 was not running then the
> error  invalid value for parameter "synchronized_standby_slots": "node1,node2"
> was generated. The problem is that statement were affected by this and they
> can't execute.
>
> STATEMENT:  select service_period,sp1_0.address_line_1 from tbl1  where http://sp1_0.vn=$1 order by sp1_0.start_of_period
> 2025-08-24 13:14:29.417 CEST [848477]: [1-1] user=,db=,client=,application= ERROR:  invalid value for parameter "synchronized_standby_slots": "node1,node2"
> 2025-08-24 13:14:29.417 CEST [848477]: [2-1] user=,db=,client=,application= DETAIL:  replication slot "s029054a" does not exist
> 2025-08-24 13:14:29.417 CEST [848477]: [3-1] user=,db=,client=,application= CONTEXT:  while setting parameter "synchronized_standby_slots" to "node1,node2"
> 2025-08-24 13:14:29.418 CEST [777453]: [48-1] user=,db=,client=,application= LOG:  background worker "parallel worker" (PID 848476) exited with exit code 1
> 2025-08-24 13:14:29.418 CEST [777453]: [49-1] user=,db=,client=,application= LOG:  background worker "parallel worker" (PID 848477) exited with exit code 1
> 
> Is this issue already observed

Thank you for reporting this issue. It seems you've added a nonexistent slot to
synchronized_standby_slots before the server startup. The server does not verify
the existence of slots at startup due to the absence of slot shared information,
allowing the server to start successfully. However, when the parallel apply
worker starts, it re-verifies the GUC setting, resulting in the ERROR you saw.

I think this scenario is not necessarily a bug, as adding nonexistent slots to GUC is
disallowed. Such slots can block the logical failover slot's advancement,
increasing the risk of disk bloat due to WAL or dead rows, which is why we added
the ERROR. There are precedents for this kind of behavior, like
default_table_access_method and default_tablespace, which prevent queries if
invalid values are set before server startup.

To resolve the issue, you can remove the invalid slot from the GUC and add it
back after creating the physical slot.

I also thought about how to improve user experience for this, but it's not
feasible to verify slot existence at startup because replication has not been
restored to shared memory during GUC checks. Another option might be to simply
remove slot existence/type checks from GUC validation.

Best Regards,
Hou zj

  3. Re: issue with synchronized_standby_slots

    Fabrice Chapuis <fabrice636861@gmail.com> — 2025-09-07T08:15:22Z

    Thanks for your reply Zhijie,

I understand that the error  invalid value for parameter will be diplayed
in case of bad value for the GUC synchronized_standby_slots or if a standby
node configured is not up and running.
But the problem I noticed is that statements could not execute normally and
error code is returned to the applcation.
This append after an upgrade from PG 14 to PG 17.
I could try to reproduce the issue

Regards,

Fabrice

On Fri, Sep 5, 2025 at 6:07 AM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
wrote:

> On Thursday, September 4, 2025 9:27 PM Fabrice Chapuis <
> fabrice636861@gmail.com>  wrote:
> > With PG 17.5 and using logical replication failover slots. When trying to
> > change the value of synchronized_standby_slots, node2 was not running
> then the
> > error  invalid value for parameter "synchronized_standby_slots":
> "node1,node2"
> > was generated. The problem is that statement were affected by this and
> they
> > can't execute.
> >
> > STATEMENT:  select service_period,sp1_0.address_line_1 from tbl1  where
> http://sp1_0.vn=$1 order by sp1_0.start_of_period
> > 2025-08-24 13:14:29.417 CEST [848477]: [1-1]
> user=,db=,client=,application= ERROR:  invalid value for parameter
> "synchronized_standby_slots": "node1,node2"
> > 2025-08-24 13:14:29.417 CEST [848477]: [2-1]
> user=,db=,client=,application= DETAIL:  replication slot "s029054a" does
> not exist
> > 2025-08-24 13:14:29.417 CEST [848477]: [3-1]
> user=,db=,client=,application= CONTEXT:  while setting parameter
> "synchronized_standby_slots" to "node1,node2"
> > 2025-08-24 13:14:29.418 CEST [777453]: [48-1]
> user=,db=,client=,application= LOG:  background worker "parallel worker"
> (PID 848476) exited with exit code 1
> > 2025-08-24 13:14:29.418 CEST [777453]: [49-1]
> user=,db=,client=,application= LOG:  background worker "parallel worker"
> (PID 848477) exited with exit code 1
> >
> > Is this issue already observed
>
> Thank you for reporting this issue. It seems you've added a nonexistent
> slot to
> synchronized_standby_slots before the server startup. The server does not
> verify
> the existence of slots at startup due to the absence of slot shared
> information,
> allowing the server to start successfully. However, when the parallel apply
> worker starts, it re-verifies the GUC setting, resulting in the ERROR you
> saw.
>
> I think this scenario is not necessarily a bug, as adding nonexistent
> slots to GUC is
> disallowed. Such slots can block the logical failover slot's advancement,
> increasing the risk of disk bloat due to WAL or dead rows, which is why we
> added
> the ERROR. There are precedents for this kind of behavior, like
> default_table_access_method and default_tablespace, which prevent queries
> if
> invalid values are set before server startup.
>
> To resolve the issue, you can remove the invalid slot from the GUC and add
> it
> back after creating the physical slot.
>
> I also thought about how to improve user experience for this, but it's not
> feasible to verify slot existence at startup because replication has not
> been
> restored to shared memory during GUC checks. Another option might be to
> simply
> remove slot existence/type checks from GUC validation.
>
> Best Regards,
> Hou zj
>

  4. Re: issue with synchronized_standby_slots

    Alexander Kukushkin <cyberdemn@gmail.com> — 2025-09-08T09:25:46Z

    Hi,


On Sun, 7 Sept 2025 at 10:15, Fabrice Chapuis <fabrice636861@gmail.com>
wrote:

> Thanks for your reply Zhijie,
>
> I understand that the error  invalid value for parameter will be diplayed
> in case of bad value for the GUC synchronized_standby_slots or if a
> standby node configured is not up and running.
> But the problem I noticed is that statements could not execute normally
> and error code is returned to the applcation.
> This append after an upgrade from PG 14 to PG 17.
> I could try to reproduce the issue
>


> > STATEMENT:  select service_period,sp1_0.address_line_1 from tbl1  where
> http://sp1_0.vn=$1 order by sp1_0.start_of_period
>
>> > 2025-08-24 13:14:29.417 CEST [848477]: [1-1]
>> user=,db=,client=,application= ERROR:  invalid value for parameter
>> "synchronized_standby_slots": "node1,node2"
>> > 2025-08-24 13:14:29.417 CEST [848477]: [2-1]
>> user=,db=,client=,application= DETAIL:  replication slot "s029054a" does
>> not exist
>> > 2025-08-24 13:14:29.417 CEST [848477]: [3-1]
>> user=,db=,client=,application= CONTEXT:  while setting parameter
>> "synchronized_standby_slots" to "node1,node2"
>> > 2025-08-24 13:14:29.418 CEST [777453]: [48-1]
>> user=,db=,client=,application= LOG:  background worker "parallel worker"
>> (PID 848476) exited with exit code 1
>> > 2025-08-24 13:14:29.418 CEST [777453]: [49-1]
>> user=,db=,client=,application= LOG:  background worker "parallel worker"
>> (PID 848477) exited with exit code 1
>> >
>> > Is this issue already observed
>>
>
Recently we also hit this problem.

I think in a current state check_synchronized_standby_slots()
and validate_sync_standby_slots() functions are not very useful:
- When the hook is executed from postmaster it only checks
that synchronized_standby_slots contains a valid list, but doesn't check
that replication slots exists, because MyProc is NULL. It happens both, on
start and on reload.
- When executed from other backends set_config_with_handle() is called
with elevel = 0, and therefore elevel becomes DEBUG3, which results in no
useful error/warning messages.

There are a couple of places where check_synchronized_standby_slots()
failure is not ignored:
1. alter system set synchronized_standby_slots='invalid value';
2. Parallel workers, because RestoreGUCState()
calls set_config_option_ext()->set_config_with_handle() with elevel=ERROR.
As a result, parallel workers fail to start with the error.

With parallel workers it is actually even worse - we get the error even in
case of standby:
1. start standby with synchronized_standby_slots referring to non-existing
slots
2. SET parallel_setup_cost, parallel_tuple_cost,
and min_parallel_table_scan_size to 0
3. Run  select * from pg_namespace; and observe following error:
ERROR:  invalid value for parameter "synchronized_standby_slots": "a1,b1"
DETAIL:  replication slot "a1" does not exist
CONTEXT:  while setting parameter "synchronized_standby_slots" to "a1,b1"
parallel worker

We may argue a lot that invalid configuration must not be used, but the
thing is that the main problem being solved by synchronized_standby_slots
is delaying logical decoding at certains LSN until it was sent to enough
physical standby.
This feature must not affect normal queries, only logical replication.

Please find attached patch fixing a problem for parallel workers.

Regards,
--
Alexander Kukushkin

  5. Re: issue with synchronized_standby_slots

    Fujii Masao <masao.fujii@gmail.com> — 2025-09-08T15:51:08Z

    On Mon, Sep 8, 2025 at 6:26 PM Alexander Kukushkin <cyberdemn@gmail.com> wrote:
>
> Hi,
>
>
> On Sun, 7 Sept 2025 at 10:15, Fabrice Chapuis <fabrice636861@gmail.com> wrote:
>>
>> Thanks for your reply Zhijie,
>>
>> I understand that the error  invalid value for parameter will be diplayed in case of bad value for the GUC synchronized_standby_slots or if a standby node configured is not up and running.
>> But the problem I noticed is that statements could not execute normally and error code is returned to the applcation.
>> This append after an upgrade from PG 14 to PG 17.
>> I could try to reproduce the issue
>
>
>>
>> > STATEMENT:  select service_period,sp1_0.address_line_1 from tbl1  where http://sp1_0.vn=$1 order by sp1_0.start_of_period
>>>
>>> > 2025-08-24 13:14:29.417 CEST [848477]: [1-1] user=,db=,client=,application= ERROR:  invalid value for parameter "synchronized_standby_slots": "node1,node2"
>>> > 2025-08-24 13:14:29.417 CEST [848477]: [2-1] user=,db=,client=,application= DETAIL:  replication slot "s029054a" does not exist
>>> > 2025-08-24 13:14:29.417 CEST [848477]: [3-1] user=,db=,client=,application= CONTEXT:  while setting parameter "synchronized_standby_slots" to "node1,node2"
>>> > 2025-08-24 13:14:29.418 CEST [777453]: [48-1] user=,db=,client=,application= LOG:  background worker "parallel worker" (PID 848476) exited with exit code 1
>>> > 2025-08-24 13:14:29.418 CEST [777453]: [49-1] user=,db=,client=,application= LOG:  background worker "parallel worker" (PID 848477) exited with exit code 1
>>> >
>>> > Is this issue already observed
>
>
> Recently we also hit this problem.
>
> I think in a current state check_synchronized_standby_slots() and validate_sync_standby_slots() functions are not very useful:
> - When the hook is executed from postmaster it only checks that synchronized_standby_slots contains a valid list, but doesn't check that replication slots exists, because MyProc is NULL. It happens both, on start and on reload.

This looks quite problematic. If a non-existent slot is specified in
synchronized_standby_slots in postgresql.conf and the configuration file
is reloaded, no error is reported. This happens because the postmaster
cannot detect that the slot doesn't exist (since it has no MyProc).
As a result, synchronized_standby_slots in the postmaster is set to
that slot. New backends then inherit this setting from the postmaster,
while already running backends correctly detect that the slot doesn't
exist and fail to apply it.

This leads to an inconsistent state: the reload succeeds with no error,
but some backends apply the new setting while others do not.
That inconsistency seems like an issue.

To fix this, ISTM that the GUC check hook for synchronized_standby_slots
should be revised so it doesn't rely on MyProc or perform slot existence
checks there....

Regards,

-- 
Fujii Masao

  6. Re: issue with synchronized_standby_slots

    Amit Kapila <amit.kapila16@gmail.com> — 2025-09-09T05:29:47Z

    On Mon, Sep 8, 2025 at 2:56 PM Alexander Kukushkin <cyberdemn@gmail.com> wrote:
>
> Recently we also hit this problem.
>
> I think in a current state check_synchronized_standby_slots() and validate_sync_standby_slots() functions are not very useful:
> - When the hook is executed from postmaster it only checks that synchronized_standby_slots contains a valid list, but doesn't check that replication slots exists, because MyProc is NULL. It happens both, on start and on reload.
> - When executed from other backends set_config_with_handle() is called with elevel = 0, and therefore elevel becomes DEBUG3, which results in no useful error/warning messages.
>
> There are a couple of places where check_synchronized_standby_slots() failure is not ignored:
> 1. alter system set synchronized_standby_slots='invalid value';
> 2. Parallel workers, because RestoreGUCState() calls set_config_option_ext()->set_config_with_handle() with elevel=ERROR. As a result, parallel workers fail to start with the error.
>
> With parallel workers it is actually even worse - we get the error even in case of standby:
> 1. start standby with synchronized_standby_slots referring to non-existing slots
> 2. SET parallel_setup_cost, parallel_tuple_cost, and min_parallel_table_scan_size to 0
> 3. Run  select * from pg_namespace; and observe following error:
> ERROR:  invalid value for parameter "synchronized_standby_slots": "a1,b1"
> DETAIL:  replication slot "a1" does not exist
> CONTEXT:  while setting parameter "synchronized_standby_slots" to "a1,b1"
> parallel worker
>

I see the same behaviour for default_table_access_method and
default_tablespace. For example, see failure cases:
postgres=# Alter system set default_table_access_method='missing';
ERROR:  invalid value for parameter "default_table_access_method": "missing"
DETAIL:  Table access method "missing" does not exist.

postgres=# SET parallel_setup_cost=0;
SET
postgres=# SET parallel_tuple_cost=0;
SET
postgres=# Set min_parallel_table_scan_size to 0;
SET
postgres=# select * from pg_namespace;
ERROR:  invalid value for parameter "default_table_access_method": "missing"
DETAIL:  Table access method "missing" does not exist.
CONTEXT:  while setting parameter "default_table_access_method" to "missing"
parallel worker

OTOH, there is no ERROR on reload or restart.

It is fair to argue that invalid GUC values should be ignored in
certain cases like parallel query but we should have the same solution
for other similar parameters as well.

As for the synchronized_standby_slots, we can follow the behavior
similar to check_synchronous_standby_names and just give parsing
ERRORs. Any non-existent slot related errors can be given when that
parameter is later used.

-- 
With Regards,
Amit Kapila.

  7. RE: issue with synchronized_standby_slots

    Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> — 2025-09-09T05:38:54Z

    On Tuesday, September 9, 2025 1:30 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 
> On Mon, Sep 8, 2025 at 2:56 PM Alexander Kukushkin
> <cyberdemn@gmail.com> wrote:
> >
> > Recently we also hit this problem.
> >
> > I think in a current state check_synchronized_standby_slots() and
> validate_sync_standby_slots() functions are not very useful:
> > - When the hook is executed from postmaster it only checks that
> synchronized_standby_slots contains a valid list, but doesn't check that
> replication slots exists, because MyProc is NULL. It happens both, on start and
> on reload.
> > - When executed from other backends set_config_with_handle() is called
> with elevel = 0, and therefore elevel becomes DEBUG3, which results in no
> useful error/warning messages.
> >
> > There are a couple of places where check_synchronized_standby_slots()
> failure is not ignored:
> > 1. alter system set synchronized_standby_slots='invalid value'; 2.
> > Parallel workers, because RestoreGUCState() calls
> set_config_option_ext()->set_config_with_handle() with elevel=ERROR. As a
> result, parallel workers fail to start with the error.
> >
> > With parallel workers it is actually even worse - we get the error even in case
> of standby:
> > 1. start standby with synchronized_standby_slots referring to
> > non-existing slots 2. SET parallel_setup_cost, parallel_tuple_cost,
> > and min_parallel_table_scan_size to 0 3. Run  select * from pg_namespace;
> and observe following error:
> > ERROR:  invalid value for parameter "synchronized_standby_slots": "a1,b1"
> > DETAIL:  replication slot "a1" does not exist
> > CONTEXT:  while setting parameter "synchronized_standby_slots" to
> "a1,b1"
> > parallel worker
> >
> 
> I see the same behaviour for default_table_access_method and
> default_tablespace. For example, see failure cases:
> postgres=# Alter system set default_table_access_method='missing';
> ERROR:  invalid value for parameter "default_table_access_method":
> "missing"
> DETAIL:  Table access method "missing" does not exist.
> 
> postgres=# SET parallel_setup_cost=0;
> SET
> postgres=# SET parallel_tuple_cost=0;
> SET
> postgres=# Set min_parallel_table_scan_size to 0; SET postgres=# select *
> from pg_namespace;
> ERROR:  invalid value for parameter "default_table_access_method":
> "missing"
> DETAIL:  Table access method "missing" does not exist.
> CONTEXT:  while setting parameter "default_table_access_method" to
> "missing"
> parallel worker
> 
> OTOH, there is no ERROR on reload or restart.
> 
> It is fair to argue that invalid GUC values should be ignored in certain cases like
> parallel query but we should have the same solution for other similar
> parameters as well.
> 
> As for the synchronized_standby_slots, we can follow the behavior similar to
> check_synchronous_standby_names and just give parsing ERRORs. Any
> non-existent slot related errors can be given when that parameter is later used.

I agree. For synchronized_standby_slots, I think it is acceptable to report only
parsing errors, because slots could be dropped even after validating the slot
existence during GUC loading. Additionally, we would report WARNINGs for
non-existent slots during the wait function anyway (e.g., in
StandbySlotsHaveCaughtup()).

Best Regards,
Hou zj

  8. Re: issue with synchronized_standby_slots

    Fujii Masao <masao.fujii@gmail.com> — 2025-09-09T08:07:18Z

    On Tue, Sep 9, 2025 at 2:39 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
> I agree. For synchronized_standby_slots, I think it is acceptable to report only
> parsing errors, because slots could be dropped even after validating the slot
> existence during GUC loading. Additionally, we would report WARNINGs for
> non-existent slots during the wait function anyway (e.g., in
> StandbySlotsHaveCaughtup()).

+1, to also address the issue I reported upthread.

As I understand it, only the walsender and the slot sync worker actually
use synchronized_standby_slots. But in the current code, all processes check
for slot existence via the GUC check hook, which wastes cycles. The proposed
change would eliminate that overhead.

Regards,

-- 
Fujii Masao

  9. Re: issue with synchronized_standby_slots

    Rahila Syed <rahilasyed90@gmail.com> — 2025-09-10T10:02:58Z

    Hi,


 As for the synchronized_standby_slots, we can follow the behavior
> similar to check_synchronous_standby_names and just give parsing
> ERRORs. Any non-existent slot related errors can be given when that
> parameter is later used.
>
> --
>

Please find attached a patch that implements this. I will work on adding a
test for it.

Thank you,
Rahila Syed

  10. Re: issue with synchronized_standby_slots

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2025-09-10T11:22:09Z

    On Wed, Sep 10, 2025 at 3:33 PM Rahila Syed <rahilasyed90@gmail.com> wrote:
>
> Hi,
>
>
>>  As for the synchronized_standby_slots, we can follow the behavior
>> similar to check_synchronous_standby_names and just give parsing
>> ERRORs. Any non-existent slot related errors can be given when that
>> parameter is later used.
>>
>> --
>
>
> Please find attached a patch that implements this. I will work on adding a test for it.
>

I suggest removing validate_sync_standby_slots() entirely since there
isn’t much left in it.

The below logic currently in validate_sync_standby_slots() to parse
the list of slots can be directly moved into
check_synchronized_standby_slots().

/* Verify syntax and parse string into a list of identifiers */
ok = SplitIdentifierString(rawname, ',', elemlist);

if (!ok)
GUC_check_errdetail("List syntax is invalid.");

--
With Regards,
Ashutosh Sharma.

  11. Re: issue with synchronized_standby_slots

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-09-10T11:34:18Z

    On Wed, 10 Sept 2025 at 15:33, Rahila Syed <rahilasyed90@gmail.com> wrote:
>
> Hi,
>
>
>>  As for the synchronized_standby_slots, we can follow the behavior
>> similar to check_synchronous_standby_names and just give parsing
>> ERRORs. Any non-existent slot related errors can be given when that
>> parameter is later used.
>>
>> --
>
>
> Please find attached a patch that implements this. I will work on adding a test for it.
>
Hi Rahila,

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 think we should also update the comment message in function
StandbySlotsHaveCaughtup
       /*
         * If a slot name provided in synchronized_standby_slots does not
         * exist, report a message and exit the loop.
         *
         * Though validate_sync_standby_slots (the GUC check_hook) tries to
         * avoid this, it can nonetheless happen because the user can specify
         * a nonexistent slot name before server startup. That function cannot
         * validate such a slot during startup, as ReplicationSlotCtl is not
         * initialized by then.  Also, the user might have dropped one slot.
         */
I made the changes in the above for the same and attached the updated patch.

[1]: https://www.postgresql.org/message-id/CAA4eK1Jprqfs_URBmZ5%3DOOL98D05rPiGup1sscWgcCGcWU%3D9iA%40mail.gmail.com

Thanks,
Shlok Kyal

  12. Re: issue with synchronized_standby_slots

    Alexander Kukushkin <cyberdemn@gmail.com> — 2025-09-10T11:53:41Z

    Hi,

On Wed, 10 Sept 2025 at 13:34, Shlok Kyal <shlok.kyal.oss@gmail.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.

-- 
Regards,
--
Alexander Kukushkin

  13. Re: issue with synchronized_standby_slots

    Amit Kapila <amit.kapila16@gmail.com> — 2025-09-11T03:32:39Z

    On Wed, Sep 10, 2025 at 5:23 PM Alexander Kukushkin <cyberdemn@gmail.com> wrote:
>
> On Wed, 10 Sept 2025 at 13:34, Shlok Kyal <shlok.kyal.oss@gmail.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?

BTW, we should also try to conclude on my yesterday's point as to why
it is okay to have the same behavior for default_tablespace and
default_table_access_method and not for this parameter? I am asking
because if we change the current behavior, tomorrow, we can get
complaints that one expects the old behaviour as that was similar to
other GUCs like default_tablespace and default_table_access_method.

-- 
With Regards,
Amit Kapila.

  14. Re: issue with synchronized_standby_slots

    Amit Kapila <amit.kapila16@gmail.com> — 2025-09-11T03:50:30Z

    On Thu, Sep 11, 2025 at 9:02 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Sep 10, 2025 at 5:23 PM Alexander Kukushkin <cyberdemn@gmail.com> wrote:
> >
> > On Wed, 10 Sept 2025 at 13:34, Shlok Kyal <shlok.kyal.oss@gmail.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.

-- 
With Regards,
Amit Kapila.

  15. Re: issue with synchronized_standby_slots

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-09-11T05:30:02Z

    On Thu, 11 Sept 2025 at 09:20, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Sep 11, 2025 at 9:02 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Sep 10, 2025 at 5:23 PM Alexander Kukushkin <cyberdemn@gmail.com> wrote:
> > >
> > > On Wed, 10 Sept 2025 at 13:34, Shlok Kyal <shlok.kyal.oss@gmail.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

  16. Re: issue with synchronized_standby_slots

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2025-09-11T05:37:23Z

    On Thu, Sep 11, 2025 at 11:00 AM Shlok Kyal <shlok.kyal.oss@gmail.com>
wrote:
>
> On Thu, 11 Sept 2025 at 09:20, Amit Kapila <amit.kapila16@gmail.com>
wrote:
> >
> > On Thu, Sep 11, 2025 at 9:02 AM Amit Kapila <amit.kapila16@gmail.com>
wrote:
> > >
> > > On Wed, Sep 10, 2025 at 5:23 PM Alexander Kukushkin <
cyberdemn@gmail.com> wrote:
> > > >
> > > > On Wed, 10 Sept 2025 at 13:34, Shlok Kyal <shlok.kyal.oss@gmail.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.

  17. Re: issue with synchronized_standby_slots

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2025-09-11T05:43:11Z

    On Thu, Sep 11, 2025 at 11:07 AM Ashutosh Sharma <ashu.coek88@gmail.com>
wrote:

>
> On Thu, Sep 11, 2025 at 11:00 AM Shlok Kyal <shlok.kyal.oss@gmail.com>
> wrote:
> >
> > On Thu, 11 Sept 2025 at 09:20, Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> > >
> > > On Thu, Sep 11, 2025 at 9:02 AM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> > > >
> > > > On Wed, Sep 10, 2025 at 5:23 PM Alexander Kukushkin <
> cyberdemn@gmail.com> wrote:
> > > > >
> > > > > On Wed, 10 Sept 2025 at 13:34, Shlok Kyal <
> shlok.kyal.oss@gmail.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.

  18. Re: issue with synchronized_standby_slots

    Rahila Syed <rahilasyed90@gmail.com> — 2025-09-11T06:30:20Z

    Hi,



>
> BTW, we should also try to conclude on my yesterday's point as to why
> it is okay to have the same behavior for default_tablespace and
> default_table_access_method and not for this parameter? I am asking
> because if we change the current behavior, tomorrow, we can get
> complaints that one expects the old behaviour as that was similar to
> other GUCs like default_tablespace and default_table_access_method.
>
>
Fair point. I haven't examined the validation of GUCs in parallel workers
closely,
but one argument for preventing parallel workers from failing due to an
incorrect
value of synchronized_standby_slots is that a select query works in this
situation
without parallel workers.

Whereas, for incorrect values of default_tablespace and
default_table_access_method,
most commands would fail regardless of whether parallel workers are enabled.

PFA a test for the original bug report on this thread.  This applies on the
v3 version of the patch
that was shared.

Thank you,
Rahila Syed

  19. Re: issue with synchronized_standby_slots

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-09-12T09:04:26Z

    On Thu, 11 Sept 2025 at 12:00, Rahila Syed <rahilasyed90@gmail.com> wrote:
>
> Hi,
>
>
>>
>>
>> BTW, we should also try to conclude on my yesterday's point as to why
>> it is okay to have the same behavior for default_tablespace and
>> default_table_access_method and not for this parameter? I am asking
>> because if we change the current behavior, tomorrow, we can get
>> complaints that one expects the old behaviour as that was similar to
>> other GUCs like default_tablespace and default_table_access_method.
>>
>
> Fair point. I haven't examined the validation of GUCs in parallel workers closely,
> but one argument for preventing parallel workers from failing due to an incorrect
> value of synchronized_standby_slots is that a select query works in this situation
> without parallel workers.
>
> Whereas, for incorrect values of default_tablespace and default_table_access_method,
> most commands would fail regardless of whether parallel workers are enabled.
>
> PFA a test for the original bug report on this thread.  This applies on the v3 version of the patch
> that was shared.
>
Thanks for sharing the patch. I checked the test and it looks good to
me. But I am not sure if we should have a new file for the test. I
have added the test in the '040_standby_failover_slots_sync.pl' file
along with some other tests.
Also I have addressed the comments by Ashutosh in [1][2].

I have attached the updated v4 patch

[1]: https://www.postgresql.org/message-id/CAE9k0P%3Dx3J3nmSmYKmTkiFXTDKLxJkXFO4%2BVHJyNu01Od6CZfg%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CAE9k0P%3DOFMFCRy9aDGWZ3bt91tbB1WnzsAbzXN72iWBaGVuMrw%40mail.gmail.com

Thanks,
Shlok Kyal

  20. Re: issue with synchronized_standby_slots

    Amit Kapila <amit.kapila16@gmail.com> — 2025-09-23T04:25:16Z

    On Fri, Sep 12, 2025 at 2:34 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> I have attached the updated v4 patch
>

+# Cannot be set synchronized_standby_slots to a reserved slot name
+($result, $stdout, $stderr) = $primary->psql('postgres',
+ "ALTER SYSTEM SET synchronized_standby_slots='pg_conflict_detection'");
+ok( $stderr =~
+   m/WARNING:  replication slot name "pg_conflict_detection" is reserved/,
+ "Cannot use a reserverd slot name");
+
+# Cannot be set synchronized_standby_slots to slot name with invalid characters
+($result, $stdout, $stderr) = $primary->psql('postgres',
+ "ALTER SYSTEM SET synchronized_standby_slots='invalid*'");
+ok( $stderr =~
+   m/WARNING:  replication slot name "invalid\*" contains invalid character/,
+ "Cannot use a invalid slot name");

These tests can be present in some sql file. I think you have kept
these in the .pl file to keep it along with other tests but I think
these are better suited for some .sql file.

-- 
With Regards,
Amit Kapila.

  21. Re: issue with synchronized_standby_slots

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-09-23T07:29:42Z

    On Tue, 23 Sept 2025 at 09:55, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Sep 12, 2025 at 2:34 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >
> > I have attached the updated v4 patch
> >
>
> +# Cannot be set synchronized_standby_slots to a reserved slot name
> +($result, $stdout, $stderr) = $primary->psql('postgres',
> + "ALTER SYSTEM SET synchronized_standby_slots='pg_conflict_detection'");
> +ok( $stderr =~
> +   m/WARNING:  replication slot name "pg_conflict_detection" is reserved/,
> + "Cannot use a reserverd slot name");
> +
> +# Cannot be set synchronized_standby_slots to slot name with invalid characters
> +($result, $stdout, $stderr) = $primary->psql('postgres',
> + "ALTER SYSTEM SET synchronized_standby_slots='invalid*'");
> +ok( $stderr =~
> +   m/WARNING:  replication slot name "invalid\*" contains invalid character/,
> + "Cannot use a invalid slot name");
>
> These tests can be present in some sql file. I think you have kept
> these in the .pl file to keep it along with other tests but I think
> these are better suited for some .sql file.
>
Thanks for reviewing the patch.
I have moved the tests to the guc.sql file. I have attached the updated patch.

Thanks,
Shlok Kyal

  22. Re: issue with synchronized_standby_slots

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2025-09-24T06:44:13Z

    Hi Amit,

On Tue, Sep 23, 2025 at 1:00 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> On Tue, 23 Sept 2025 at 09:55, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Sep 12, 2025 at 2:34 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> > >
> > > I have attached the updated v4 patch
> > >
> >
> > +# Cannot be set synchronized_standby_slots to a reserved slot name
> > +($result, $stdout, $stderr) = $primary->psql('postgres',
> > + "ALTER SYSTEM SET synchronized_standby_slots='pg_conflict_detection'");
> > +ok( $stderr =~
> > +   m/WARNING:  replication slot name "pg_conflict_detection" is reserved/,
> > + "Cannot use a reserverd slot name");
> > +
> > +# Cannot be set synchronized_standby_slots to slot name with invalid characters
> > +($result, $stdout, $stderr) = $primary->psql('postgres',
> > + "ALTER SYSTEM SET synchronized_standby_slots='invalid*'");
> > +ok( $stderr =~
> > +   m/WARNING:  replication slot name "invalid\*" contains invalid character/,
> > + "Cannot use a invalid slot name");
> >
> > These tests can be present in some sql file. I think you have kept
> > these in the .pl file to keep it along with other tests but I think
> > these are better suited for some .sql file.
> >
> Thanks for reviewing the patch.
> I have moved the tests to the guc.sql file. I have attached the updated patch.
>

Are we planning to wait for [1] to go in first, since this also
depends on ReplicationSlotValidateName?

[1] - https://www.postgresql.org/message-id/flat/CAHGQGwFud-cvthCTfusBfKHBS6Jj6kdAPTdLWKvP2qjUX6L_wA%40mail.gmail.com

--
With Regards,
Ashutosh Sharma.

  23. Re: issue with synchronized_standby_slots

    Amit Kapila <amit.kapila16@gmail.com> — 2025-09-24T10:15:36Z

    On Wed, Sep 24, 2025 at 12:14 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> Hi Amit,
>
> On Tue, Sep 23, 2025 at 1:00 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >
> > On Tue, 23 Sept 2025 at 09:55, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Fri, Sep 12, 2025 at 2:34 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> > > >
> > > > I have attached the updated v4 patch
> > > >
> > >
> > > +# Cannot be set synchronized_standby_slots to a reserved slot name
> > > +($result, $stdout, $stderr) = $primary->psql('postgres',
> > > + "ALTER SYSTEM SET synchronized_standby_slots='pg_conflict_detection'");
> > > +ok( $stderr =~
> > > +   m/WARNING:  replication slot name "pg_conflict_detection" is reserved/,
> > > + "Cannot use a reserverd slot name");
> > > +
> > > +# Cannot be set synchronized_standby_slots to slot name with invalid characters
> > > +($result, $stdout, $stderr) = $primary->psql('postgres',
> > > + "ALTER SYSTEM SET synchronized_standby_slots='invalid*'");
> > > +ok( $stderr =~
> > > +   m/WARNING:  replication slot name "invalid\*" contains invalid character/,
> > > + "Cannot use a invalid slot name");
> > >
> > > These tests can be present in some sql file. I think you have kept
> > > these in the .pl file to keep it along with other tests but I think
> > > these are better suited for some .sql file.
> > >
> > Thanks for reviewing the patch.
> > I have moved the tests to the guc.sql file. I have attached the updated patch.
> >
>
> Are we planning to wait for [1] to go in first, since this also
> depends on ReplicationSlotValidateName?
>

I think we can go either way. It somewhat depends on whether we want
to backpatch this change. The argument for having this as a HEAD-only
patch is that, we have a similar behavior for other variables like
default_table_access_method and default_tablespace in HEAD and
back-branches. We want to change to a better behavior for this GUC, so
better to keep this as a HEAD-only patch. Do you or others have
thoughts on this matter?

If we decide to do this as a HEAD-only patch, then I think we can push
this without waiting for the other patch to commit as we have ample
time to get that done and we already have a solution as well for it.
OTOH, if we want to backpatch this then I would prefer to wait for the
other patch to be committed first.

-- 
With Regards,
Amit Kapila.

  24. Re: issue with synchronized_standby_slots

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2025-09-26T07:00:19Z

    Hi,

On Wed, Sep 24, 2025 at 3:45 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Sep 24, 2025 at 12:14 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> >
> > Hi Amit,
> >
> > On Tue, Sep 23, 2025 at 1:00 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> > >
> > > On Tue, 23 Sept 2025 at 09:55, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > On Fri, Sep 12, 2025 at 2:34 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> > > > >
> > > > > I have attached the updated v4 patch
> > > > >
> > > >
> > > > +# Cannot be set synchronized_standby_slots to a reserved slot name
> > > > +($result, $stdout, $stderr) = $primary->psql('postgres',
> > > > + "ALTER SYSTEM SET synchronized_standby_slots='pg_conflict_detection'");
> > > > +ok( $stderr =~
> > > > +   m/WARNING:  replication slot name "pg_conflict_detection" is reserved/,
> > > > + "Cannot use a reserverd slot name");
> > > > +
> > > > +# Cannot be set synchronized_standby_slots to slot name with invalid characters
> > > > +($result, $stdout, $stderr) = $primary->psql('postgres',
> > > > + "ALTER SYSTEM SET synchronized_standby_slots='invalid*'");
> > > > +ok( $stderr =~
> > > > +   m/WARNING:  replication slot name "invalid\*" contains invalid character/,
> > > > + "Cannot use a invalid slot name");
> > > >
> > > > These tests can be present in some sql file. I think you have kept
> > > > these in the .pl file to keep it along with other tests but I think
> > > > these are better suited for some .sql file.
> > > >
> > > Thanks for reviewing the patch.
> > > I have moved the tests to the guc.sql file. I have attached the updated patch.
> > >
> >
> > Are we planning to wait for [1] to go in first, since this also
> > depends on ReplicationSlotValidateName?
> >
>
> I think we can go either way. It somewhat depends on whether we want
> to backpatch this change. The argument for having this as a HEAD-only
> patch is that, we have a similar behavior for other variables like
> default_table_access_method and default_tablespace in HEAD and
> back-branches. We want to change to a better behavior for this GUC, so
> better to keep this as a HEAD-only patch. Do you or others have
> thoughts on this matter?
>
> If we decide to do this as a HEAD-only patch, then I think we can push
> this without waiting for the other patch to commit as we have ample
> time to get that done and we already have a solution as well for it.
> OTOH, if we want to backpatch this then I would prefer to wait for the
> other patch to be committed first.
>

Although it's a behaviour change at GUC level, to me it doesn't look
like something that would break the user applications, so I'm inclined
toward backpatching it, but I'd definitely want to hear if others see
potential compatibility risks that I might be missing.

--
With Regards,
Ashutosh Sharma.

  25. Re: issue with synchronized_standby_slots

    Fabrice Chapuis <fabrice636861@gmail.com> — 2025-10-08T15:35:06Z

    As Ashutosh suggests I will go more for the backpatching approach because
the synchronized_standby_slots parameter impacts the last 2 major versions
and this problem is critical on production environments.

Best regards
Fabrice

On Fri, Sep 26, 2025 at 9:00 AM Ashutosh Sharma <ashu.coek88@gmail.com>
wrote:

> Hi,
>
> On Wed, Sep 24, 2025 at 3:45 PM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> >
> > On Wed, Sep 24, 2025 at 12:14 PM Ashutosh Sharma <ashu.coek88@gmail.com>
> wrote:
> > >
> > > Hi Amit,
> > >
> > > On Tue, Sep 23, 2025 at 1:00 PM Shlok Kyal <shlok.kyal.oss@gmail.com>
> wrote:
> > > >
> > > > On Tue, 23 Sept 2025 at 09:55, Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> > > > >
> > > > > On Fri, Sep 12, 2025 at 2:34 PM Shlok Kyal <
> shlok.kyal.oss@gmail.com> wrote:
> > > > > >
> > > > > > I have attached the updated v4 patch
> > > > > >
> > > > >
> > > > > +# Cannot be set synchronized_standby_slots to a reserved slot name
> > > > > +($result, $stdout, $stderr) = $primary->psql('postgres',
> > > > > + "ALTER SYSTEM SET
> synchronized_standby_slots='pg_conflict_detection'");
> > > > > +ok( $stderr =~
> > > > > +   m/WARNING:  replication slot name "pg_conflict_detection" is
> reserved/,
> > > > > + "Cannot use a reserverd slot name");
> > > > > +
> > > > > +# Cannot be set synchronized_standby_slots to slot name with
> invalid characters
> > > > > +($result, $stdout, $stderr) = $primary->psql('postgres',
> > > > > + "ALTER SYSTEM SET synchronized_standby_slots='invalid*'");
> > > > > +ok( $stderr =~
> > > > > +   m/WARNING:  replication slot name "invalid\*" contains invalid
> character/,
> > > > > + "Cannot use a invalid slot name");
> > > > >
> > > > > These tests can be present in some sql file. I think you have kept
> > > > > these in the .pl file to keep it along with other tests but I think
> > > > > these are better suited for some .sql file.
> > > > >
> > > > Thanks for reviewing the patch.
> > > > I have moved the tests to the guc.sql file. I have attached the
> updated patch.
> > > >
> > >
> > > Are we planning to wait for [1] to go in first, since this also
> > > depends on ReplicationSlotValidateName?
> > >
> >
> > I think we can go either way. It somewhat depends on whether we want
> > to backpatch this change. The argument for having this as a HEAD-only
> > patch is that, we have a similar behavior for other variables like
> > default_table_access_method and default_tablespace in HEAD and
> > back-branches. We want to change to a better behavior for this GUC, so
> > better to keep this as a HEAD-only patch. Do you or others have
> > thoughts on this matter?
> >
> > If we decide to do this as a HEAD-only patch, then I think we can push
> > this without waiting for the other patch to commit as we have ample
> > time to get that done and we already have a solution as well for it.
> > OTOH, if we want to backpatch this then I would prefer to wait for the
> > other patch to be committed first.
> >
>
> Although it's a behaviour change at GUC level, to me it doesn't look
> like something that would break the user applications, so I'm inclined
> toward backpatching it, but I'd definitely want to hear if others see
> potential compatibility risks that I might be missing.
>
> --
> With Regards,
> Ashutosh Sharma.
>

  26. Re: issue with synchronized_standby_slots

    Amit Kapila <amit.kapila16@gmail.com> — 2025-10-09T05:23:54Z

    On Wed, Oct 8, 2025 at 9:05 PM Fabrice Chapuis <fabrice636861@gmail.com> wrote:
>
> As Ashutosh suggests I will go more for the backpatching approach because the synchronized_standby_slots parameter impacts the last 2 major versions and this problem is critical on production environments.
>

Fair enough. Let's wait for the related issue being discussed in email
[1] to be fixed.

[1] - https://www.postgresql.org/message-id/CAHGQGwFud-cvthCTfusBfKHBS6Jj6kdAPTdLWKvP2qjUX6L_wA%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

  27. Re: issue with synchronized_standby_slots

    Amit Kapila <amit.kapila16@gmail.com> — 2025-10-23T04:06:07Z

    On Thu, Oct 9, 2025 at 10:53 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Oct 8, 2025 at 9:05 PM Fabrice Chapuis <fabrice636861@gmail.com> wrote:
> >
> > As Ashutosh suggests I will go more for the backpatching approach because the synchronized_standby_slots parameter impacts the last 2 major versions and this problem is critical on production environments.
> >
>
> Fair enough. Let's wait for the related issue being discussed in email
> [1] to be fixed.
>

As the other patch is committed
(f33e60a53a9ca89b5078df49416acae20affe1f5), can you update and prepare
backbranch patches for this fix as well?

-- 
With Regards,
Amit Kapila.

  28. Re: issue with synchronized_standby_slots

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-10-23T07:49:24Z

    On Thu, 23 Oct 2025 at 09:36, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Oct 9, 2025 at 10:53 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Oct 8, 2025 at 9:05 PM Fabrice Chapuis <fabrice636861@gmail.com> wrote:
> > >
> > > As Ashutosh suggests I will go more for the backpatching approach because the synchronized_standby_slots parameter impacts the last 2 major versions and this problem is critical on production environments.
> > >
> >
> > Fair enough. Let's wait for the related issue being discussed in email
> > [1] to be fixed.
> >
>
> As the other patch is committed
> (f33e60a53a9ca89b5078df49416acae20affe1f5), can you update and prepare
> backbranch patches for this fix as well?
>
Hi Amit,

Please find the updated patch.

v6-0001 : It applies on HEAD and REL_18_STABLE branches
v6_REL_17-0001 : It applies on REL_17_STABLE branch.

Since this GUC was introduced in PG_17, we do not need to back-patch
to PG_16 or prior.

Thanks,
Shlok Kyal

  29. Re: issue with synchronized_standby_slots

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-10-23T08:14:50Z

    On Thu, 23 Oct 2025 at 13:19, Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> On Thu, 23 Oct 2025 at 09:36, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Thu, Oct 9, 2025 at 10:53 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Wed, Oct 8, 2025 at 9:05 PM Fabrice Chapuis <fabrice636861@gmail.com> wrote:
> > > >
> > > > As Ashutosh suggests I will go more for the backpatching approach because the synchronized_standby_slots parameter impacts the last 2 major versions and this problem is critical on production environments.
> > > >
> > >
> > > Fair enough. Let's wait for the related issue being discussed in email
> > > [1] to be fixed.
> > >
> >
> > As the other patch is committed
> > (f33e60a53a9ca89b5078df49416acae20affe1f5), can you update and prepare
> > backbranch patches for this fix as well?
> >
> Hi Amit,
>
> Please find the updated patch.
>
> v6-0001 : It applies on HEAD and REL_18_STABLE branches
> v6_REL_17-0001 : It applies on REL_17_STABLE branch.
>
> Since this GUC was introduced in PG_17, we do not need to back-patch
> to PG_16 or prior.
>
The CFbot was failing due to the merge conflict. It happened because
CFbot tried to apply v6_REL_17 on top of v6-0001 patch. Added
v6_REL_17 as a .txt file so this merge conflict do not happen.

Thanks,
Shlok Kyal

  30. RE: issue with synchronized_standby_slots

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-10-23T08:15:01Z

    Dear Shlok,

Thanks for updating the patch! Few comments.

1.
You must separate patch for master and PG18, because ReplicationSlotValidateNameInternal()
does not accept `bool allow_reserved_name` in the version.

2.
Also, test for PG18 should not have the case which rejects the reserved name.

3.
```
-- Parallel worker does not throw error during startup.
SET min_parallel_table_scan_size TO 0;
SET parallel_setup_cost TO 0;
SET parallel_tuple_cost TO 0;
```

According to contrib/pg_stat_statements/sql/parallel.sql, max_parallel_workers_per_gather
should be also set. There is a possiblity that `make installcheck` is used and
it has max_parallel_workers_per_gather=0.

4.
```
	foreach_ptr(char, name, *elemlist)
```

You can add a comment atop here like:
Iterate the list to validate each slot name.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

  31. Re: issue with synchronized_standby_slots

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-10-23T09:28:11Z

    On Thu, 23 Oct 2025 at 13:45, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shlok,
>
> Thanks for updating the patch! Few comments.
>
> 1.
> You must separate patch for master and PG18, because ReplicationSlotValidateNameInternal()
> does not accept `bool allow_reserved_name` in the version.
>
> 2.
> Also, test for PG18 should not have the case which rejects the reserved name.
>
> 3.
> ```
> -- Parallel worker does not throw error during startup.
> SET min_parallel_table_scan_size TO 0;
> SET parallel_setup_cost TO 0;
> SET parallel_tuple_cost TO 0;
> ```
>
> According to contrib/pg_stat_statements/sql/parallel.sql, max_parallel_workers_per_gather
> should be also set. There is a possiblity that `make installcheck` is used and
> it has max_parallel_workers_per_gather=0.
>
> 4.
> ```
>         foreach_ptr(char, name, *elemlist)
> ```
>
> You can add a comment atop here like:
> Iterate the list to validate each slot name.
>
Hi Kuroda-san,
Thanks for reviewing the patches.
I have addressed the comments and attached the updated version.

Thanks,
Shlok Kyal

  32. Re: issue with synchronized_standby_slots

    Amit Kapila <amit.kapila16@gmail.com> — 2025-10-23T11:28:18Z

    On Thu, Oct 23, 2025 at 2:58 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> On Thu, 23 Oct 2025 at 13:45, Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
> >
> > 2.
> > Also, test for PG18 should not have the case which rejects the reserved name.
> >

Why to have that even for HEAD and PG18?

> > 3.
> > ```
> > -- Parallel worker does not throw error during startup.
> > SET min_parallel_table_scan_size TO 0;
> > SET parallel_setup_cost TO 0;
> > SET parallel_tuple_cost TO 0;
> > ```
> >
> > According to contrib/pg_stat_statements/sql/parallel.sql, max_parallel_workers_per_gather
> > should be also set. There is a possiblity that `make installcheck` is used and
> > it has max_parallel_workers_per_gather=0.
> >

+-- Parallel worker does not throw error during startup.
+SET min_parallel_table_scan_size TO 0;
+SET max_parallel_workers_per_gather TO 2;
+SET parallel_setup_cost TO 0;
+SET parallel_tuple_cost TO 0;
+CREATE TABLE t1(a int);
+INSERT INTO t1 VALUES(1), (2), (3), (4);
+SELECT count(*) FROM t1;

Isn't it better to reset these parameters after the test?

-- 
With Regards,
Amit Kapila.

  33. Re: issue with synchronized_standby_slots

    Fujii Masao <masao.fujii@gmail.com> — 2025-10-23T15:38:32Z

    On Thu, Oct 23, 2025 at 8:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> +-- Parallel worker does not throw error during startup.
> +SET min_parallel_table_scan_size TO 0;
> +SET max_parallel_workers_per_gather TO 2;
> +SET parallel_setup_cost TO 0;
> +SET parallel_tuple_cost TO 0;
> +CREATE TABLE t1(a int);
> +INSERT INTO t1 VALUES(1), (2), (3), (4);
> +SELECT count(*) FROM t1;
>
> Isn't it better to reset these parameters after the test?

I think the intention of this test case is to verify that the issue seen
in HEAD no longer occurs with the patch applied. So in HEAD this test
procedure needs to be able to reproduce the problem with
synchronized_standby_slots and parallel workers, but does it actually do?
I'm afraid additional steps are needed.

Also, I wonder if it's really worth doing this test after the fix,
since it seems a special case.


+-- Cannot set synchronized_standby_slots to a invalid slot name.
+ALTER SYSTEM SET synchronized_standby_slots='invalid*';

Typo: "a invalid" should be "an invalid"

Regards,

-- 
Fujii Masao

  34. Re: issue with synchronized_standby_slots

    Amit Kapila <amit.kapila16@gmail.com> — 2025-10-24T05:01:13Z

    On Thu, Oct 23, 2025 at 9:08 PM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Thu, Oct 23, 2025 at 8:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > +-- Parallel worker does not throw error during startup.
> > +SET min_parallel_table_scan_size TO 0;
> > +SET max_parallel_workers_per_gather TO 2;
> > +SET parallel_setup_cost TO 0;
> > +SET parallel_tuple_cost TO 0;
> > +CREATE TABLE t1(a int);
> > +INSERT INTO t1 VALUES(1), (2), (3), (4);
> > +SELECT count(*) FROM t1;
> >
> > Isn't it better to reset these parameters after the test?
>
> I think the intention of this test case is to verify that the issue seen
> in HEAD no longer occurs with the patch applied. So in HEAD this test
> procedure needs to be able to reproduce the problem with
> synchronized_standby_slots and parallel workers, but does it actually do?
> I'm afraid additional steps are needed.
>
> Also, I wonder if it's really worth doing this test after the fix,
> since it seems a special case.
>

Agreed. I also don't see the need.

>
> +-- Cannot set synchronized_standby_slots to a invalid slot name.
> +ALTER SYSTEM SET synchronized_standby_slots='invalid*';
>
> Typo: "a invalid" should be "an invalid"
>

We can keep this test as the results with and without patch will be
different for this case.

HEAD:
postgres=# ALTER SYSTEM SET synchronized_standby_slots='invalid*';
ERROR:  invalid value for parameter "synchronized_standby_slots": "invalid*"
DETAIL:  Replication slot "invalid*" does not exist.

Patch:
postgres=# ALTER SYSTEM SET synchronized_standby_slots='invalid*';
ERROR:  invalid value for parameter "synchronized_standby_slots": "invalid*"
DETAIL:  replication slot name "invalid*" contains invalid character
HINT:  Replication slot names may only contain lower case letters,
numbers, and the underscore character.

-- 
With Regards,
Amit Kapila.

  35. Re: issue with synchronized_standby_slots

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-10-24T07:20:56Z

    Hi Amit,
Thanks for reviewing the patch.

On Thu, 23 Oct 2025 at 16:58, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Oct 23, 2025 at 2:58 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
> >
> > On Thu, 23 Oct 2025 at 13:45, Hayato Kuroda (Fujitsu)
> > <kuroda.hayato@fujitsu.com> wrote:
> > >
> > > 2.
> > > Also, test for PG18 should not have the case which rejects the reserved name.
> > >
>
> Why to have that even for HEAD and PG18?
>
I added it as per comment in [1] to increase test coverage. I also do
not see any other existing test in HEAD hitting this error. So I added
this test.

> > > 3.
> > > ```
> > > -- Parallel worker does not throw error during startup.
> > > SET min_parallel_table_scan_size TO 0;
> > > SET parallel_setup_cost TO 0;
> > > SET parallel_tuple_cost TO 0;
> > > ```
> > >
> > > According to contrib/pg_stat_statements/sql/parallel.sql, max_parallel_workers_per_gather
> > > should be also set. There is a possiblity that `make installcheck` is used and
> > > it has max_parallel_workers_per_gather=0.
> > >
>
> +-- Parallel worker does not throw error during startup.
> +SET min_parallel_table_scan_size TO 0;
> +SET max_parallel_workers_per_gather TO 2;
> +SET parallel_setup_cost TO 0;
> +SET parallel_tuple_cost TO 0;
> +CREATE TABLE t1(a int);
> +INSERT INTO t1 VALUES(1), (2), (3), (4);
> +SELECT count(*) FROM t1;
>
> Isn't it better to reset these parameters after the test?
>
According to the latest discussion. I have removed this test.
Attached the updated patches.

[1]: https://www.postgresql.org/message-id/CAE9k0P%3DOFMFCRy9aDGWZ3bt91tbB1WnzsAbzXN72iWBaGVuMrw%40mail.gmail.com

Thanks,
Shlok Kyal

  36. Re: issue with synchronized_standby_slots

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-10-24T07:22:18Z

    Hi Fujii-san, Amit,

Thanks for reviewing the patch.

On Fri, 24 Oct 2025 at 10:31, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Oct 23, 2025 at 9:08 PM Fujii Masao <masao.fujii@gmail.com> wrote:
> >
> > On Thu, Oct 23, 2025 at 8:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > +-- Parallel worker does not throw error during startup.
> > > +SET min_parallel_table_scan_size TO 0;
> > > +SET max_parallel_workers_per_gather TO 2;
> > > +SET parallel_setup_cost TO 0;
> > > +SET parallel_tuple_cost TO 0;
> > > +CREATE TABLE t1(a int);
> > > +INSERT INTO t1 VALUES(1), (2), (3), (4);
> > > +SELECT count(*) FROM t1;
> > >
> > > Isn't it better to reset these parameters after the test?
> >
> > I think the intention of this test case is to verify that the issue seen
> > in HEAD no longer occurs with the patch applied. So in HEAD this test
> > procedure needs to be able to reproduce the problem with
> > synchronized_standby_slots and parallel workers, but does it actually do?
> > I'm afraid additional steps are needed.
> >
> > Also, I wonder if it's really worth doing this test after the fix,
> > since it seems a special case.
> >
>
> Agreed. I also don't see the need.
>
I also agree. I have removed the test in the latest version of patch.

> >
> > +-- Cannot set synchronized_standby_slots to a invalid slot name.
> > +ALTER SYSTEM SET synchronized_standby_slots='invalid*';
> >
> > Typo: "a invalid" should be "an invalid"
> >
>
> We can keep this test as the results with and without patch will be
> different for this case.
>
> HEAD:
> postgres=# ALTER SYSTEM SET synchronized_standby_slots='invalid*';
> ERROR:  invalid value for parameter "synchronized_standby_slots": "invalid*"
> DETAIL:  Replication slot "invalid*" does not exist.
>
> Patch:
> postgres=# ALTER SYSTEM SET synchronized_standby_slots='invalid*';
> ERROR:  invalid value for parameter "synchronized_standby_slots": "invalid*"
> DETAIL:  replication slot name "invalid*" contains invalid character
> HINT:  Replication slot names may only contain lower case letters,
> numbers, and the underscore character.
>
I have corrected the typo in the comment and also kept this test.

I have attached the patches in [1].
[1]: https://www.postgresql.org/message-id/CANhcyEXBnQs31N2zAH7m9uSNWfOBianZEMEAzi-rj_kQ3B9ihQ%40mail.gmail.com

Thanks,
Shlok Kyal

  37. RE: issue with synchronized_standby_slots

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-10-24T07:54:30Z

    Dear Shlok,

```
-- Can set synchronized_standby_slots to a non-existent slot name.
ALTER SYSTEM SET synchronized_standby_slots='missing';
SELECT pg_reload_conf();

-- Reset the GUC.
ALTER SYSTEM RESET synchronized_standby_slots;
SELECT pg_reload_conf();
```

pg_reload_conf() is called twice here but I'm not sure it is really needed.
ALTER SYSTEM itself can validate parameters via parse_and_validate_value(elevel=ERROR),
and pg_reload_conf() won't affect synchronously. Also, the backend won't output
even if the parameter is invalid.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

  38. Re: issue with synchronized_standby_slots

    Amit Kapila <amit.kapila16@gmail.com> — 2025-10-24T09:38:19Z

    On Fri, Oct 24, 2025 at 1:24 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shlok,
>
> ```
> -- Can set synchronized_standby_slots to a non-existent slot name.
> ALTER SYSTEM SET synchronized_standby_slots='missing';
> SELECT pg_reload_conf();
>
> -- Reset the GUC.
> ALTER SYSTEM RESET synchronized_standby_slots;
> SELECT pg_reload_conf();
> ```
>
> pg_reload_conf() is called twice here but I'm not sure it is really needed.
> ALTER SYSTEM itself can validate parameters via parse_and_validate_value(elevel=ERROR),
> and pg_reload_conf() won't affect synchronously.

I also think pg_reload_conf() won't be required here.

-- 
With Regards,
Amit Kapila.

  39. Re: issue with synchronized_standby_slots

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-10-24T10:34:09Z

    On Fri, 24 Oct 2025 at 15:08, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Oct 24, 2025 at 1:24 PM Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
> >
> > Dear Shlok,
> >
> > ```
> > -- Can set synchronized_standby_slots to a non-existent slot name.
> > ALTER SYSTEM SET synchronized_standby_slots='missing';
> > SELECT pg_reload_conf();
> >
> > -- Reset the GUC.
> > ALTER SYSTEM RESET synchronized_standby_slots;
> > SELECT pg_reload_conf();
> > ```
> >
> > pg_reload_conf() is called twice here but I'm not sure it is really needed.
> > ALTER SYSTEM itself can validate parameters via parse_and_validate_value(elevel=ERROR),
> > and pg_reload_conf() won't affect synchronously.
>
> I also think pg_reload_conf() won't be required here.
>
I agree that .pg_reload_conf() is not required. I have removed it in
the latest version of patches.

Thanks,
Shlok Kyal

  40. RE: issue with synchronized_standby_slots

    Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com> — 2025-10-27T04:05:49Z

    Dear Shlok,

> I agree that .pg_reload_conf() is not required. I have removed it in
> the latest version of patches.

Thanks for updating the patch!
After considering bit more, I started to feel the test should be under the
test/modules/unsafe_tests. Adding test requires that wal_level >= replica and
max_logical_replication_slots >= 1, but `make installcheck` might allow to run
such an environment. unsafe_tests can ensure that test would be run only while
`make check`. Also, ALTER SYSTEM requires to be superuser and the test has
already used the privileges.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

  41. Re: issue with synchronized_standby_slots

    Shlok Kyal <shlok.kyal.oss@gmail.com> — 2025-10-27T04:24:18Z

    On Mon, 27 Oct 2025 at 09:35, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Shlok,
>
> > I agree that .pg_reload_conf() is not required. I have removed it in
> > the latest version of patches.
>
> Thanks for updating the patch!
> After considering bit more, I started to feel the test should be under the
> test/modules/unsafe_tests. Adding test requires that wal_level >= replica and
> max_logical_replication_slots >= 1, but `make installcheck` might allow to run
> such an environment. unsafe_tests can ensure that test would be run only while
> `make check`. Also, ALTER SYSTEM requires to be superuser and the test has
> already used the privileges.
>
Hi Kuroda-san,

I also agree with the analysis. I have added the test in 'guc_privs.sql'.

Thanks,
Shlok Kyal