Обсуждение: Can we do something to help stop users mistakenly using force_parallel_mode?

Over on [1] I noticed that the user had set force_parallel_mode to
"on" in the hope that would trick the planner into making their query
run more quickly.  Of course, that's not what they want since that GUC
is only there to inject some parallel nodes into the plan in order to
verify the tuple communication works.

I get the idea that Robert might have copped some flak about this at
some point, given that he wrote the blog post at [2].

The user would have realised this if they'd read the documentation
about the GUC. However, I imagine they only went as far as finding a
GUC with a name which appears to be exactly what they need.  I mean,
what else could force_parallel_mode possibly do?

Should we maybe rename it to something less tempting? Maybe
debug_parallel_query?

I wonder if \dconfig *parallel* is going to make force_parallel_mode
even easier to find once PG15 is out.

David

[1]
https://www.postgresql.org/message-id/DB4PR02MB8774E06D595D3088BE04ED92E7B99%40DB4PR02MB8774.eurprd02.prod.outlook.com
[2] https://www.enterprisedb.com/postgres-tutorials/using-forceparallelmode-correctly-postgresql

On Wed, 2022-06-29 at 15:23 +1200, David Rowley wrote:
> Over on [1] I noticed that the user had set force_parallel_mode to
> "on" in the hope that would trick the planner into making their query
> run more quickly.  Of course, that's not what they want since that GUC
> is only there to inject some parallel nodes into the plan in order to
> verify the tuple communication works.
> 
> I get the idea that Robert might have copped some flak about this at
> some point, given that he wrote the blog post at [2].
> 
> The user would have realised this if they'd read the documentation
> about the GUC. However, I imagine they only went as far as finding a
> GUC with a name which appears to be exactly what they need.  I mean,
> what else could force_parallel_mode possibly do?
> 
> Should we maybe rename it to something less tempting? Maybe
> debug_parallel_query?
> 
> I wonder if \dconfig *parallel* is going to make force_parallel_mode
> even easier to find once PG15 is out.
> 
> [1]
https://www.postgresql.org/message-id/DB4PR02MB8774E06D595D3088BE04ED92E7B99%40DB4PR02MB8774.eurprd02.prod.outlook.com
> [2] https://www.enterprisedb.com/postgres-tutorials/using-forceparallelmode-correctly-postgresql

I share the sentiment, but at the same time am worried about an unnecessary
compatibility break.  The parameter is not in "postgresql.conf" and
documented as a "developer option", which should already be warning enough.

Perhaps some stronger wording in the documetation would be beneficial.
I have little sympathy with people who set unusual parameters without
even glancing at the documentation.

Yours,
Laurenz Albe

On Wed, Jun 29, 2022 at 03:23:27PM +1200, David Rowley wrote:
> Over on [1] I noticed that the user had set force_parallel_mode to
> "on" in the hope that would trick the planner into making their query
> run more quickly.  Of course, that's not what they want since that GUC
> is only there to inject some parallel nodes into the plan in order to
> verify the tuple communication works.
> 
> I get the idea that Robert might have copped some flak about this at
> some point, given that he wrote the blog post at [2].
> 
> The user would have realised this if they'd read the documentation
> about the GUC. However, I imagine they only went as far as finding a
> GUC with a name which appears to be exactly what they need.  I mean,
> what else could force_parallel_mode possibly do?

Note that it was already changed to be a developer GUC
https://www.postgresql.org/message-id/20210404012546.GK6592%40telsasoft.com

And I asked if that re-classification should be backpatched:
> It's to their benefit and ours if they don't do that on v10-13 for the next 5
> years, not just v14-17.

Since the user in this recent thread is running v13.7, I'm *guessing* that
if that had been backpatched, they wouldn't have made this mistake.

> I wonder if \dconfig *parallel* is going to make force_parallel_mode
> even easier to find once PG15 is out.

Maybe.  Another consequence is that if someone *does* set f_p_m, it may be a
bit easier and more likely for a local admin to discover it (before mailing the
pgsql lists).

-- 
Justin

On Thu, 30 Jun 2022 at 00:31, Justin Pryzby <pryzby@telsasoft.com> wrote:
> Since the user in this recent thread is running v13.7, I'm *guessing* that
> if that had been backpatched, they wouldn't have made this mistake.

