Re: [PATCH] force_parallel_mode and GUC categories

On Sat, Apr 03, 2021 at 08:25:46PM -0500, Justin Pryzby wrote:
> Forking this thread
> https://www.postgresql.org/message-id/20210403154336.GG29125%40momjian.us

Didn't see this one, thanks for forking.

> 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

0001 has some changes to pg_config_manual.h related to valgrind and
memory randomization. You may want to remove that before posting a
patch.

- {"track_commit_timestamp", PGC_POSTMASTER, REPLICATION,
+ {"track_commit_timestamp", PGC_POSTMASTER, REPLICATION_SENDING,
I can get behind this change for clarity where it gets actively used.

- {"track_activity_query_size", PGC_POSTMASTER, RESOURCES_MEM,
+ {"track_activity_query_size", PGC_POSTMASTER, STATS_COLLECTOR,
But not this one, because it is a memory setting.

- {"force_parallel_mode", PGC_USERSET, QUERY_TUNING_OTHER,
+ {"force_parallel_mode", PGC_USERSET, DEVELOPER_OPTIONS,
And not this one either, as it is mainly a planner thing, like the
other parameters in the same area.

The last change is related to log_autovacuum_min_duration, and I can
get behind the argument you are making to group all log activity
parameters together. Now, about this part:
+#log_autovacuum_min_duration = -1 # -1 disables, 0 logs all actions and
+ # their durations, > 0 logs only
+ # actions running at least this number
+ # of milliseconds.
I think that we should clarify in the description that this is an
autovacuum-only thing, say by appending a small sentence about the
fact that it logs autovacuum activities, in a similar fashion to
log_temp_files. Moving the parameter out of the autovacuum section
makes it lose a bit of context.

@@ -6903,6 +6903,7 @@ fetch_more_data_begin(AsyncRequest *areq)
char sql[64];

Assert(!fsstate->conn_state->pendingAreq);
+ Assert(fsstate->conn);
What's this diff doing here?
--
Michaelx