Moving the vacuum GUCs' docs out of the Client Connection Defaults section

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 Mon, Jan 6, 2025 at 8:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

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).

- Melanie

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 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. For clarity, I added a note to the commit message
that the actual GUCs' docs are just lifted and shifted.

I decided in the end not to wrap it anyway, though. I tried it and
didn't like the result. If I wrap it to 78, that actually makes a good
number of the GUCs' docs wider (i.e. they were wrapped to less than
78). Which means future additions are more likely to need to wrap (as
Tom mentioned upthread).

Attached is v2 (required a rebase).

- 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 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.

+ 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.

+      <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.

I hear you. I couldn't think of something much clearer, so I left it as is.

+         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.

- Melanie

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]https://www.postgresql.org/docs/devel/bookindex.html has an entry for MultiXacts but it goes straight to
MultiXacts and Wraparound [2]https://www.postgresql.org/docs/devel/routine-vacuuming.html#VACUUM-FOR-MULTIXACT-WRAPAROUND.

- Melanie

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