Обсуждение: Moving the vacuum GUCs' docs out of the Client Connection Defaults section

I was reviewing all the vacuum related GUCs, and I noticed that they
fall into three main subsections of Chapter 19 (Server Configuration)
in the docs [1]: Automatic Vacuuming [2], Resource Consumption [3],
and Client Connection Defaults [4]. The last one I find pretty
confusing.

vacuum_freeze_min_age, vacuum_freeze_table_age, vacuum_failsafe_age
and their multixact equivalents are all in the Statement Behavior
subsection of the Client Connection Defaults subsection. I could maybe
see a justification for this if these GUCs only affected VACUUM
statements -- but that's not the case. All of these GUCs affect the
behavior of both manually invoked vacuums and autovacuums (with some
caveats about precedence when equivalent table storage parameters are
specified).
But maybe there is some other reason they are documented there that I
am missing.
If not, maybe we should fix it?

I'm not totally sure what the solution should be. Perhaps we rename
the "Automatic Vacuuming" subsection "Vacuuming" and then make
"Automatic Vacuuming" a sub-subsection? And move the vacuum-related
GUCs from client connection defaults to "Vacuuming"?

- Melanie

[1] https://www.postgresql.org/docs/17/runtime-config.html
[2] https://www.postgresql.org/docs/17/runtime-config-autovacuum.html
[3] https://www.postgresql.org/docs/17/runtime-config-resource.html
[4] https://www.postgresql.org/docs/17/runtime-config-client.html

Melanie Plageman <melanieplageman@gmail.com> writes:
> I was reviewing all the vacuum related GUCs, and I noticed that they
> fall into three main subsections of Chapter 19 (Server Configuration)
> in the docs [1]: Automatic Vacuuming [2], Resource Consumption [3],
> and Client Connection Defaults [4]. The last one I find pretty
> confusing.

Yeah, it's a mess.  It sounds good to consolidate all of those under
a top-level Vacuuming section.

            regards, tom lane

On Tue, Jan 7, 2025 at 12:15 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> Cool, I've attached a patch to do this. I left a few of the GUCs under
> Resource Consumption (like autovacuum_work_mem and
> vacuum_buffer_usage_limit) where they are because it seemed
> appropriate.
>
> This is my first docs patch that introduces new sections and such, so
> I'm not sure I got the indentation 100% correct (I, of course, tried
> to follow conventions).

Oh, one thing I forgot to say. Though I increased the indentation of
some of the subsections that I moved, I didn't rewrap the lines
because they were already not wrapped to 78. I can do this, but I
can't tell from the existing docs what text width the paragraphs of
text are supposed to be wrapped to.

- Melanie

On Tue, Jan 07, 2025 at 12:15:17PM -0500, Melanie Plageman wrote:
> This is my first docs patch that introduces new sections and such, so
> I'm not sure I got the indentation 100% correct (I, of course, tried
> to follow conventions).

I haven't reviewed the patch in depth, but I think it's worth considering
whether this change will break any links that work in one version but break
if you change the version number.  I believe appendix-obsolete.sgml is
designed to help with that a bit, but I've had mixed results when I've
tried to use it.  Of course, it's also possible that we don't care too
much...

But overall, consolidating the docs for the vacuum GUCs seems like a good
idea.

-- 
nathan

Melanie Plageman <melanieplageman@gmail.com> writes:
>> This is my first docs patch that introduces new sections and such, so
>> I'm not sure I got the indentation 100% correct (I, of course, tried
>> to follow conventions).

There really isn't much convention there :-(.  The amount of
indentation used varies wildly across different chunks of our docs.
I'd try to make it look like nearby sections, but those might
themselves be inconsistent.

> Oh, one thing I forgot to say. Though I increased the indentation of
> some of the subsections that I moved, I didn't rewrap the lines
> because they were already not wrapped to 78. I can do this, but I
> can't tell from the existing docs what text width the paragraphs of
> text are supposed to be wrapped to.

If you're moving the text anyway, I'd rewrap it to something less than
80 columns.  Again, there's not a lot of uniformity about exactly how
much less.  I frequently use 74-column width for new docs text, to
leave some room for minor adjustments without having to rewrap;
but I don't think other people follow that rule.

