At Mon, 30 Oct 2023 12:38:47 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@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.
> 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.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center