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 <pe...@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