A lot of the existing violations of 80-column right margin come from
when we switched to XML rules and had to fill out abbreviated closing
tags ("</>") to full tags ("</foo>").  That was done with some more or
less automated conversion that didn't do anything about rewrapping
lines that it made too long.  I think that choice was fine, because
it reduced the size of the diff.  But if you're rewriting or moving a
para, that's a good time to clean things up by rewrapping it; it'll
all be new according to "git blame" anyway.

            regards, tom lane

Nathan Bossart <nathandbossart@gmail.com> writes:
> I haven't reviewed the patch in depth, but I think it's worth considering
> whether this change will break any links that work in one version but break
> if you change the version number.  I believe appendix-obsolete.sgml is
> designed to help with that a bit, but I've had mixed results when I've
> tried to use it.  Of course, it's also possible that we don't care too
> much...

Yeah, this change:

-   <sect1 id="runtime-config-autovacuum">
+   <sect1 id="runtime-config-vacuum">

will change the doc page's URL and thus break any external links or
bookmarks for the whole page or anything on it.  That's not the
end of the world, but it's slightly annoying.  appendix-obsolete.sgml
seems to be designed for slightly more heavyweight changes, where
it's worth having an intermediate page that explains what changed.
Here I think we'd just like a redirect.

I might be wrong, but I had the idea that our docs website has a
capability to provide such redirects.  You'd probably need to ask
about that on the pgsql-www list, unless somebody who knows the
answer notices this thread.

            regards, tom lane

> On 7 Jan 2025, at 21:14, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I might be wrong, but I had the idea that our docs website has a
> capability to provide such redirects.  You'd probably need to ask
> about that on the pgsql-www list, unless somebody who knows the
> answer notices this thread.

There is functionality in pgweb to provide a page alias for whenever a page is
renamed to keep the links to other versions working.  One example is:

  https://www.postgresql.org/docs/9.4/catalog-pg-replication-slots.html
  https://www.postgresql.org/docs/17/view-pg-replication-slots.html

There is also a redirect functionality, which isn't used anywhere right now,
but ideally could be used to redirect bookmarks for /current/<oldname> to
/current/<newname>.

--
Daniel Gustafsson

On 07.01.25 18:31, Melanie Plageman wrote:
> On Tue, Jan 7, 2025 at 12:15 PM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
>>
>> Cool, I've attached a patch to do this. I left a few of the GUCs under
>> Resource Consumption (like autovacuum_work_mem and
>> vacuum_buffer_usage_limit) where they are because it seemed
>> appropriate.
>>
>> This is my first docs patch that introduces new sections and such, so
>> I'm not sure I got the indentation 100% correct (I, of course, tried
>> to follow conventions).
> 
> Oh, one thing I forgot to say. Though I increased the indentation of
> some of the subsections that I moved, I didn't rewrap the lines
> because they were already not wrapped to 78. I can do this, but I
> can't tell from the existing docs what text width the paragraphs of
> text are supposed to be wrapped to.

For a patch that moves things around like this, I would leave everything 
as is and not rewrap.  That makes it easier to follow what's going on 
with "git diff -w", "git show -w" etc.

In .dir-locals.el, there is this configuration for Emacs:

  (nxml-mode . ((fill-column . 78)
                (indent-tabs-mode . nil)))

So that's one data point about what the line length should be.

On Wed, Jan 8, 2025 at 6:35 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 7 Jan 2025, at 21:14, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> > I might be wrong, but I had the idea that our docs website has a
> > capability to provide such redirects.  You'd probably need to ask
> > about that on the pgsql-www list, unless somebody who knows the
> > answer notices this thread.
>
> There is functionality in pgweb to provide a page alias for whenever a page is
> renamed to keep the links to other versions working.  One example is:
>
>   https://www.postgresql.org/docs/9.4/catalog-pg-replication-slots.html
>   https://www.postgresql.org/docs/17/view-pg-replication-slots.html
>
> There is also a redirect functionality, which isn't used anywhere right now,
> but ideally could be used to redirect bookmarks for /current/<oldname> to
> /current/<newname>.

Thanks to Nathan and Tom for noticing and Daniel for replying. So, if
I understand correctly, pgweb will do this for me without me needing
to do anything in my patch?

