Thread: [PATCH] force_parallel_mode and GUC categories

Forking this thread
https://www.postgresql.org/message-id/20210403154336.GG29125%40momjian.us

On Sat, Apr  3, 2021 at 08:38:18PM +0530, aditya desai wrote:
> > > >> Yes, force_parallel_mode is on. Should we set it off?

Bruce Momjian <bruce@momjian.us> writes:
> > > > Yes.  I bet someone set it without reading our docs:
...
> > > > We might need to clarify this sentence to be clearer it is _only_ for
> > > > testing.

On Sat, Apr 03, 2021 at 11:39:19AM -0400, Tom Lane wrote:
> > > I wonder why it is listed under planner options at all, and not under
> > > developer options.

On Sat, Apr  3, 2021 at 10:41:14AM -0500, Justin Pryzby wrote:
> > Because it's there to help DBAs catch errors in functions incorrectly marked as
> > parallel safe.

On Sat, Apr 03, 2021 at 11:43:36AM -0400, Bruce Momjian wrote:
> Uh, isn't that developer/debugging?

I understood "developer" to mean someone who's debugging postgres itself, not
(say) a function written using pl/pgsql.  Like backtrace_functions,
post_auth_delay, jit_profiling_support.

But I see that some "dev" options are more user-facing (for a sufficiently
advanced user):
ignore_checksum_failure, ignore_invalid_pages, zero_damaged_pages.

Also, I understood this to mean the "category" in pg_settings, but I guess
what's important here is the absense of the GUC in the sample/template config
file.  pg_settings.category and the sample headings it appears are intended to
be synchronized, but a few of them are out of sync.  See attached.

+1 to move this to "developer" options and remove it from the sample config:

# - Other Planner Options -
#force_parallel_mode = off

-- 
Justin

The previous patches accidentally included some unrelated changes.

-- 
Justin

On Mon, Apr 12, 2021 at 10:31 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Apr 09, 2021 at 07:39:28AM -0400, Bruce Momjian wrote:
> > On Thu, Apr  8, 2021 at 10:17:18PM -0500, Justin Pryzby wrote:
> >> This is the main motive behind the patch.
> >>
> >> Developer options aren't shown in postgresql.conf.sample, which it seems like
> >> sometimes people read through quickly, setting a whole bunch of options that
> >> sound good, sometimes including this one.  And in the best case they then ask
> >> on -performance why their queries are slow and we tell them to turn it back off
> >> to fix their issues.  This changes to no longer put it in .sample, and calling
> >> it a "dev" option seems to be the classification and mechanism by which to do
> >> that.
> >
> > +1
>
> Hm.  I can see the point you are making based on the bug report that
> has led to this thread:
> https://www.postgresql.org/message-id/CAN0SRDFV=Fv0zXHCGbh7gh=MTfw05Xd1x7gjJrZs5qn-TEphOw@mail.gmail.com
>
> However, I'd like to think that we can do better than what's proposed
> in the patch.  There are a couple of things to consider here:
> - Should the parameter be renamed to reflect that it should only be
> used for testing purposes?
> - Should we make more general the description of the developer options
> in the docs?

IMO, categorizing force_parallel_mode to DEVELOPER_OPTIONS and moving
it to the "Developer Options" section in config.sgml looks
appropriate. So, the v2-0004 patch proposed by Justin at [1] looks
good to me. If there are any other GUCs that are not meant to be used
in production, IMO we could follow the same.

[1] https://www.postgresql.org/message-id/20210408213812.GA18734%40telsasoft.com

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

Michael Paquier <michael@paquier.xyz> writes:
> However, I'd like to think that we can do better than what's proposed
> in the patch.  There are a couple of things to consider here:
> - Should the parameter be renamed to reflect that it should only be
> used for testing purposes?

-1 to that part, because it would break a bunch of buildfarm animals'
configurations.  I doubt that any gain in clarity would be worth it.

> - Should we make more general the description of the developer options
> in the docs?

Perhaps ... what did you have in mind?

            regards, tom lane

Michael Paquier <michael@paquier.xyz> writes:
> On Mon, Apr 12, 2021 at 01:40:52AM -0400, Tom Lane wrote:
>> Perhaps ... what did you have in mind?

> The first sentence of the page now says that:
> "The following parameters are intended for work on the PostgreSQL
> source code, and in some cases to assist with recovery of severely
> damaged databases."

> That does not stick with force_parallel_mode IMO.  Maybe:
> "The following parameters are intended for development work related to
> PostgreSQL.  Some of them work on the PostgreSQL source code, some of
> them can be used to control the run-time behavior of the server, and
> in some cases they can be used to assist with the recovery of severely
> damaged databases."

I think that's overly wordy.  Maybe

The following parameters are intended for developer testing, and
should never be enabled for production work.  However, some of
them can be used to assist with the recovery of severely
damaged databases.

            regards, tom lane

Justin Pryzby <pryzby@telsasoft.com> writes:
> ...but, I realized just now that *zero* other GUCs use "REPLICATION".
> And the documentation puts it in 20.6.1. Sending Servers,
> so it still seems to me that this is correct to move this, too.
> https://www.postgresql.org/docs/devel/runtime-config-replication.html
> Then, I wonder if REPLICATION should be removed from guc_tables.h...

For the archives' sake --- these things are now committed as part of
a55a98477.  I'd forgotten this thread, and then rediscovered the same
inconsistencies as Justin had while reviewing Bharath Rupireddy's patch
for bug #16997 [1].

I think this thread can now be closed off as done.  However, there
are some open issues mentioned in the other thread, if anyone here
wants to comment.

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/16997-ff16127f6e0d1390%40postgresql.org