I wasn't aware of that change.  Thanks for highlighting it.

Maybe it's worth seeing if fewer mistakes are made now that we've
changed the GUC into a developer option.

David

On Wed, 29 Jun 2022 at 18:57, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
> Perhaps some stronger wording in the documetation would be beneficial.
> I have little sympathy with people who set unusual parameters without
> even glancing at the documentation.

My thoughts are that the documentation is ok as is. I have a feeling
the misusages come from stumbling upon a GUC that has a name which
seems to indicate the GUC does exactly what they want.

David

On Thu, 30 Jun 2022 at 00:31, Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Wed, Jun 29, 2022 at 03:23:27PM +1200, David Rowley wrote:
> > Over on [1] I noticed that the user had set force_parallel_mode to
> > "on" in the hope that would trick the planner into making their query
> > run more quickly.  Of course, that's not what they want since that GUC
> > is only there to inject some parallel nodes into the plan in order to
> > verify the tuple communication works.

> Note that it was already changed to be a developer GUC
> https://www.postgresql.org/message-id/20210404012546.GK6592%40telsasoft.com

> Since the user in this recent thread is running v13.7, I'm *guessing* that
> if that had been backpatched, they wouldn't have made this mistake.

I was just reading [1] where a PG15 user made this mistake, so it
seems people are still falling for it even now it's been changed to a
developer GUC.

I don't really share Laurenz's worry [2] about compatibility break
from renaming this GUC. I think the legitimate usages of this setting
are probably far more rare than the illegitimate ones. I'm not overly
concerned about renaming if it helps stop people from making this
mistake. I believe the current name is just too conveniently named and
that users are likely just to incorrectly assume it does exactly what
they want because what else could it possibly do?!

I think something like debug_parallel_query is much less likely to be misused.

David

[1] https://www.postgresql.org/message-id/CAN4ko3B4y75pg5Ro_oAjWf8L1HYSYgXcDgsS6nzOTvQOkKnM1Q@mail.gmail.com
[2] https://www.postgresql.org/message-id/26139c03e118bec967c77da374d947e9ecf81333.camel@cybertec.at

On Thu, 2 Feb 2023 at 01:24, John Naylor <john.naylor@enterprisedb.com> wrote:
>
>
> On Wed, Feb 1, 2023 at 6:41 PM David Rowley <dgrowleyml@gmail.com> wrote:
> >
> > I don't really share Laurenz's worry [2] about compatibility break
> > from renaming this GUC. I think the legitimate usages of this setting
> > are probably far more rare than the illegitimate ones. I'm not overly
> > concerned about renaming if it helps stop people from making this
> > mistake. I believe the current name is just too conveniently named and
> > that users are likely just to incorrectly assume it does exactly what
> > they want because what else could it possibly do?!
> >
> > I think something like debug_parallel_query is much less likely to be misused.
>
> +1 on both points.

I've attached a patch which does the renaming to debug_parallel_query.
I've made it so the old name can still be used.  This is only intended
to temporarily allow backward compatibility until buildfarm member
owners can change their configs to use debug_parallel_query instead of
force_parallel_mode.

David

David Rowley <dgrowleyml@gmail.com> writes:
> I've attached a patch which does the renaming to debug_parallel_query.
> I've made it so the old name can still be used.

There's a better way to do that last, which is to add the translation to
map_old_guc_names[].  I am not very sure what happens if you have multiple
GUC entries pointing at the same underlying variable, but I bet that
it isn't great.

            regards, tom lane

On Thu, 9 Feb 2023 at 11:26, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > I've attached a patch which does the renaming to debug_parallel_query.
> > I've made it so the old name can still be used.
>
> There's a better way to do that last, which is to add the translation to
> map_old_guc_names[].  I am not very sure what happens if you have multiple
> GUC entries pointing at the same underlying variable, but I bet that
> it isn't great.

Thanks for pointing that out. That might mean we can keep the
translation long-term as it won't appear in pg_settings and \dconfig,
or we might want to remove it if we want to be more deliberate about
breaking things for users who are misusing it. We maybe could just
consider that if/when all buildfarm animals are all using the new
name.

Attached updated patch.

