Re: Convert node test compile-time settings into run-time parameters
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.
Re: Convert node test compile-time settings into run-time parameters
Peter Eisentraut 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
Re: Convert node test compile-time settings into run-time parameters
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. From 80f35c90e3414dabd879e8832ce1b89c685e5de5 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Fri, 24 May 2024 11:42:02 +0200 Subject: [PATCH v2] Convert node test compile-time settings into run-time parameters This converts 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 compile-time symbols are kept for build farm compatibility, but they now just determine the default value of the run-time settings. Furthermore, support for these settings is not compiled in at all unless assertions are enabled, or the new symbol DEBUG_NODE_TESTS_ENABLED is defined at compile time, or any of the legacy compile-time setting symbols are defined. So there is no run-time overhead in production builds. (This is similar to the handling of DISCARD_CACHES_ENABLED.) Discussion: https://www.postgresql.org/message-id/flat/30747bd8-f51e-4e0c-a310-a6e2c37ec8aa%40eisentraut.org --- .cirrus.tasks.yml | 3 +- doc/src/sgml/config.sgml| 71 + src/backend/nodes/README| 9 ++-- src/backend/nodes/read.c| 12 ++--- src/backend/nodes/readfuncs.c | 4 +- src/backend/parser/analyze.c| 44 ++ src/backend/tcop/postgres.c | 30 +++- src/backend/utils/misc/guc_tables.c | 59 src/include/nodes/nodes.h | 2 +- src/include/nodes/readfuncs.h | 2 +- src/include/pg_config_manual.h | 34 +++--- src/include/utils/guc.h | 6 +++ 12 files changed, 212 insertions(+), 64 deletions(-) diff --git a/.cirrus.tasks.yml b/.cirrus.tasks.yml index a2388cd5036..6a9ff066391 100644 --- a/.cirrus.tasks.yml +++ b/.cirrus.tasks.yml @@ -133,9 +133,10 @@ task: DISK_SIZE: 50 CCACHE_DIR: /tmp/ccache_dir -CPPFLAGS: -DRELCACHE_FORCE_RELEASE -DCOPY_PARSE_PLAN_TREES -DWRITE_READ_PARSE_PLAN_TREES -DRAW_EXPRESSION_COVERAGE_TEST -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS +CPPFLAGS: -DRELCACHE_FORCE_RELEASE -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS CFLAGS: -Og -ggdb +PG_TEST_INITDB_EXTRA_OPTS: -c debug_copy_parse_plan_trees=on -c debug_write_read_parse_plan_trees=on -c debug_raw_expression_coverage_test=on PG_TEST_PG_UPGRADE_MODE: --link <<: *freebsd_task_template diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 698169afdb6..c121a13b4c3 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -11372,6 +11372,29 @@ Developer Options + + debug_copy_parse_plan_trees (boolean) + + debug_copy_parse_plan_trees configuration parameter + + + + +Enabling this forces all parse and plan trees to be passed through +copyObject(), to facilitate catching errors and +omissions in copyObject(). Th
Re: Convert node test compile-time settings into run-time parameters
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
Re: Convert node test compile-time settings into run-time parameters
Em ter., 21 de mai. de 2024 às 09:25, Peter Eisentraut escreveu: > On 20.05.24 15:59, Tom Lane wrote: > > Peter Eisentraut 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
Re: Convert node test compile-time settings into run-time parameters
On 20.05.24 15:59, Tom Lane wrote: Peter Eisentraut 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.
Re: Convert node test compile-time settings into run-time parameters
Peter Eisentraut 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
Re: Convert node test compile-time settings into run-time parameters
Em seg., 20 de mai. de 2024 às 04:28, Peter Eisentraut 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
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?
From 568c620eb97f82aa8eab850cb3ce703c5c94a912 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 20 May 2024 09:13:23 +0200
Subject: [PATCH v1] Convert node test compile-time settings into run-time
parameters
This converts
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 compile-time symbols are kept for build farm compatibility, but
they now just determine the default value of the run-time settings.
TODO: config.sgml documentation
---
.cirrus.tasks.yml | 3 +-
src/backend/nodes/README| 9 ++---
src/backend/nodes/read.c| 15 +---
src/backend/nodes/readfuncs.c | 10 +-
src/backend/parser/analyze.c| 14 +++-
src/backend/tcop/postgres.c | 18 --
src/backend/utils/misc/guc_tables.c | 55 +
src/include/nodes/nodes.h | 2 --
src/include/nodes/readfuncs.h | 2 --
src/include/pg_config_manual.h | 21 ---
src/include/utils/guc.h | 4 +++
11 files changed, 79 insertions(+), 74 deletions(-)
diff --git a/.cirrus.tasks.yml b/.cirrus.tasks.yml
index a2388cd5036..6a9ff066391 100644
--- a/.cirrus.tasks.yml
+++ b/.cirrus.tasks.yml
@@ -133,9 +133,10 @@ task:
DISK_SIZE: 50
CCACHE_DIR: /tmp/ccache_dir
-CPPFLAGS: -DRELCACHE_FORCE_RELEASE -DCOPY_PARSE_PLAN_TREES
-DWRITE_READ_PARSE_PLAN_TREES -DRAW_EXPRESSION_COVERAGE_TEST
-DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
+CPPFLAGS: -DRELCACHE_FORCE_RELEASE
-DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
CFLAGS: -Og -ggdb
+PG_TEST_INITDB_EXTRA_OPTS: -c debug_copy_parse_plan_trees=on -c
debug_write_read_parse_plan_trees=on -c debug_raw_expression_coverage_test=on
PG_TEST_PG_UPGRADE_MODE: --link
<<: *freebsd_task_template
diff --git a/src/backend/nodes/README b/src/backend/nodes/README
index 52364470205..f8bbd605386 100644
--- a/src/backend/nodes/README
+++ b/src/backend/nodes/README
@@ -98,10 +98,11 @@ Suppose you want to define a node Foo:
node types to find all the places to touch.
(Except for frequently-created nodes, don't bother writing a creator
function in makefuncs.c.)
-4. Consider testing your new code with COPY_PARSE_PLAN_TREES,
- WRITE_READ_PARSE_PLAN_TREES, and RAW_EXPRESSION_COVERAGE_TEST to ensure
- support has been added everywhere that it's necessary; see
- pg_config_manual.h about these.
+4. Consider testing your new code with debug_copy_parse_plan_trees,
+ debug_write_read_parse_plan_trees, and
+ debug_raw_expression_coverage_test to ensure support has been added
+ everywhere that it's necessary (e.g., run the tests with
+ PG_TEST_INITDB_EXTRA_OPTS='-c debug_...=on').
Adding a new node type moves the numbers associated with existing
tags, so you'll need to recompile the whole tree after doing this.
diff --git a/src/backend/nodes/read.c b/src/backend/nodes/read.c
index 4eb42445c52..e2d2ce7374d 100644
--- a/src/backend/nodes/read.c
+++ b/src/backend/nodes/read.c
@@ -32,9 +32,7 @@
static const char *pg_strtok_ptr = NULL;
/* State flag that determines how readfuncs.c should treat location fields */
-#ifdef WRITE_READ_PARSE_PLAN_TREES
bool restore_location_fields = false;
-#endif
/*
@@ -42,17 +40,14 @@ boolrestore_location_fields = false;
* builds a Node tree from its string representation (assumed valid)
*
* restore_loc_fields instructs readfuncs.c whether to restore location
- * fields rather than set them to -1. This is currently only supported
- * in builds with the WRITE_READ_PARSE_PLAN_TREES debugging flag set.
+ * fields rather than set them to -1.
*/
static void *
stringToNodeInternal(const char *str, bool restore_loc_fields)
{
void *retval;
const char *save_