- Melanie

> On 9 Jan 2025, at 02:30, Melanie Plageman <melanieplageman@gmail.com> wrote:
>
> On Wed, Jan 8, 2025 at 6:35 AM Daniel Gustafsson <daniel@yesql.se> wrote:
>>
>>> On 7 Jan 2025, at 21:14, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>>> I might be wrong, but I had the idea that our docs website has a
>>> capability to provide such redirects.  You'd probably need to ask
>>> about that on the pgsql-www list, unless somebody who knows the
>>> answer notices this thread.
>>
>> There is functionality in pgweb to provide a page alias for whenever a page is
>> renamed to keep the links to other versions working.  One example is:
>>
>>  https://www.postgresql.org/docs/9.4/catalog-pg-replication-slots.html
>>  https://www.postgresql.org/docs/17/view-pg-replication-slots.html
>>
>> There is also a redirect functionality, which isn't used anywhere right now,
>> but ideally could be used to redirect bookmarks for /current/<oldname> to
>> /current/<newname>.
>
> Thanks to Nathan and Tom for noticing and Daniel for replying. So, if
> I understand correctly, pgweb will do this for me without me needing
> to do anything in my patch?

Correct, it's a setting in the database on the postgresql.org website.

--
Daniel Gustafsson

On 09.01.25 02:45, Melanie Plageman wrote:
> On Wed, Jan 8, 2025 at 8:39 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>>
>> On 07.01.25 18:31, Melanie Plageman wrote:
>>>
>>> Oh, one thing I forgot to say. Though I increased the indentation of
>>> some of the subsections that I moved, I didn't rewrap the lines
>>> because they were already not wrapped to 78. I can do this, but I
>>> can't tell from the existing docs what text width the paragraphs of
>>> text are supposed to be wrapped to.
>>
>> For a patch that moves things around like this, I would leave everything
>> as is and not rewrap.  That makes it easier to follow what's going on
>> with "git diff -w", "git show -w" etc.
>>
>> In .dir-locals.el, there is this configuration for Emacs:
>>
>>    (nxml-mode . ((fill-column . 78)
>>                  (indent-tabs-mode . nil)))
>>
>> So that's one data point about what the line length should be.
> 
> Well, in this case, the diff won't look different with git show/diff
> -w because moving them to another part of the file is more than a
> whitespace change.

Correct.  The right option is

     git diff --color-moved

This can also be put into .gitconfig.

> On 9 Jan 2025, at 02:45, Melanie Plageman <melanieplageman@gmail.com> wrote:

> Attached is v2 (required a rebase).

I think this is a really good restructuring which will make life easier for our
users.  Some of the comments below are on wording which wasn't introduced in
this patch, but which I hadn't thought about before, so feel free to ignore
those comments.

+    <sect2 id="runtime-config-vacuum-freezing">
+     <title>Freezing</title>
+
+     <para>
Trying to read this as a new user, I think it would be good to start this
subsection with a sentence describing what freezing actually is.  Vacuum is
hard enough for users as it is =)


+         default value is one.
Grepping around indicates that we typically use the numeric value rather than
writing it in text, and the next settting down has "default value is 2".  For
consistency I would change that to "1" instead of "one".


+      <varname>vacuum_cost_delay</varname>. Then it will reset the counter and
+      continue execution.
I know starting a sentence with "Then" is grammatically correct when it's the
last sentence in a paragraph, but being a non-native speaker I always find
myself re-reading such sentences to parse them as they stand out as odd.


+         can be set to fractional-millisecond values, such delays may not be
+         measured accurately on older platforms.  On such platforms,
This sentence seems quite vague and hard to act on for users, what qualifies as
an "older platform" by the time v18 rolls around (this was added in v12).  I'm
sure there are such platforms in existence that postgres 18 will run on, but
are we helping users with ambiguity?

--
Daniel Gustafsson

