Обсуждение: Incorrect GUC descriptions in docs and postgresql.conf.sample

Hi all,

I got curious with what Justin just told here with
max_logical_replication_workers:
https://www.postgresql.org/message-id/20210526001359.GE3676@telsasoft.com

And while looking at the full set of GUCs, I noticed much more than
one parameter that needed adjustments in the documentation when these
are PGC_SIGHUP or PGC_POSTMASTER, leading me to the attached patch.

Any comments or objections?

Thanks,
--
Michael

Your patch adds documentation about GUCs that can only be set at server
start/config/commandline.

But it's not true for any of these, which are all HUP/SUSET.
Please double check your logic :)

src/backend/utils/misc/guc.c:           {"autovacuum_work_mem", PGC_SIGHUP, RESOURCES_MEM,
src/backend/utils/misc/guc.c:           {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS,
src/backend/utils/misc/guc.c:           {"restart_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS,
src/backend/utils/misc/guc.c:           {"log_lock_waits", PGC_SUSET, LOGGING_WHAT,
src/backend/utils/misc/guc.c:           {"autovacuum_work_mem", PGC_SIGHUP, RESOURCES_MEM,
src/backend/utils/misc/guc.c:           {"ssl_max_protocol_version", PGC_SIGHUP, CONN_AUTH_SSL,
src/backend/utils/misc/guc.c:           {"ssl_min_protocol_version", PGC_SIGHUP, CONN_AUTH_SSL,

-- 
Justin

On Tue, May 25, 2021 at 08:43:14PM -0500, Justin Pryzby wrote:
> Your patch adds documentation about GUCs that can only be set at server
> start/config/commandline.

Oh: I realized that I read too quickly and misinterpretted what "only be set in
the config" means (I know I'm not the only one).  Oops.

In some cases it sounds strange to say that a parameter can "only" be set in
the config file, since it's dynamically changed at runtime.  Which is more
flexible than restrictive.

of a restriction.

> But it's not true for any of these, which are all HUP/SUSET.
> Please double check your logic :)
> 
> src/backend/utils/misc/guc.c:           {"autovacuum_work_mem", PGC_SIGHUP, RESOURCES_MEM,
> src/backend/utils/misc/guc.c:           {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS,
> src/backend/utils/misc/guc.c:           {"restart_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS,
> src/backend/utils/misc/guc.c:           {"log_lock_waits", PGC_SUSET, LOGGING_WHAT,
> src/backend/utils/misc/guc.c:           {"autovacuum_work_mem", PGC_SIGHUP, RESOURCES_MEM,
> src/backend/utils/misc/guc.c:           {"ssl_max_protocol_version", PGC_SIGHUP, CONN_AUTH_SSL,
> src/backend/utils/misc/guc.c:           {"ssl_min_protocol_version", PGC_SIGHUP, CONN_AUTH_SSL,

-- 
Justin

On Tue, May 25, 2021 at 09:01:30PM -0500, Justin Pryzby wrote:
> On Tue, May 25, 2021 at 08:43:14PM -0500, Justin Pryzby wrote:
>> Your patch adds documentation about GUCs that can only be set at server
>> start/config/commandline.
>
> Oh: I realized that I read too quickly and misinterpretted what "only be set in
> the config" means (I know I'm not the only one).  Oops.
>
> In some cases it sounds strange to say that a parameter can "only" be set in
> the config file, since it's dynamically changed at runtime.  Which is more
> flexible than restrictive.

That's the wording used for ages in the documentation, so I would
stick with that.

>> But it's not true for any of these, which are all HUP/SUSET.
>> Please double check your logic :)
>>
>> src/backend/utils/misc/guc.c:           {"autovacuum_work_mem", PGC_SIGHUP, RESOURCES_MEM,
>> src/backend/utils/misc/guc.c:           {"remove_temp_files_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS,
>> src/backend/utils/misc/guc.c:           {"restart_after_crash", PGC_SIGHUP, ERROR_HANDLING_OPTIONS,
>> src/backend/utils/misc/guc.c:           {"log_lock_waits", PGC_SUSET, LOGGING_WHAT,
>> src/backend/utils/misc/guc.c:           {"autovacuum_work_mem", PGC_SIGHUP, RESOURCES_MEM,
>> src/backend/utils/misc/guc.c:           {"ssl_max_protocol_version", PGC_SIGHUP, CONN_AUTH_SSL,
>> src/backend/utils/misc/guc.c:           {"ssl_min_protocol_version", PGC_SIGHUP, CONN_AUTH_SSL,

There is one point where you are right here: log_lock_waits has no
need to be changed.  Looks like I checked too many things at once.
--
Michael

On Wed, May 26, 2021 at 11:10:22AM +0900, Michael Paquier wrote:
> There is one point where you are right here: log_lock_waits has no
> need to be changed.  Looks like I checked too many things at once.

Fixed that, did one extra round of review, and applied.
--
Michael