On Mon, Oct 30, 2023 at 1:42 PM Kyotaro Horiguchi
<horikyota....@gmail.com> wrote:
>
> At Mon, 30 Oct 2023 12:38:47 +0530, Bharath Rupireddy
> <bharath.rupireddyforpostg...@gmail.com> wrote in
> > Will the check_hook approach work correctly? I haven't checked that by
> > myself, but I see InitializeGUCOptions() getting called before
> > IsBinaryUpgrade is set to true and the passed-in config options ('c')
> > are parsed.
>
> I'm not sure about the wanted behavior exactly, but the fast you
> pointed doesn't matter because the check is required after parsing the
> command line options. On the other hand, I'm not sure about the
> behavior that a setting in postgresql.conf is rejected.
Yeah. The check_hook is called even after the param is specified in
postgresql.conf during the upgrade, so I see no problem there.
> > If the check_hook approach works correctly, I think we must add a test
> > hitting the error in check_max_slot_wal_keep_size for the
> > IsBinaryUpgrade case. And, I agree with Amit to have a detailed
> > messaging with GUC_check_errmsg/GUC_check_errdetail. Also, IMV,
> > leaving the error message in InvalidatePossiblyObsoleteSlot() there
> > (if required with a better wording as discussed initially in this
> > thread) does no harm. Actually, it acts as another safety net given
> > that max_slot_wal_keep_size GUC is reloadable via SIGHUP.
>
> The error message, which is deemed impossible, adds an additional
> message translation. In another thread, we are discussing the
> reduction of translatable messages. Therefore, I suggest using elog()
> for the condition at the very least. Whether it should be elog() or
> Assert() remains open for discussion, as I don't have a firm stance on
> it.
I get it. I agree to go with just the assert because the GUC
check_hook kinda tightens the screws against setting
max_slot_wal_keep_size to a value other than -1 during the binary
upgrade,
A few comments on your inhibit_m_s_w_k_s_during_upgrade_2.txt:
1.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