David

On Thu, 9 Feb 2023 at 21:20, John Naylor <john.naylor@enterprisedb.com> wrote:
> Looks good at a glance, just found a spurious word:
>
> + "by forcing the planner into to generate plans which contains nodes "

Thanks for looking. I'll fix that.

Likely the hardest part to get right here is the new name.  Can anyone
think of anything better than debug_parallel_query?

David

On Thu, 9 Feb 2023 at 21:20, John Naylor <john.naylor@enterprisedb.com> wrote:

Looks good at a glance, just found a spurious word:

+ "by forcing the planner into to generate plans which contains nodes "

Thanks for looking. I'll fix that.

Likely the hardest part to get right here is the new name.  Can anyone
think of anything better than debug_parallel_query?

WFM

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Sat, 11 Feb 2023 at 04:34, Andrew Dunstan <andrew@dunslane.net> wrote:
>
> On 2023-02-09 Th 15:25, David Rowley wrote:
> Likely the hardest part to get right here is the new name.  Can anyone
> think of anything better than debug_parallel_query?
>
>
> WFM

Thanks for chipping in.

I've attached a patch which fixes the problem John mentioned.

I feel like nobody is against doing this rename, so I'd quite like to
get this done pretty soon.  If anyone else wants to voice their
opinion in regards to this, please feel free to do so. +1s are more
reassuring than silence.  I just want to get this right so we never
have to think about it again.

If nobody is against this then I'd like to push the attached in about
24 hours from now.

David

On Sat, 11 Feb 2023 at 04:34, Andrew Dunstan <andrew@dunslane.net> wrote:

On 2023-02-09 Th 15:25, David Rowley wrote:
Likely the hardest part to get right here is the new name.  Can anyone
think of anything better than debug_parallel_query?


WFM

Thanks for chipping in.

I've attached a patch which fixes the problem John mentioned.

I feel like nobody is against doing this rename, so I'd quite like to
get this done pretty soon.  If anyone else wants to voice their
opinion in regards to this, please feel free to do so. +1s are more
reassuring than silence.  I just want to get this right so we never
have to think about it again.

If nobody is against this then I'd like to push the attached in about
24 hours from now.

It's just occurred to me that this could break the buildfarm fairly comprehensively. I just took a count and we have 74 members using force_parallel_mode. Maybe we need to keep force_parallel_mode as an alternative spelling for debug_parallel_query until we can get them all switched over. I know it's more trouble ...

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Wed, 15 Feb 2023 at 11:27, Andrew Dunstan <andrew@dunslane.net> wrote:
> It's just occurred to me that this could break the buildfarm fairly comprehensively. I just took a count and we have
74members using force_parallel_mode. Maybe we need to keep force_parallel_mode as an alternative spelling for
debug_parallel_queryuntil we can get them all switched over. I know it's more trouble ... 

Yeah, I mentioned in [1] about that and took measures there to keep
the old name in place.  In the latest patch, there's an entry in
map_old_guc_names[] to allow the old name to work. I think the
buildfarm will still work ok because of that.

What I'm not so sure about is how to go about getting all owners to
change the config for versions >= PG16. Is that a question of emailing
each owner individually to ask them if they can make the change? Or
should we just forever keep the map_old_guc_names[] entry for this?

David

[1] https://postgr.es/m/CAApHDvrT8eq0UwgetGtQE7XLC8HFN8weqembtvYxMVgtWbcnjQ@mail.gmail.com

On Wed, 15 Feb 2023 at 11:27, Andrew Dunstan <andrew@dunslane.net> wrote:

It's just occurred to me that this could break the buildfarm fairly comprehensively. I just took a count and we have 74 members using force_parallel_mode. Maybe we need to keep force_parallel_mode as an alternative spelling for debug_parallel_query until we can get them all switched over. I know it's more trouble ...

Yeah, I mentioned in [1] about that and took measures there to keep
the old name in place.  In the latest patch, there's an entry in
map_old_guc_names[] to allow the old name to work. I think the
buildfarm will still work ok because of that.

Oops, I missed or forgot that.

What I'm not so sure about is how to go about getting all owners to
change the config for versions >= PG16. Is that a question of emailing
each owner individually to ask them if they can make the change? Or
should we just forever keep the map_old_guc_names[] entry for this?

