On Mon, Nov 29, 2021 at 05:04:01PM +0900, Michael Paquier wrote:
> 0001, to adjust the units, and 0003, to make the GUC descriptions less
> unit-dependent, are good ideas.
Actually, after sleeping on it and doing some digging in
codesearch.debian.org, changing the units of max_identifier_length,
block_size and wal_block_size could induce some breakages for anything
using a SHOW command, something that becomes particularly worse now
that SHOW is supported in replication connections, and it would force
clients to know and parse the units of a value. Perhaps I am being
too careful here, but that could harm a lot of users. It is worth
noting that I have found some driver code making use of pg_settings,
which would not be influenced by such a change, but it is unsafe to
assume that everybody does that.
The addition of GUC_EXPLAIN for enable_incremental_sort, the comment
fix for autovacuum_freeze_max_age, the use of COMPAT_OPTIONS_PREVIOUS
for ssl_renegotiation_limit and the addition of GUC_NOT_IN_SAMPLE for
trace_recovery_messages are fine, though.
> I am not completely sure that all the contents of 0002 are
> improvements, but the suggestions done for huge_pages,
> ssl_passphrase_command_supports_reload, checkpoint_warning and
> commit_siblings seem fine to me.
Hmm, I think the patched description of checkpoint_warning is not that
much an improvement compared to the current one. While the current
description uses the term "checkpoint segments", which is, I agree,
weird. The new one would lose the term "checkpoint", making the short
description of the parameter lose some of its context.
I have done a full review of the patch set, and applied the obvious
fixes/improvements as of be54551. Attached is an extra patch based on
the contents of the whole set sent upthread:
- Improvement of the description of checkpoint_segments.
- Reworded the description of all parameters using "N units", rather
than just switching to "this period of units". I have been using
something more generic.
Thoughts?
--
Michael