On Thu, Nov 20, 2025 at 3:54 PM Chao Li <li.evan.chao@gmail.com> wrote:
> Before this patch, all user specified options are silently discarded,
The GUC settings in CREATE SUBSCRIPTION were honored up through v14;
the behavior changed in commit f3d4019da5d, so some might view this
as a regression.
> now all user specified options expect the 3 will be kept, will that expose a hold where user badly specifies some
optionthat may break logical replication? If that’s true, then we need to parse user specified options and do some
verifications.
>
Yeah, I agree that if certain GUCs can break logical replication,
we should enforce "safe" values, just as we currently do for datestyle.
And if any other GUCs can cause the issue, they could affect
postgres_fdw etc, so the fix would need to be broader.
> I just reviewed v2, and got some comments:
Thanks for the review!
>
> 1.
> ```
> + char sql[100];
> ```
>
> Hardcode 100 here doesn’t look good. If you decide to keep, I won’t have a strong objection.
I think hardcoding 100 here is sufficient, since the queries built on
that buffer are fixed and clearly fit within that limit.
>
> 2
> ```
> + const char *params[] =
> + {"datestyle", "intervalstyle", "extra_float_digits", NULL};
> + const char *values[] = {"ISO", "postgres", "3", NULL};
> ```
>
> Nit: we don’t need to have a NULL terminator element. We can use lengthof() macro to get array length. lengthof() is
definedin c.h.
Okay, I'll adjust the patch accordingly.
>
> 3. To minimize the network round-trip, maybe we can combine the 3 set into a single statement?
As for the extra network round trip, I still doubt it will matter
in practice given that it happens only at replication connection startup.
>
> 4. The commit message:
> ```
> This commit removes the restriction by changing how logical replication
> connections are established so that GUC settings in the CONNECTION string
> are properly passed through to and uesd by the walsender. This enables
> ```
>
> This is a little bit inaccurate, all user specified settings expected the 3 ones being overwritten will be honored.
Are you suggesting that, because datestyle and the other two parameters
specified in CONNECTION aren(t actually applied by the walsender,
the commit message should explicitly mention that not all parameters
from CONNECTION are used?
Regards,
--
Fujii Masao
