confusing positioning of notes in connection settings
While testing a few other things on the connection settings page[1]https://www.postgresql.org/docs/current/runtime-config-connection.html#GUC-TCP-KEEPALIVES-COUNT, I
noticed the notes on the "tcp_*" family of settings. While scrolling
further down the page, I found myself slightly confused over which note
corresponded to which setting (example in screenshot).
Given the nature of these notes, i.e. to say that the setting is not
supported in Windows, couldn't we just add that text to the description
of the parameter and remove the note? I think that'd make it a bit
clearer which comment applies to which parameter.
While arguably this is not a big deal now, the new deep-linking work for
v16[2]https://www.postgresql.org/docs/devel/runtime-config-connection.html#GUC-TCP-KEEPALIVES-COUNT could make this a bit more confusing.
Thoughts?
Thanks,
Jonathan
[1]: https://www.postgresql.org/docs/current/runtime-config-connection.html#GUC-TCP-KEEPALIVES-COUNT
https://www.postgresql.org/docs/current/runtime-config-connection.html#GUC-TCP-KEEPALIVES-COUNT
[2]: https://www.postgresql.org/docs/devel/runtime-config-connection.html#GUC-TCP-KEEPALIVES-COUNT
https://www.postgresql.org/docs/devel/runtime-config-connection.html#GUC-TCP-KEEPALIVES-COUNT
Attachments:
Screen Shot 2023-04-22 at 3.46.34 PM.pngimage/png; name="Screen Shot 2023-04-22 at 3.46.34 PM.png"Download+3-2
On 22.04.23 21:53, Jonathan S. Katz wrote:
While testing a few other things on the connection settings page[1], I
noticed the notes on the "tcp_*" family of settings. While scrolling
further down the page, I found myself slightly confused over which note
corresponded to which setting (example in screenshot).Given the nature of these notes, i.e. to say that the setting is not
supported in Windows, couldn't we just add that text to the description
of the parameter and remove the note? I think that'd make it a bit
clearer which comment applies to which parameter.
I agree we could fold the notes (and other similar notes nearby) into
the main text. Or we could just remove them, because the main text
already contains the information in more general form.
I wonder if the notes are even true. The text for
tcp_keepalives_interval already says that it is only supported if
TCP_KEEPCNT is supported. There is no specific code for Windows.
Random googling suggests that TCP_KEEPCNT does exist on Windows.
On 26 Apr 2023, at 08:08, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
I wonder if the notes are even true. The text for tcp_keepalives_interval already says that it is only supported if TCP_KEEPCNT is supported.
Don't forget the "and on Windows;" part.
There is no specific code for Windows.
We have pq_setkeepaliveswin32() for Windows which can work on Windows 2000 and
later versions based on the existence of SIO_KEEPALIVE_VALS.
Random googling suggests that TCP_KEEPCNT does exist on Windows.
For Windows what you want is SIO_KEEPALIVE_VALS.
--
Daniel Gustafsson
On 26 Apr 2023, at 10:18, Daniel Gustafsson <daniel@yesql.se> wrote:
On 26 Apr 2023, at 08:08, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
I wonder if the notes are even true. The text for tcp_keepalives_interval already says that it is only supported if TCP_KEEPCNT is supported.
Re-reading this I think there was some confusion, definitely so on my part.
tcp_keepalives_interval relies on TCP_KEEPINTVL, with the Windows equivalent
being SIO_KEEPALIVE_VALS. TCP_KEEPCNT is for tcp_keepalives_count which indeed
is not supported on Windows. Jonathans original question was regarding _count
and _timeout and not _interval.
I do agree that all of these notes may just as well be added to the text, the
option client_connection_check_interval following these have text about
platform compatibility without using a note.
--
Daniel Gustafsson
On 26.04.23 07:36, Daniel Gustafsson wrote:
On 26 Apr 2023, at 10:18, Daniel Gustafsson <daniel@yesql.se> wrote:
On 26 Apr 2023, at 08:08, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
I wonder if the notes are even true. The text for tcp_keepalives_interval already says that it is only supported if TCP_KEEPCNT is supported.
Re-reading this I think there was some confusion, definitely so on my part.
tcp_keepalives_interval relies on TCP_KEEPINTVL, with the Windows equivalent
being SIO_KEEPALIVE_VALS. TCP_KEEPCNT is for tcp_keepalives_count which indeed
is not supported on Windows. Jonathans original question was regarding _count
and _timeout and not _interval.I do agree that all of these notes may just as well be added to the text, the
option client_connection_check_interval following these have text about
platform compatibility without using a note.
How about this patch?
The first two hunks are pretty straightforward, they just move the
existing text around.
For the other two, which are not supported on Windows, I added an
explicit parenthetical note. We don't list which of the Unix-like
platforms support the respective options, but I suspect that it's all of
them in practice? (Otherwise we should be more explicit.) So I think
calling out Windows explicitly is sensible, also considering that the
first two settings are supported on Windows but the latter two are not.
Attachments:
0001-doc-Fix-confusing-positioning-of-notes-in-connection.patchtext/plain; charset=UTF-8; name=0001-doc-Fix-confusing-positioning-of-notes-in-connection.patchDownload+6-25
On 31 May 2023, at 13:16, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
The first two hunks are pretty straightforward, they just move the existing text around.
For the other two, which are not supported on Windows, I added an explicit parenthetical note. We don't list which of the Unix-like platforms support the respective options, but I suspect that it's all of them in practice? (Otherwise we should be more explicit.) So I think calling out Windows explicitly is sensible, also considering that the first two settings are supported on Windows but the latter two are not.
I think this is a clear improvement over the current docs.
Should we call out Windows explicitly in the same way in the corresponding
options in the libpq connection string param docs as well?
--
Daniel Gustafsson
On 5/31/23 7:53 AM, Daniel Gustafsson wrote:
On 31 May 2023, at 13:16, Peter Eisentraut <peter.eisentraut@enterprisedb.com> wrote:
The first two hunks are pretty straightforward, they just move the existing text around.
For the other two, which are not supported on Windows, I added an explicit parenthetical note. We don't list which of the Unix-like platforms support the respective options, but I suspect that it's all of them in practice? (Otherwise we should be more explicit.) So I think calling out Windows explicitly is sensible, also considering that the first two settings are supported on Windows but the latter two are not.
I think this is a clear improvement over the current docs.
+1.
Small nit:
"which does not include Windows" =>
"which is not supported on Windows"
(in two places)
Should we call out Windows explicitly in the same way in the corresponding
options in the libpq connection string param docs as well?
Probably, but I see language like this:
"It is only supported on systems where TCP_USER_TIMEOUT is available; on
other systems, it has no effect."
If this is really only unsupported / has different settings on Windows,
I think it's OK to call that out. The original gripe was about
readability, but if we think the description in the other settings is no
clear enough, we can edit it.
Jonathan
On 5 Jun 2023, at 19:10, Jonathan S. Katz <jkatz@postgresql.org> wrote:
"It is only supported on systems where TCP_USER_TIMEOUT is available; on other systems, it has no effect."
If this is really only unsupported / has different settings on Windows, I think it's OK to call that out. The original gripe was about readability, but if we think the description in the other settings is no clear enough, we can edit it.
TCP_USER_TIMEOUT is not widely available, AFAIK FreeBSD, OpenBSD and macOS do
not support it yet.
--
Daniel Gustafsson
On 05.06.23 19:10, Jonathan S. Katz wrote:
On 5/31/23 7:53 AM, Daniel Gustafsson wrote:
On 31 May 2023, at 13:16, Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:The first two hunks are pretty straightforward, they just move the
existing text around.For the other two, which are not supported on Windows, I added an
explicit parenthetical note. We don't list which of the Unix-like
platforms support the respective options, but I suspect that it's all
of them in practice? (Otherwise we should be more explicit.) So I
think calling out Windows explicitly is sensible, also considering
that the first two settings are supported on Windows but the latter
two are not.I think this is a clear improvement over the current docs.
+1.
Small nit:
"which does not include Windows" =>
"which is not supported on Windows"(in two places)
The proposed text in the patch is
"This parameter is supported only on systems that {have this property}
(which does not include Windows)."
I don't see how the change you are proposing is correct.
On 6/7/23 11:17 AM, Peter Eisentraut wrote:
The proposed text in the patch is
"This parameter is supported only on systems that {have this property}
(which does not include Windows)."I don't see how the change you are proposing is correct.
I see -- I had read it quickly and didn't sound it out. Nit withdrawn.
Jonathan
On 07.06.23 17:34, Jonathan S. Katz wrote:
On 6/7/23 11:17 AM, Peter Eisentraut wrote:
The proposed text in the patch is
"This parameter is supported only on systems that {have this property}
(which does not include Windows)."I don't see how the change you are proposing is correct.
I see -- I had read it quickly and didn't sound it out. Nit withdrawn.
Ok, I committed it.
I don't have an opinion on whether any changes are necessary on the
libpq side.