Обсуждение: confusing positioning of notes in connection settings
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. While arguably this is not a big deal now, the new deep-linking work for v16[2] 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 [2] https://www.postgresql.org/docs/devel/runtime-config-connection.html#GUC-TCP-KEEPALIVES-COUNT
Вложения
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_KEEPCNTis 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_KEEPCNTis 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 ifTCP_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.
Вложения
> 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 ofthe Unix-like platforms support the respective options, but I suspect that it's all of them in practice? (Otherwise weshould be more explicit.) So I think calling out Windows explicitly is sensible, also considering that the first two settingsare 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 ofthe Unix-like platforms support the respective options, but I suspect that it's all of them in practice? (Otherwise weshould be more explicit.) So I think calling out Windows explicitly is sensible, also considering that the first two settingsare 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 originalgripe was about readability, but if we think the description in the other settings is no clear enough, we can editit. 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.