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?
Attachments:
v1-0001-Convert-node-test-compile-time-settings-into-run-.patchtext/plain; charset=UTF-8; name=v1-0001-Convert-node-test-compile-time-settings-into-run-.patchDownload
From 568c620eb97f82aa8eab850cb3ce703c5c94a912 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
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 @@ bool restore_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_strtok;
-#ifdef WRITE_READ_PARSE_PLAN_TREES
bool save_restore_location_fields;
-#endif
/*
* We save and restore the pre-existing state of pg_strtok. This makes the
@@ -67,18 +62,14 @@ stringToNodeInternal(const char *str, bool restore_loc_fields)
/*
* If enabled, likewise save/restore the location field handling flag.
*/
-#ifdef WRITE_READ_PARSE_PLAN_TREES
save_restore_location_fields = restore_location_fields;
restore_location_fields = restore_loc_fields;
-#endif
retval = nodeRead(NULL, 0); /* do the reading */
pg_strtok_ptr = save_strtok;
-#ifdef WRITE_READ_PARSE_PLAN_TREES
restore_location_fields = save_restore_location_fields;
-#endif
return retval;
}
@@ -92,16 +83,12 @@ stringToNode(const char *str)
return stringToNodeInternal(str, false);
}
-#ifdef WRITE_READ_PARSE_PLAN_TREES
-
void *
stringToNodeWithLocations(const char *str)
{
return stringToNodeInternal(str, true);
}
-#endif
-
/*****************************************************************************
*
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index c4d01a441a0..32d6b1b2aba 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -19,7 +19,7 @@
*
* However, if restore_location_fields is true, we do restore location
* fields from the string. This is currently intended only for use by the
- * WRITE_READ_PARSE_PLAN_TREES test code, which doesn't want to cause
+ * debug_write_read_parse_plan_trees test code, which doesn't want to cause
* any change in the node contents.
*
*-------------------------------------------------------------------------
@@ -118,18 +118,10 @@
local_node->fldname = nullable_string(token, length)
/* Read a parse location field (and possibly throw away the value) */
-#ifdef WRITE_READ_PARSE_PLAN_TREES
#define READ_LOCATION_FIELD(fldname) \
token = pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
local_node->fldname = restore_location_fields ? atoi(token) : -1
-#else
-#define READ_LOCATION_FIELD(fldname) \
- token = pg_strtok(&length); /* skip :fldname */ \
- token = pg_strtok(&length); /* get field value */ \
- (void) token; /* in case not used elsewhere */ \
- local_node->fldname = -1 /* set field to "unknown" */
-#endif
/* Read a Node field */
#define READ_NODE_FIELD(fldname) \
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 28fed9d87f6..4b0f3c92c0a 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -50,6 +50,7 @@
#include "parser/parsetree.h"
#include "utils/backend_status.h"
#include "utils/builtins.h"
+#include "utils/guc.h"
#include "utils/rel.h"
#include "utils/syscache.h"
@@ -84,9 +85,7 @@ static Query *transformCallStmt(ParseState *pstate,
CallStmt *stmt);
static void transformLockingClause(ParseState *pstate, Query *qry,
LockingClause *lc, bool pushedDown);
-#ifdef RAW_EXPRESSION_COVERAGE_TEST
static bool test_raw_expression_coverage(Node *node, void *context);
-#endif
/*
@@ -313,11 +312,12 @@ transformStmt(ParseState *pstate, Node *parseTree)
Query *result;
/*
- * We apply RAW_EXPRESSION_COVERAGE_TEST testing to basic DML statements;
+ * We apply debug_raw_expression_coverage_test testing to basic DML statements;
* we can't just run it on everything because raw_expression_tree_walker()
* doesn't claim to handle utility statements.
*/
-#ifdef RAW_EXPRESSION_COVERAGE_TEST
+ if (Debug_raw_expression_coverage_test)
+ {
switch (nodeTag(parseTree))
{
case T_SelectStmt:
@@ -330,7 +330,7 @@ transformStmt(ParseState *pstate, Node *parseTree)
default:
break;
}
-#endif /* RAW_EXPRESSION_COVERAGE_TEST */
+ }
/*
* Caution: when changing the set of statement types that have non-default
@@ -3583,8 +3583,6 @@ applyLockingClause(Query *qry, Index rtindex,
* applied in limited cases involving CTEs, and we don't really want to have
* to test everything inside as well as outside a CTE.
*/
-#ifdef RAW_EXPRESSION_COVERAGE_TEST
-
static bool
test_raw_expression_coverage(Node *node, void *context)
{
@@ -3594,5 +3592,3 @@ test_raw_expression_coverage(Node *node, void *context)
test_raw_expression_coverage,
context);
}
-
-#endif /* RAW_EXPRESSION_COVERAGE_TEST */
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 45a3794b8e3..dac04004e26 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -622,8 +622,8 @@ pg_parse_query(const char *query_string)
if (log_parser_stats)
ShowUsage("PARSER STATISTICS");
-#ifdef COPY_PARSE_PLAN_TREES
/* Optional debugging check: pass raw parsetrees through copyObject() */
+ if (Debug_copy_parse_plan_trees)
{
List *new_list = copyObject(raw_parsetree_list);
@@ -633,13 +633,12 @@ pg_parse_query(const char *query_string)
else
raw_parsetree_list = new_list;
}
-#endif
/*
* Optional debugging check: pass raw parsetrees through
* outfuncs/readfuncs
*/
-#ifdef WRITE_READ_PARSE_PLAN_TREES
+ if (Debug_write_read_parse_plan_trees)
{
char *str = nodeToStringWithLocations(raw_parsetree_list);
List *new_list = stringToNodeWithLocations(str);
@@ -651,7 +650,6 @@ pg_parse_query(const char *query_string)
else
raw_parsetree_list = new_list;
}
-#endif
TRACE_POSTGRESQL_QUERY_PARSE_DONE(query_string);
@@ -826,8 +824,8 @@ pg_rewrite_query(Query *query)
if (log_parser_stats)
ShowUsage("REWRITER STATISTICS");
-#ifdef COPY_PARSE_PLAN_TREES
/* Optional debugging check: pass querytree through copyObject() */
+ if (Debug_copy_parse_plan_trees)
{
List *new_list;
@@ -838,10 +836,9 @@ pg_rewrite_query(Query *query)
else
querytree_list = new_list;
}
-#endif
-#ifdef WRITE_READ_PARSE_PLAN_TREES
/* Optional debugging check: pass querytree through outfuncs/readfuncs */
+ if (Debug_write_read_parse_plan_trees)
{
List *new_list = NIL;
ListCell *lc;
@@ -868,7 +865,6 @@ pg_rewrite_query(Query *query)
else
querytree_list = new_list;
}
-#endif
if (Debug_print_rewritten)
elog_node_display(LOG, "rewritten parse tree", querytree_list,
@@ -906,8 +902,8 @@ pg_plan_query(Query *querytree, const char *query_string, int cursorOptions,
if (log_planner_stats)
ShowUsage("PLANNER STATISTICS");
-#ifdef COPY_PARSE_PLAN_TREES
/* Optional debugging check: pass plan tree through copyObject() */
+ if (Debug_copy_parse_plan_trees)
{
PlannedStmt *new_plan = copyObject(plan);
@@ -923,10 +919,9 @@ pg_plan_query(Query *querytree, const char *query_string, int cursorOptions,
#endif
plan = new_plan;
}
-#endif
-#ifdef WRITE_READ_PARSE_PLAN_TREES
/* Optional debugging check: pass plan tree through outfuncs/readfuncs */
+ if (Debug_write_read_parse_plan_trees)
{
char *str;
PlannedStmt *new_plan;
@@ -947,7 +942,6 @@ pg_plan_query(Query *querytree, const char *query_string, int cursorOptions,
#endif
plan = new_plan;
}
-#endif
/*
* Print plan if debugging.
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 46c258be282..82ddb3db730 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -502,6 +502,10 @@ bool Debug_print_parse = false;
bool Debug_print_rewritten = false;
bool Debug_pretty_print = true;
+bool Debug_copy_parse_plan_trees;
+bool Debug_write_read_parse_plan_trees;
+bool Debug_raw_expression_coverage_test;
+
bool log_parser_stats = false;
bool log_planner_stats = false;
bool log_executor_stats = false;
@@ -1301,6 +1305,57 @@ struct config_bool ConfigureNamesBool[] =
false,
NULL, NULL, NULL
},
+ {
+ {"debug_copy_parse_plan_trees", PGC_USERSET, DEVELOPER_OPTIONS,
+ gettext_noop("Set this to force all parse and plan trees to be passed through "
+ "copyObject(), to facilitate catching errors and omissions in "
+ "copyObject()."),
+ NULL,
+ GUC_NOT_IN_SAMPLE
+ },
+ &Debug_copy_parse_plan_trees,
+/* support for legacy compile-time setting */
+#ifdef COPY_PARSE_PLAN_TREES
+ true,
+#else
+ false,
+#endif
+ NULL, NULL, NULL
+ },
+ {
+ {"debug_write_read_parse_plan_trees", PGC_USERSET, DEVELOPER_OPTIONS,
+ gettext_noop("Set this to force all parse and plan trees to be passed through "
+ "outfuncs.c/readfuncs.c, to facilitate catching errors and omissions in "
+ "those modules."),
+ NULL,
+ GUC_NOT_IN_SAMPLE
+ },
+ &Debug_write_read_parse_plan_trees,
+/* support for legacy compile-time setting */
+#ifdef WRITE_READ_PARSE_PLAN_TREES
+ true,
+#else
+ false,
+#endif
+ NULL, NULL, NULL
+ },
+ {
+ {"debug_raw_expression_coverage_test", PGC_USERSET, DEVELOPER_OPTIONS,
+ gettext_noop("Set this to force all raw parse trees for DML statements to be scanned "
+ "by raw_expression_tree_walker(), to facilitate catching errors and "
+ "omissions in that function."),
+ NULL,
+ GUC_NOT_IN_SAMPLE
+ },
+ &Debug_raw_expression_coverage_test,
+/* support for legacy compile-time setting */
+#ifdef RAW_EXPRESSION_COVERAGE_TEST
+ true,
+#else
+ false,
+#endif
+ NULL, NULL, NULL
+ },
{
{"debug_print_parse", PGC_USERSET, LOGGING_WHAT,
gettext_noop("Logs each query's parse tree."),
diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index 855009fd6e2..6e290224fe5 100644
--- a/src/include/nodes/nodes.h
+++ b/src/include/nodes/nodes.h
@@ -202,9 +202,7 @@ extern char *bmsToString(const struct Bitmapset *bms);
* nodes/{readfuncs.c,read.c}
*/
extern void *stringToNode(const char *str);
-#ifdef WRITE_READ_PARSE_PLAN_TREES
extern void *stringToNodeWithLocations(const char *str);
-#endif
extern struct Bitmapset *readBitmapset(void);
extern uintptr_t readDatum(bool typbyval);
extern bool *readBoolCols(int numCols);
diff --git a/src/include/nodes/readfuncs.h b/src/include/nodes/readfuncs.h
index 8466038ed06..80d83e77ef0 100644
--- a/src/include/nodes/readfuncs.h
+++ b/src/include/nodes/readfuncs.h
@@ -19,9 +19,7 @@
/*
* variable in read.c that needs to be accessible to readfuncs.c
*/
-#ifdef WRITE_READ_PARSE_PLAN_TREES
extern PGDLLIMPORT bool restore_location_fields;
-#endif
/*
* prototypes for functions in read.c (the lisp token parser)
diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h
index f941ee2faf8..392a5e8db6b 100644
--- a/src/include/pg_config_manual.h
+++ b/src/include/pg_config_manual.h
@@ -335,33 +335,12 @@
/* #define RECOVER_RELATION_BUILD_MEMORY 0 */ /* Force disable */
/* #define RECOVER_RELATION_BUILD_MEMORY 1 */ /* Force enable */
-/*
- * Define this to force all parse and plan trees to be passed through
- * copyObject(), to facilitate catching errors and omissions in
- * copyObject().
- */
-/* #define COPY_PARSE_PLAN_TREES */
-
/*
* Define this to force Bitmapset reallocation on each modification. Helps
* to find dangling pointers to Bitmapset's.
*/
/* #define REALLOCATE_BITMAPSETS */
-/*
- * Define this to force all parse and plan trees to be passed through
- * outfuncs.c/readfuncs.c, to facilitate catching errors and omissions in
- * those modules.
- */
-/* #define WRITE_READ_PARSE_PLAN_TREES */
-
-/*
- * Define this to force all raw parse trees for DML statements to be scanned
- * by raw_expression_tree_walker(), to facilitate catching errors and
- * omissions in that function.
- */
-/* #define RAW_EXPRESSION_COVERAGE_TEST */
-
/*
* Enable debugging print statements for lock-related operations.
*/
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index e4a594b5e80..f63ee448848 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -245,6 +245,10 @@ extern PGDLLIMPORT bool Debug_print_parse;
extern PGDLLIMPORT bool Debug_print_rewritten;
extern PGDLLIMPORT bool Debug_pretty_print;
+extern PGDLLIMPORT bool Debug_copy_parse_plan_trees;
+extern PGDLLIMPORT bool Debug_write_read_parse_plan_trees;
+extern PGDLLIMPORT bool Debug_raw_expression_coverage_test;
+
extern PGDLLIMPORT bool log_parser_stats;
extern PGDLLIMPORT bool log_planner_stats;
extern PGDLLIMPORT bool log_executor_stats;
base-commit: fa25dfcd7ec1c72cce68e17aae99bb0f0349749f
--
2.44.0
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_TESTinto run-time parameters
debug_copy_parse_plan_trees
debug_write_read_parse_plan_trees
debug_raw_expression_coverage_testThey 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_TESTinto run-time parameters
debug_copy_parse_plan_trees
debug_write_read_parse_plan_trees
debug_raw_expression_coverage_testI'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_TESTinto run-time parameters
debug_copy_parse_plan_trees
debug_write_read_parse_plan_trees
debug_raw_expression_coverage_testI'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 asdebug_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.
Attachments:
v2-0001-Convert-node-test-compile-time-settings-into-run-.patchtext/plain; charset=UTF-8; name=v2-0001-Convert-node-test-compile-time-settings-into-run-.patchDownload
From 80f35c90e3414dabd879e8832ce1b89c685e5de5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
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 @@ <title>Developer Options</title>
</listitem>
</varlistentry>
+ <varlistentry id="guc-debug-copy-parse-plan-trees" xreflabel="debug_copy_parse_plan_trees">
+ <term><varname>debug_copy_parse_plan_trees</varname> (<type>boolean</type>)
+ <indexterm>
+ <primary><varname>debug_copy_parse_plan_trees</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Enabling this forces all parse and plan trees to be passed through
+ <function>copyObject()</function>, to facilitate catching errors and
+ omissions in <function>copyObject()</function>. The default is off.
+ </para>
+
+ <para>
+ This parameter is only available when
+ <symbol>DEBUG_NODE_TESTS_ENABLED</symbol> was defined at compile time
+ (which happens automatically when using the
+ <application>configure</application> option
+ <option>--enable-cassert</option>).
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-debug-discard-caches" xreflabel="debug_discard_caches">
<term><varname>debug_discard_caches</varname> (<type>integer</type>)
<indexterm>
@@ -11487,6 +11510,54 @@ <title>Developer Options</title>
</listitem>
</varlistentry>
+ <varlistentry id="guc-debug-raw-expression-coverage-test" xreflabel="debug_raw_expression_coverage_test">
+ <term><varname>debug_raw_expression_coverage_test</varname> (<type>boolean</type>)
+ <indexterm>
+ <primary><varname>debug_raw_expression_coverage_test</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Enabling this forces all raw parse trees for DML statements to be
+ scanned by <function>raw_expression_tree_walker()</function>, to
+ facilitate catching errors and omissions in that function. The
+ default is off.
+ </para>
+
+ <para>
+ This parameter is only available when
+ <symbol>DEBUG_NODE_TESTS_ENABLED</symbol> was defined at compile time
+ (which happens automatically when using the
+ <application>configure</application> option
+ <option>--enable-cassert</option>).
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="guc-debug-write-read-parse-plan-trees" xreflabel="debug_write_read_parse_plan_trees">
+ <term><varname>debug_write_read_parse_plan_trees</varname> (<type>boolean</type>)
+ <indexterm>
+ <primary><varname>debug_write_read_parse_plan_trees</varname> configuration parameter</primary>
+ </indexterm>
+ </term>
+ <listitem>
+ <para>
+ Enabling this forces all parse and plan trees to be passed through
+ <filename>outfuncs.c</filename>/<filename>readfuncs.c</filename>, to
+ facilitate catching errors and omissions in those modules. The
+ default is off.
+ </para>
+
+ <para>
+ This parameter is only available when
+ <symbol>DEBUG_NODE_TESTS_ENABLED</symbol> was defined at compile time
+ (which happens automatically when using the
+ <application>configure</application> option
+ <option>--enable-cassert</option>).
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="guc-ignore-system-indexes" xreflabel="ignore_system_indexes">
<term><varname>ignore_system_indexes</varname> (<type>boolean</type>)
<indexterm>
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..190099e5cf3 100644
--- a/src/backend/nodes/read.c
+++ b/src/backend/nodes/read.c
@@ -32,7 +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
+#ifdef DEBUG_NODE_TESTS_ENABLED
bool restore_location_fields = false;
#endif
@@ -43,14 +43,14 @@ bool restore_location_fields = false;
*
* 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.
+ * in builds with DEBUG_NODE_TESTS_ENABLED defined.
*/
static void *
stringToNodeInternal(const char *str, bool restore_loc_fields)
{
void *retval;
const char *save_strtok;
-#ifdef WRITE_READ_PARSE_PLAN_TREES
+#ifdef DEBUG_NODE_TESTS_ENABLED
bool save_restore_location_fields;
#endif
@@ -67,7 +67,7 @@ stringToNodeInternal(const char *str, bool restore_loc_fields)
/*
* If enabled, likewise save/restore the location field handling flag.
*/
-#ifdef WRITE_READ_PARSE_PLAN_TREES
+#ifdef DEBUG_NODE_TESTS_ENABLED
save_restore_location_fields = restore_location_fields;
restore_location_fields = restore_loc_fields;
#endif
@@ -76,7 +76,7 @@ stringToNodeInternal(const char *str, bool restore_loc_fields)
pg_strtok_ptr = save_strtok;
-#ifdef WRITE_READ_PARSE_PLAN_TREES
+#ifdef DEBUG_NODE_TESTS_ENABLED
restore_location_fields = save_restore_location_fields;
#endif
@@ -92,7 +92,7 @@ stringToNode(const char *str)
return stringToNodeInternal(str, false);
}
-#ifdef WRITE_READ_PARSE_PLAN_TREES
+#ifdef DEBUG_NODE_TESTS_ENABLED
void *
stringToNodeWithLocations(const char *str)
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index c4d01a441a0..b47950764a4 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -19,7 +19,7 @@
*
* However, if restore_location_fields is true, we do restore location
* fields from the string. This is currently intended only for use by the
- * WRITE_READ_PARSE_PLAN_TREES test code, which doesn't want to cause
+ * debug_write_read_parse_plan_trees test code, which doesn't want to cause
* any change in the node contents.
*
*-------------------------------------------------------------------------
@@ -118,7 +118,7 @@
local_node->fldname = nullable_string(token, length)
/* Read a parse location field (and possibly throw away the value) */
-#ifdef WRITE_READ_PARSE_PLAN_TREES
+#ifdef DEBUG_NODE_TESTS_ENABLED
#define READ_LOCATION_FIELD(fldname) \
token = pg_strtok(&length); /* skip :fldname */ \
token = pg_strtok(&length); /* get field value */ \
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 28fed9d87f6..e901203424d 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -50,6 +50,7 @@
#include "parser/parsetree.h"
#include "utils/backend_status.h"
#include "utils/builtins.h"
+#include "utils/guc.h"
#include "utils/rel.h"
#include "utils/syscache.h"
@@ -84,7 +85,7 @@ static Query *transformCallStmt(ParseState *pstate,
CallStmt *stmt);
static void transformLockingClause(ParseState *pstate, Query *qry,
LockingClause *lc, bool pushedDown);
-#ifdef RAW_EXPRESSION_COVERAGE_TEST
+#ifdef DEBUG_NODE_TESTS_ENABLED
static bool test_raw_expression_coverage(Node *node, void *context);
#endif
@@ -312,25 +313,30 @@ transformStmt(ParseState *pstate, Node *parseTree)
{
Query *result;
+#ifdef DEBUG_NODE_TESTS_ENABLED
+
/*
- * We apply RAW_EXPRESSION_COVERAGE_TEST testing to basic DML statements;
- * we can't just run it on everything because raw_expression_tree_walker()
- * doesn't claim to handle utility statements.
+ * We apply debug_raw_expression_coverage_test testing to basic DML
+ * statements; we can't just run it on everything because
+ * raw_expression_tree_walker() doesn't claim to handle utility
+ * statements.
*/
-#ifdef RAW_EXPRESSION_COVERAGE_TEST
- switch (nodeTag(parseTree))
+ if (Debug_raw_expression_coverage_test)
{
- case T_SelectStmt:
- case T_InsertStmt:
- case T_UpdateStmt:
- case T_DeleteStmt:
- case T_MergeStmt:
- (void) test_raw_expression_coverage(parseTree, NULL);
- break;
- default:
- break;
+ switch (nodeTag(parseTree))
+ {
+ case T_SelectStmt:
+ case T_InsertStmt:
+ case T_UpdateStmt:
+ case T_DeleteStmt:
+ case T_MergeStmt:
+ (void) test_raw_expression_coverage(parseTree, NULL);
+ break;
+ default:
+ break;
+ }
}
-#endif /* RAW_EXPRESSION_COVERAGE_TEST */
+#endif /* DEBUG_NODE_TESTS_ENABLED */
/*
* Caution: when changing the set of statement types that have non-default
@@ -3575,6 +3581,7 @@ applyLockingClause(Query *qry, Index rtindex,
qry->rowMarks = lappend(qry->rowMarks, rc);
}
+#ifdef DEBUG_NODE_TESTS_ENABLED
/*
* Coverage testing for raw_expression_tree_walker().
*
@@ -3583,8 +3590,6 @@ applyLockingClause(Query *qry, Index rtindex,
* applied in limited cases involving CTEs, and we don't really want to have
* to test everything inside as well as outside a CTE.
*/
-#ifdef RAW_EXPRESSION_COVERAGE_TEST
-
static bool
test_raw_expression_coverage(Node *node, void *context)
{
@@ -3594,5 +3599,4 @@ test_raw_expression_coverage(Node *node, void *context)
test_raw_expression_coverage,
context);
}
-
-#endif /* RAW_EXPRESSION_COVERAGE_TEST */
+#endif /* DEBUG_NODE_TESTS_ENABLED */
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 45a3794b8e3..3746e7e4d17 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -622,8 +622,10 @@ pg_parse_query(const char *query_string)
if (log_parser_stats)
ShowUsage("PARSER STATISTICS");
-#ifdef COPY_PARSE_PLAN_TREES
+#ifdef DEBUG_NODE_TESTS_ENABLED
+
/* Optional debugging check: pass raw parsetrees through copyObject() */
+ if (Debug_copy_parse_plan_trees)
{
List *new_list = copyObject(raw_parsetree_list);
@@ -633,13 +635,12 @@ pg_parse_query(const char *query_string)
else
raw_parsetree_list = new_list;
}
-#endif
/*
* Optional debugging check: pass raw parsetrees through
* outfuncs/readfuncs
*/
-#ifdef WRITE_READ_PARSE_PLAN_TREES
+ if (Debug_write_read_parse_plan_trees)
{
char *str = nodeToStringWithLocations(raw_parsetree_list);
List *new_list = stringToNodeWithLocations(str);
@@ -651,7 +652,8 @@ pg_parse_query(const char *query_string)
else
raw_parsetree_list = new_list;
}
-#endif
+
+#endif /* DEBUG_NODE_TESTS_ENABLED */
TRACE_POSTGRESQL_QUERY_PARSE_DONE(query_string);
@@ -826,8 +828,10 @@ pg_rewrite_query(Query *query)
if (log_parser_stats)
ShowUsage("REWRITER STATISTICS");
-#ifdef COPY_PARSE_PLAN_TREES
+#ifdef DEBUG_NODE_TESTS_ENABLED
+
/* Optional debugging check: pass querytree through copyObject() */
+ if (Debug_copy_parse_plan_trees)
{
List *new_list;
@@ -838,10 +842,9 @@ pg_rewrite_query(Query *query)
else
querytree_list = new_list;
}
-#endif
-#ifdef WRITE_READ_PARSE_PLAN_TREES
/* Optional debugging check: pass querytree through outfuncs/readfuncs */
+ if (Debug_write_read_parse_plan_trees)
{
List *new_list = NIL;
ListCell *lc;
@@ -868,7 +871,8 @@ pg_rewrite_query(Query *query)
else
querytree_list = new_list;
}
-#endif
+
+#endif /* DEBUG_NODE_TESTS_ENABLED */
if (Debug_print_rewritten)
elog_node_display(LOG, "rewritten parse tree", querytree_list,
@@ -906,8 +910,10 @@ pg_plan_query(Query *querytree, const char *query_string, int cursorOptions,
if (log_planner_stats)
ShowUsage("PLANNER STATISTICS");
-#ifdef COPY_PARSE_PLAN_TREES
+#ifdef DEBUG_NODE_TESTS_ENABLED
+
/* Optional debugging check: pass plan tree through copyObject() */
+ if (Debug_copy_parse_plan_trees)
{
PlannedStmt *new_plan = copyObject(plan);
@@ -923,10 +929,9 @@ pg_plan_query(Query *querytree, const char *query_string, int cursorOptions,
#endif
plan = new_plan;
}
-#endif
-#ifdef WRITE_READ_PARSE_PLAN_TREES
/* Optional debugging check: pass plan tree through outfuncs/readfuncs */
+ if (Debug_write_read_parse_plan_trees)
{
char *str;
PlannedStmt *new_plan;
@@ -947,7 +952,8 @@ pg_plan_query(Query *querytree, const char *query_string, int cursorOptions,
#endif
plan = new_plan;
}
-#endif
+
+#endif /* DEBUG_NODE_TESTS_ENABLED */
/*
* Print plan if debugging.
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 46c258be282..d76b2cf0957 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -502,6 +502,12 @@ bool Debug_print_parse = false;
bool Debug_print_rewritten = false;
bool Debug_pretty_print = true;
+#ifdef DEBUG_NODE_TESTS_ENABLED
+bool Debug_copy_parse_plan_trees;
+bool Debug_write_read_parse_plan_trees;
+bool Debug_raw_expression_coverage_test;
+#endif
+
bool log_parser_stats = false;
bool log_planner_stats = false;
bool log_executor_stats = false;
@@ -1301,6 +1307,59 @@ struct config_bool ConfigureNamesBool[] =
false,
NULL, NULL, NULL
},
+#ifdef DEBUG_NODE_TESTS_ENABLED
+ {
+ {"debug_copy_parse_plan_trees", PGC_SUSET, DEVELOPER_OPTIONS,
+ gettext_noop("Set this to force all parse and plan trees to be passed through "
+ "copyObject(), to facilitate catching errors and omissions in "
+ "copyObject()."),
+ NULL,
+ GUC_NOT_IN_SAMPLE
+ },
+ &Debug_copy_parse_plan_trees,
+/* support for legacy compile-time setting */
+#ifdef COPY_PARSE_PLAN_TREES
+ true,
+#else
+ false,
+#endif
+ NULL, NULL, NULL
+ },
+ {
+ {"debug_write_read_parse_plan_trees", PGC_SUSET, DEVELOPER_OPTIONS,
+ gettext_noop("Set this to force all parse and plan trees to be passed through "
+ "outfuncs.c/readfuncs.c, to facilitate catching errors and omissions in "
+ "those modules."),
+ NULL,
+ GUC_NOT_IN_SAMPLE
+ },
+ &Debug_write_read_parse_plan_trees,
+/* support for legacy compile-time setting */
+#ifdef WRITE_READ_PARSE_PLAN_TREES
+ true,
+#else
+ false,
+#endif
+ NULL, NULL, NULL
+ },
+ {
+ {"debug_raw_expression_coverage_test", PGC_SUSET, DEVELOPER_OPTIONS,
+ gettext_noop("Set this to force all raw parse trees for DML statements to be scanned "
+ "by raw_expression_tree_walker(), to facilitate catching errors and "
+ "omissions in that function."),
+ NULL,
+ GUC_NOT_IN_SAMPLE
+ },
+ &Debug_raw_expression_coverage_test,
+/* support for legacy compile-time setting */
+#ifdef RAW_EXPRESSION_COVERAGE_TEST
+ true,
+#else
+ false,
+#endif
+ NULL, NULL, NULL
+ },
+#endif /* DEBUG_NODE_TESTS_ENABLED */
{
{"debug_print_parse", PGC_USERSET, LOGGING_WHAT,
gettext_noop("Logs each query's parse tree."),
diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index 855009fd6e2..a96f7c97f16 100644
--- a/src/include/nodes/nodes.h
+++ b/src/include/nodes/nodes.h
@@ -202,7 +202,7 @@ extern char *bmsToString(const struct Bitmapset *bms);
* nodes/{readfuncs.c,read.c}
*/
extern void *stringToNode(const char *str);
-#ifdef WRITE_READ_PARSE_PLAN_TREES
+#ifdef DEBUG_NODE_TESTS_ENABLED
extern void *stringToNodeWithLocations(const char *str);
#endif
extern struct Bitmapset *readBitmapset(void);
diff --git a/src/include/nodes/readfuncs.h b/src/include/nodes/readfuncs.h
index 8466038ed06..5cd35d68706 100644
--- a/src/include/nodes/readfuncs.h
+++ b/src/include/nodes/readfuncs.h
@@ -19,7 +19,7 @@
/*
* variable in read.c that needs to be accessible to readfuncs.c
*/
-#ifdef WRITE_READ_PARSE_PLAN_TREES
+#ifdef DEBUG_NODE_TESTS_ENABLED
extern PGDLLIMPORT bool restore_location_fields;
#endif
diff --git a/src/include/pg_config_manual.h b/src/include/pg_config_manual.h
index f941ee2faf8..e3da9fa13d2 100644
--- a/src/include/pg_config_manual.h
+++ b/src/include/pg_config_manual.h
@@ -336,31 +336,31 @@
/* #define RECOVER_RELATION_BUILD_MEMORY 1 */ /* Force enable */
/*
- * Define this to force all parse and plan trees to be passed through
- * copyObject(), to facilitate catching errors and omissions in
- * copyObject().
+ * Define DEBUG_NODE_TESTS_ENABLED to enable use of the GUCs
+ * debug_copy_parse_plan_trees, debug_write_read_parse_plan_trees, and
+ * debug_raw_expression_coverage_test, to test coverage of node support
+ * functions in src/backend/nodes/.
+ *
+ * USE_ASSERT_CHECKING builds default to enabling this.
*/
-/* #define COPY_PARSE_PLAN_TREES */
+/* #define DEBUG_NODE_TESTS_ENABLED */
-/*
- * Define this to force Bitmapset reallocation on each modification. Helps
- * to find dangling pointers to Bitmapset's.
- */
-/* #define REALLOCATE_BITMAPSETS */
+#if defined(USE_ASSERT_CHECKING) && !defined(DEBUG_NODE_TESTS_ENABLED)
+#define DEBUG_NODE_TESTS_ENABLED
+#endif
/*
- * Define this to force all parse and plan trees to be passed through
- * outfuncs.c/readfuncs.c, to facilitate catching errors and omissions in
- * those modules.
+ * Backwards compatibility for the older compile-time-only node-tests macros.
*/
-/* #define WRITE_READ_PARSE_PLAN_TREES */
+#if !defined(DEBUG_NODE_TESTS_ENABLED) && (defined(COPY_PARSE_PLAN_TREES) || defined(WRITE_READ_PARSE_PLAN_TREES) || defined(RAW_EXPRESSION_COVERAGE_TEST))
+#define DEBUG_NODE_TESTS_ENABLED
+#endif
/*
- * Define this to force all raw parse trees for DML statements to be scanned
- * by raw_expression_tree_walker(), to facilitate catching errors and
- * omissions in that function.
+ * Define this to force Bitmapset reallocation on each modification. Helps
+ * to find dangling pointers to Bitmapset's.
*/
-/* #define RAW_EXPRESSION_COVERAGE_TEST */
+/* #define REALLOCATE_BITMAPSETS */
/*
* Enable debugging print statements for lock-related operations.
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index e4a594b5e80..35f0ef6f5b6 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -245,6 +245,12 @@ extern PGDLLIMPORT bool Debug_print_parse;
extern PGDLLIMPORT bool Debug_print_rewritten;
extern PGDLLIMPORT bool Debug_pretty_print;
+#ifdef DEBUG_NODE_TESTS_ENABLED
+extern PGDLLIMPORT bool Debug_copy_parse_plan_trees;
+extern PGDLLIMPORT bool Debug_write_read_parse_plan_trees;
+extern PGDLLIMPORT bool Debug_raw_expression_coverage_test;
+#endif
+
extern PGDLLIMPORT bool log_parser_stats;
extern PGDLLIMPORT bool log_planner_stats;
extern PGDLLIMPORT bool log_executor_stats;
base-commit: 53785d2a2aaa7899eb82fb4eba9af6da83680c8d
--
2.44.0
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.