Обсуждение: Convert node test compile-time settings into run-time parameters

This patch converts the compile-time settings

     COPY_PARSE_PLAN_TREES
     WRITE_READ_PARSE_PLAN_TREES
     RAW_EXPRESSION_COVERAGE_TEST

into run-time parameters

     debug_copy_parse_plan_trees
     debug_write_read_parse_plan_trees
     debug_raw_expression_coverage_test

They can be activated for tests using PG_TEST_INITDB_EXTRA_OPTS.

The effect is the same, but now you don't need to recompile in order to 
use these checks.

The compile-time symbols are kept for build farm compatibility, but they 
now just determine the default value of the run-time settings.

Possible concerns:

- Performance?  Looking for example at pg_parse_query() and its 
siblings, they also check for other debugging settings like 
log_parser_stats in the main code path, so it doesn't seem to be a concern.

- Access control?  I have these settings as PGC_USERSET for now. Maybe 
they should be PGC_SUSET?

Another thought:  Do we really need three separate settings?

Peter Eisentraut <peter@eisentraut.org> writes:
> This patch converts the compile-time settings
>      COPY_PARSE_PLAN_TREES
>      WRITE_READ_PARSE_PLAN_TREES
>      RAW_EXPRESSION_COVERAGE_TEST

> into run-time parameters

>      debug_copy_parse_plan_trees
>      debug_write_read_parse_plan_trees
>      debug_raw_expression_coverage_test

I'm kind of down on this.  It seems like forcing a bunch of
useless-in-production debug support into the standard build.
What of this would be of any use to any non-developer?

            regards, tom lane

On 20.05.24 15:59, Tom Lane wrote:
> Peter Eisentraut <peter@eisentraut.org> writes:
>> This patch converts the compile-time settings
>>       COPY_PARSE_PLAN_TREES
>>       WRITE_READ_PARSE_PLAN_TREES
>>       RAW_EXPRESSION_COVERAGE_TEST
> 
>> into run-time parameters
> 
>>       debug_copy_parse_plan_trees
>>       debug_write_read_parse_plan_trees
>>       debug_raw_expression_coverage_test
> 
> I'm kind of down on this.  It seems like forcing a bunch of
> useless-in-production debug support into the standard build.
> What of this would be of any use to any non-developer?

We have a bunch of other debug_* settings that are available in 
production builds, such as

debug_print_parse
debug_print_rewritten
debug_print_plan
debug_pretty_print
debug_discard_caches
debug_io_direct
debug_parallel_query
debug_logical_replication_streaming

Maybe we could hide all of them behind some #ifdef DEBUG_OPTIONS, but in 
any case, I don't think the ones being proposed here are substantially 
different from those existing ones that they would require a separate 
treatment.

My goal is to make these facilities easier to use, avoiding hand-editing 
pg_config_manual.h and having to recompile.

Hi,

On 2024-05-20 09:28:39 +0200, Peter Eisentraut wrote:
> - Performance?  Looking for example at pg_parse_query() and its siblings,
> they also check for other debugging settings like log_parser_stats in the
> main code path, so it doesn't seem to be a concern.

I don't think we can conclude that. Just because we've not been that careful
about performance in a few spots doesn't mean we shouldn't be careful in other
areas. And I think something like log_parser_stats is a lot more generally
useful than debug_copy_parse_plan_trees.

The branch itself isn't necessarily the issue, the branch predictor can handle
that to a good degree. The reduction in code density is a bigger concern - and
also very hard to measure, because the cost is very incremental and
distributed.

At the very least I'd add unlikely() to all of the branches, so the debug code
can be placed separately from the "normal" portions.


Where I'd be more concerned about peformance is the added branch in
READ_LOCATION_FIELD. There are a lot of calls to that, addding runtime
branches to each, with external function calls inside, is somewhat likely to
be measurable.


> - Access control?  I have these settings as PGC_USERSET for now. Maybe they
> should be PGC_SUSET?

That probably would be right.


> Another thought:  Do we really need three separate settings?

Maybe not three settings, but a single setting, with multiple values, like
debug_io_direct?


Greetings,

Andres Freund

On 21.05.24 20:48, Andres Freund wrote:
> Where I'd be more concerned about peformance is the added branch in
> READ_LOCATION_FIELD. There are a lot of calls to that, addding runtime
> branches to each, with external function calls inside, is somewhat likely to
> be measurable.

Ok, I have an improved plan.  I'm wrapping all the code related to this 
in #ifdef DEBUG_NODE_TESTS_ENABLED.  This in turn is defined in 
assert-enabled builds, or if you define it explicitly, or if you define 
one of the legacy individual symbols.  That way you get the run-time 
settings in a normal development build, but there is no new run-time 
overhead.  This is the same scheme that we use for debug_discard_caches.

(An argument could be made to enable this code if and only if assertions 
are enabled, since these tests are themselves kind of assertions.  But I 
think having a separate symbol documents the purpose of the various code 
sections better.)

>> Another thought:  Do we really need three separate settings?
> 
> Maybe not three settings, but a single setting, with multiple values, like
> debug_io_direct?

Yeah, good idea.  Let's get some more feedback on this before I code up 
a complicated list parser.

Another approach might be levels.  My testing showed that the overhead 
of the copy_parse_plan_trees and raw_expression_coverage_tests flags is 
hardly noticeable, but write_read_parse_plan_trees has some noticeable 
impact.  So you could do 0=off, 1=only the cheap ones, 2=all tests.

In fact, if we could make "only the cheap ones" the default for 
assert-enabled builds, then most people won't even need to worry about 
this setting: The only way to mess up the write_read_parse_plan_trees is 
if you write custom node support, which is rare.  But the raw expression 
coverage still needs to be maintained by hand, so it's more often 
valuable to have it checked automatically.

Peter Eisentraut <peter@eisentraut.org> writes:
> Ok, I have an improved plan.  I'm wrapping all the code related to this 
> in #ifdef DEBUG_NODE_TESTS_ENABLED.  This in turn is defined in 
> assert-enabled builds, or if you define it explicitly, or if you define 
> one of the legacy individual symbols.  That way you get the run-time 
> settings in a normal development build, but there is no new run-time 
> overhead.  This is the same scheme that we use for debug_discard_caches.

+1; this addresses my concern about not adding effectively-dead code
to production builds.  Your point upthread about debug_print_plan and
other legacy debug switches was not without merit; should we also fold
those into this plan?  (In that case we'd need a symbol named more
generically than DEBUG_NODE_TESTS_ENABLED.)

> (An argument could be made to enable this code if and only if assertions 
> are enabled, since these tests are themselves kind of assertions.  But I 
> think having a separate symbol documents the purpose of the various code 
> sections better.)

Agreed.

>> Maybe not three settings, but a single setting, with multiple values, like
>> debug_io_direct?

> Yeah, good idea.  Let's get some more feedback on this before I code up 
> a complicated list parser.

Kinda doubt it's worth the trouble, either to code the GUC support or
to use it.  I don't object to having the booleans in a debug build,
I was just concerned about whether they should exist in production.

            regards, tom lane