Обсуждение: BUG #16972: parameter parallel_leader_participation's category problem

The following bug has been logged on the website:

Bug reference:      16972
Logged by:          yanliang lei
Email address:      leiyanliang@highgo.com
PostgreSQL version: 13.1
Operating system:   CentOS7.6
Description:

In the following SQL statement, parallel_leader_participation's category is
' Resource Usage / Asynchronous Behavior'

postgres=# select * from pg_settings where
name='parallel_leader_participation';
-[ RECORD 1
]---+--------------------------------------------------------------
name            | parallel_leader_participation
setting         | on
unit            | 
category        | Resource Usage / Asynchronous Behavior
short_desc      | Controls whether Gather and Gather Merge also run
subplans.
extra_desc      | Should gather nodes also run subplans, or just gather
tuples?
context         | user
vartype         | bool
source          | default
min_val         | 
max_val         | 
enumvals        | 
boot_val        | on
reset_val       | on
sourcefile      | 
sourceline      | 
pending_restart | f

postgres=# 

but in the documents
(https://www.postgresql.org/docs/13/runtime-config-query.html#RUNTIME-CONFIG-QUERY-OTHER)
parallel_leader_participation's category is 'Query Tuning / Other Planner
Options' 
and in the category "Asynchronous
Behavior"(https://www.postgresql.org/docs/13/runtime-config-resource.html#RUNTIME-CONFIG-RESOURCE-ASYNC-BEHAVIOR),there
is no parallel_leader_participation parameter.

so, what is  parallel_leader_participation's category ?

On Tue, Apr 20, 2021 at 12:54 PM PG Bug reporting form
<noreply@postgresql.org> wrote:
>
> The following bug has been logged on the website:
>
> Bug reference:      16972
> Logged by:          yanliang lei
> Email address:      leiyanliang@highgo.com
> PostgreSQL version: 13.1
> Operating system:   CentOS7.6
> Description:
>
> In the following SQL statement, parallel_leader_participation's category is
> ' Resource Usage / Asynchronous Behavior'
>
> postgres=# select * from pg_settings where
> name='parallel_leader_participation';
> -[ RECORD 1
> ]---+--------------------------------------------------------------
> name            | parallel_leader_participation
> setting         | on
> unit            |
> category        | Resource Usage / Asynchronous Behavior
> short_desc      | Controls whether Gather and Gather Merge also run
> subplans.
> extra_desc      | Should gather nodes also run subplans, or just gather
> tuples?
> context         | user
> vartype         | bool
> source          | default
> min_val         |
> max_val         |
> enumvals        |
> boot_val        | on
> reset_val       | on
> sourcefile      |
> sourceline      |
> pending_restart | f
>
> postgres=#
>
> but in the documents
> (https://www.postgresql.org/docs/13/runtime-config-query.html#RUNTIME-CONFIG-QUERY-OTHER)
> parallel_leader_participation's category is 'Query Tuning / Other Planner
> Options'
> and in the category "Asynchronous
>
Behavior"(https://www.postgresql.org/docs/13/runtime-config-resource.html#RUNTIME-CONFIG-RESOURCE-ASYNC-BEHAVIOR),there
> is no parallel_leader_participation parameter.
>
> so, what is  parallel_leader_participation's category ?

Thanks for reporting.

I think it comes under the category "Resource Usage / Asynchronous
Behavior", so what pg_settings showing is correct. ISTM that we need
to correct the docs, attached a patch for that.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Apr 20, 2021 at 01:38:48PM +0530, Bharath Rupireddy wrote:
> I think it comes under the category "Resource Usage / Asynchronous
> Behavior", so what pg_settings showing is correct. ISTM that we need
> to correct the docs, attached a patch for that.

This got introduced in e5253fd.  My first impression was that this
had better be a developer option, but Thomas is mentioning an extra
reason why this category may not be a good fit:
https://www.postgresql.org/message-id/CAEepm=3G1-SKd2qKN-3uen=Xvyi-OxAVg9RAwqWDH-KZWuGqNA@mail.gmail.com

And Robert has moved that at the end to its GUC section without
addressing the docs:
https://www.postgresql.org/message-id/CA+Tgmoa=AxuLKo2f1AKwLmvFoo3rr3j+SYHaJ5GVyx1PhhOJ0Q@mail.gmail.com

So I agree that your patch is adapted, even postgresql.conf.sample
gets that right.  Something that your patch makes worse is the
alphabetical order of the parameters listed in this section
(backend_flush_after can be also blamed here), so I'll go reorder this
sub-area a bit while on it, except if somebody objects.
--
Michael

On Wed, Apr 21, 2021 at 8:16 AM Michael Paquier <michael@paquier.xyz> wrote:
> So I agree that your patch is adapted, even postgresql.conf.sample
> gets that right.  Something that your patch makes worse is the
> alphabetical order of the parameters listed in this section
> (backend_flush_after can be also blamed here), so I'll go reorder this
> sub-area a bit while on it, except if somebody objects.

If we arrange only the "Asynchronous Behaviour" subsection in
alphabetical order, I think the order may not be maintained in case of
new GUCs that may get added there. Because all the other subsections
are unordered and there's no note of maintaining the order as such.
And, it looks like the relevant GUCs are grouped for better
readability. For instance, all "parallelism", "io_concurrency", "jit_"
related GUCs are together. Developers tend to add the new GUCs in
relevant areas.

So, -1 for reordering.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Apr 21, 2021 at 09:15:23AM +0530, Bharath Rupireddy wrote:
> If we arrange only the "Asynchronous Behaviour" subsection in
> alphabetical order, I think the order may not be maintained in case of
> new GUCs that may get added there. Because all the other subsections
> are unordered and there's no note of maintaining the order as such.
> And, it looks like the relevant GUCs are grouped for better
> readability. For instance, all "parallelism", "io_concurrency", "jit_"
> related GUCs are together. Developers tend to add the new GUCs in
> relevant areas.

That's up to the committers adding them to be careful, but I of course
agree that the context is important.  IMV, we can do a slightly better
organization in "Asynchronous Behaviour".  First, backend_flush_after
is independent, and could just be first.

parallel_leader_participation can also be moved after
max_parallel_workers without impacting the readability nor impacting
the set of parallel-ish parameters grouped together.
--
Michael

On Wed, Apr 21, 2021 at 4:15 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Apr 21, 2021 at 09:15:23AM +0530, Bharath Rupireddy wrote:
> > If we arrange only the "Asynchronous Behaviour" subsection in
> > alphabetical order, I think the order may not be maintained in case of
> > new GUCs that may get added there. Because all the other subsections
> > are unordered and there's no note of maintaining the order as such.
> > And, it looks like the relevant GUCs are grouped for better
> > readability. For instance, all "parallelism", "io_concurrency", "jit_"
> > related GUCs are together. Developers tend to add the new GUCs in
> > relevant areas.
>
> That's up to the committers adding them to be careful, but I of course
> agree that the context is important.  IMV, we can do a slightly better
> organization in "Asynchronous Behaviour".  First, backend_flush_after
> is independent, and could just be first.
>
> parallel_leader_participation can also be moved after
> max_parallel_workers without impacting the readability nor impacting
> the set of parallel-ish parameters grouped together.

+1. Attached v2.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Apr 21, 2021 at 05:07:37PM +0530, Bharath Rupireddy wrote:
> +1. Attached v2.

This patch broke the compilation of the docs as you moved the
<varlistentry> for backend_flush_after before <variablelist>.  Fixed
that, then applied the patch.

Thanks,
--
Michael

On Thu, Apr 22, 2021 at 06:32:57AM +0530, Bharath Rupireddy wrote:
> Thanks for the correction! I never compiled doc changes, I will ensure to
> do so from now on.

That may help:
https://www.postgresql.org/docs/devel/docguide-toolsets.html

It is worth noting that when doing patches for stable branches, the
set of dependencies for ~10 is different than what's used for
11~HEAD.
--
Michael

On Thu, Apr 22, 2021 at 10:30 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Apr 22, 2021 at 06:32:57AM +0530, Bharath Rupireddy wrote:
> > Thanks for the correction! I never compiled doc changes, I will ensure to
> > do so from now on.
>
> That may help:
> https://www.postgresql.org/docs/devel/docguide-toolsets.html

Thanks. I was able to install the required packages on my dev system,
ran "make check" on the docs and saw the error that was happening on
v2 patch.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com