Horton

Re: issue with synchronized_standby_slots

Shlok Kyal <shlok.kyal.oss@gmail.com>

View thread
From: Shlok Kyal <shlok.kyal.oss@gmail.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Fujii Masao <masao.fujii@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, Fabrice Chapuis <fabrice636861@gmail.com>, Ashutosh Sharma <ashu.coek88@gmail.com>, Rahila Syed <rahilasyed90@gmail.com>, Alexander Kukushkin <cyberdemn@gmail.com>, "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2025-10-24T07:22:18Z
Lists: pgsql-hackers

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.

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