> On 10 Jan 2025, at 23:09, Melanie Plageman <melanieplageman@gmail.com> wrote:
>
> On Thu, Jan 9, 2025 at 5:05 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>>
>> I think this is a really good restructuring which will make life easier for our
>> users.  Some of the comments below are on wording which wasn't introduced in
>> this patch, but which I hadn't thought about before, so feel free to ignore
>> those comments.
>>
>> +    <sect2 id="runtime-config-vacuum-freezing">
>> +     <title>Freezing</title>
>> +
>> +     <para>
>> Trying to read this as a new user, I think it would be good to start this
>> subsection with a sentence describing what freezing actually is.  Vacuum is
>> hard enough for users as it is =)
>
> I've taken a stab at improving this. Let me know if you think it works.

I like what you added, it's IMO the right level of detail here.

>> +         default value is one.
>> Grepping around indicates that we typically use the numeric value rather than
>> writing it in text, and the next settting down has "default value is 2".  For
>> consistency I would change that to "1" instead of "one".
>
> I think this is a reasonable cleanup to lump in with the rest of this
> commit. I have taken the liberty of also adding a <literal> tag and
> then updating the other places in the proposed "Vacuuming" subsection
> where a literal default value is specified without the <literal> tag.

LGTM.  We're not entirely consistent with how we mark up the default value, but
I think moving towards using <literal>value</literal> whenever we are changing
things there anyways is the right level of cleanup.

>> +         can be set to fractional-millisecond values, such delays may not be
>> +         measured accurately on older platforms.  On such platforms,
>> This sentence seems quite vague and hard to act on for users, what qualifies as
>> an "older platform" by the time v18 rolls around (this was added in v12).  I'm
>> sure there are such platforms in existence that postgres 18 will run on, but
>> are we helping users with ambiguity?
>
> While I agree that what counted as older hardware in 2019 may no
> longer be around at all, I am more hesitant to update this in the same
> commit as a bunch of other cut-and-pastes. Someone at some point
> decided this was important to point out, and I don't have sufficient
> evidence that it no longer makes sense. And if I did, I'd probably
> want to update this part of the docs in a dedicated commit.

To be fair I didn't really expect this to be changed as part of this (and I
should written that in my email), but it stood out when reading so wanted to
point it out.  No objections to leaving it as is as part of this work.

+         Specifies the cutoff age (in multixacts) that <command>VACUUM</command>
I wish we had a glossary entry for multixact we could link to here.  But,
that's not really the responsibility of this patch to fix, just something that
came to mind when reading the resulting page.

+1 on going ahead with this version, there are still improvements we can make
to the vacuum config docs but that shouldn't stand in the way of a reorg patch
which improves overall presentation structure.

--
Daniel Gustafsson

On Fri, Jan 10, 2025 at 6:00 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 10 Jan 2025, at 23:09, Melanie Plageman <melanieplageman@gmail.com> wrote:
> >
> > On Thu, Jan 9, 2025 at 5:05 PM Daniel Gustafsson <daniel@yesql.se> wrote:
> >>
> >> I think this is a really good restructuring which will make life easier for our
> >> users.  Some of the comments below are on wording which wasn't introduced in
> >> this patch, but which I hadn't thought about before, so feel free to ignore
> >> those comments.
> >>
> >> +    <sect2 id="runtime-config-vacuum-freezing">
> >> +     <title>Freezing</title>
> >> +
> >> +     <para>
> >> Trying to read this as a new user, I think it would be good to start this
> >> subsection with a sentence describing what freezing actually is.  Vacuum is
> >> hard enough for users as it is =)
> >
> > I've taken a stab at improving this. Let me know if you think it works.
>
> I like what you added, it's IMO the right level of detail here.

Cool, thanks! I pushed.

> +         Specifies the cutoff age (in multixacts) that <command>VACUUM</command>
> I wish we had a glossary entry for multixact we could link to here.  But,
> that's not really the responsibility of this patch to fix, just something that
> came to mind when reading the resulting page.

Personally, I really wish we had a docs page explaining more about
what mutlixacts are and maybe even a bit about the architecture. FWIW,
the index [1] has an entry for MultiXacts but it goes straight to
MultiXacts and Wraparound [2].

- Melanie

[1] https://www.postgresql.org/docs/devel/bookindex.html
[2] https://www.postgresql.org/docs/devel/routine-vacuuming.html#VACUUM-FOR-MULTIXACT-WRAPAROUND