Convert node test compile-time settings into run-time parameters

Em seg., 20 de mai. de 2024 às 04:28, Peter Eisentraut <peter@eisentraut.org>
escreveu:

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?

What is the use for production use?

best regards,
Ranier Vilela

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.

Em ter., 21 de mai. de 2024 às 09:25, Peter Eisentraut <peter@eisentraut.org>
escreveu:

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

If some of this is useful for non-developer users,
it shouldn't be called debug, or in this category.

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.

Although there are some developer users.
I believe that anything that is not useful for common users and is not used
for production
should not be compiled at runtime.

best regards,
Ranier Vilela

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

On 24.05.24 16:39, Tom Lane wrote:

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.

Right. My inclination is to go ahead with the patch as proposed at this
time. There might be other ideas for tweaks in this area, but they
could be applied as new patches on top of this. The main goal here was
to do $subject, and without overhead for production builds, and this
accomplishes that.