We'll email them once this is in. Most people are fairly reponsive.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Wed, 15 Feb 2023 at 14:10, Andrew Dunstan <andrew@dunslane.net> wrote:
> We'll email them once this is in. Most people are fairly reponsive.

I pushed the rename patch earlier.

How should we go about making contact with the owners?  I'm thinking
it may be better coming from you, especially if you think technical
details of what exactly should be changed should be included in the
email. But I can certainly have a go if you'd rather I did it or you
don't have time for this.

Thanks

David

On Wed, 15 Feb 2023 at 14:10, Andrew Dunstan <andrew@dunslane.net> wrote:

We'll email them once this is in. Most people are fairly reponsive.

I pushed the rename patch earlier.

How should we go about making contact with the owners?  I'm thinking
it may be better coming from you, especially if you think technical
details of what exactly should be changed should be included in the
email. But I can certainly have a go if you'd rather I did it or you
don't have time for this.

Leave it with me.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Thu, 16 Feb 2023 at 00:05, David Rowley <dgrowleyml@gmail.com> wrote:
> I pushed the rename patch earlier.
>
> How should we go about making contact with the owners?

After a quick round of making direct contact with the few remaining
buildfarm machine owners which are still using force_parallel_mode,
we're now down to just one remainder (warbler).

In preparation for when that's ticked off, I'd like to gather people's
thoughts about if we should remove force_parallel_mode from v16?

My thoughts are that providing we can get the remaining animal off it
and remove the GUC alias before beta1, we should remove it.  Renaming
the GUC to debug_parallel_query was entirely aimed at breaking things
for people who are (mistakenly) using it, so I don't see why we
shouldn't remove it.

Does anyone feel differently?

David

David Rowley <dgrowleyml@gmail.com> writes:
> In preparation for when that's ticked off, I'd like to gather people's
> thoughts about if we should remove force_parallel_mode from v16?

To clarify, you just mean removing that alias, right?  +1.
I don't see a reason to wait longer once the buildfarm is on board.

            regards, tom lane

On Wed, 12 Apr 2023 at 09:53, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > In preparation for when that's ticked off, I'd like to gather people's
> > thoughts about if we should remove force_parallel_mode from v16?
>
> To clarify, you just mean removing that alias, right?  +1.
> I don't see a reason to wait longer once the buildfarm is on board.

Yip, alias. i.e:

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index ea67cfa5e5..7d3b20168a 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -186,7 +186,6 @@ static const unit_conversion time_unit_conversion_table[] =
 static const char *const map_old_guc_names[] = {
        "sort_mem", "work_mem",
        "vacuum_mem", "maintenance_work_mem",
-       "force_parallel_mode", "debug_parallel_query",
        NULL
 };

David

On Wed, 12 Apr 2023 at 09:53, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I don't see a reason to wait longer once the buildfarm is on board.

I did a final sweep of the latest runs for each animal this morning.
Everything has been switched over to debug_parallel_query, so I've
gone and pushed the patch to remove the mapping.

For the record, the only things I see mentioning force_parallel_mode
in there are:

hoverfly
      <td>force_parallel_mode; RANDOMIZE_ALLOCATED_MEMORY</td>
mantid
      <td>force_parallel_mode on REL_10_STABLE and later</td>

'force_parallel_mode = regress'

'force_parallel_mode = regress'

'force_parallel_mode = regress'

'force_parallel_mode = regress'

'force_parallel_mode = regress'
mandrill
      <td>force_parallel_mode; RANDOMIZE_ALLOCATED_MEMORY</td>
seawasp
<a href="https://git.postgresql.org/gitweb?p=postgresql.git;a=commitdiff;h=98a88bc2bc">98a88bc2bc</a>
Thu Mar 2 22:47:20 2023 UTC  Harden new test case against
force_parallel_mode = regress.
<a href="https://git.postgresql.org/gitweb?p=postgresql.git;a=commitdiff;h=5352ca22e0">5352ca22e0</a>
Wed Feb 15 08:21:59 2023 UTC  Rename force_parallel_mode to
debug_parallel_query

seawasp's is just references to older commits.  The rest seem like
just outdated comments.

David