Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit d0cfc3d

Browse files
committed
Add a debugging option to stress-test outfuncs.c and readfuncs.c.
In the normal course of operation, query trees will be serialized only if they are stored as views or rules; and plan trees will be serialized only if they get passed to parallel-query workers. This leaves an awful lot of opportunity for bugs/oversights to not get detected, as indeed we've just been reminded of the hard way. To improve matters, this patch adds a new compile option WRITE_READ_PARSE_PLAN_TREES, which is modeled on the longstanding option COPY_PARSE_PLAN_TREES; but instead of passing all parse and plan trees through copyObject, it passes them through nodeToString + stringToNode. Enabling this option in a buildfarm animal or two will catch problems at least for cases that are exercised by the regression tests. A small problem with this idea is that readfuncs.c historically has discarded location fields, on the reasonable grounds that parse locations in a retrieved view are not relevant to the current query. But doing that in WRITE_READ_PARSE_PLAN_TREES breaks pg_stat_statements, and it could cause problems for future improvements that might try to report error locations at runtime. To fix that, provide a variant behavior in readfuncs.c that makes it restore location fields when told to. In passing, const-ify the string arguments of stringToNode and its subsidiary functions, just because it annoyed me that they weren't const already. Discussion: https://postgr.es/m/17114.1537138992@sss.pgh.pa.us
1 parent db1071d commit d0cfc3d

File tree

7 files changed

+170
-29
lines changed

7 files changed

+170
-29
lines changed

src/backend/nodes/read.c

+56-12
Original file line numberDiff line numberDiff line change
@@ -28,18 +28,30 @@
2828

2929

3030
/* Static state for pg_strtok */
31-
static char *pg_strtok_ptr = NULL;
31+
static const char *pg_strtok_ptr = NULL;
32+
33+
/* State flag that determines how readfuncs.c should treat location fields */
34+
#ifdef WRITE_READ_PARSE_PLAN_TREES
35+
bool restore_location_fields = false;
36+
#endif
3237

3338

3439
/*
3540
* stringToNode -
36-
* returns a Node with a given legal ASCII representation
41+
* builds a Node tree from its string representation (assumed valid)
42+
*
43+
* restore_loc_fields instructs readfuncs.c whether to restore location
44+
* fields rather than set them to -1. This is currently only supported
45+
* in builds with the WRITE_READ_PARSE_PLAN_TREES debugging flag set.
3746
*/
38-
void *
39-
stringToNode(char *str)
47+
static void *
48+
stringToNodeInternal(const char *str, bool restore_loc_fields)
4049
{
41-
char *save_strtok;
4250
void *retval;
51+
const char *save_strtok;
52+
#ifdef WRITE_READ_PARSE_PLAN_TREES
53+
bool save_restore_location_fields;
54+
#endif
4355

4456
/*
4557
* We save and restore the pre-existing state of pg_strtok. This makes the
@@ -51,13 +63,45 @@ stringToNode(char *str)
5163

5264
pg_strtok_ptr = str; /* point pg_strtok at the string to read */
5365

66+
/*
67+
* If enabled, likewise save/restore the location field handling flag.
68+
*/
69+
#ifdef WRITE_READ_PARSE_PLAN_TREES
70+
save_restore_location_fields = restore_location_fields;
71+
restore_location_fields = restore_loc_fields;
72+
#endif
73+
5474
retval = nodeRead(NULL, 0); /* do the reading */
5575

5676
pg_strtok_ptr = save_strtok;
5777

78+
#ifdef WRITE_READ_PARSE_PLAN_TREES
79+
restore_location_fields = save_restore_location_fields;
80+
#endif
81+
5882
return retval;
5983
}
6084

85+
/*
86+
* Externally visible entry points
87+
*/
88+
void *
89+
stringToNode(const char *str)
90+
{
91+
return stringToNodeInternal(str, false);
92+
}
93+
94+
#ifdef WRITE_READ_PARSE_PLAN_TREES
95+
96+
void *
97+
stringToNodeWithLocations(const char *str)
98+
{
99+
return stringToNodeInternal(str, true);
100+
}
101+
102+
#endif
103+
104+
61105
/*****************************************************************************
62106
*
63107
* the lisp token parser
@@ -104,11 +148,11 @@ stringToNode(char *str)
104148
* code should add backslashes to a string constant to ensure it is treated
105149
* as a single token.
106150
*/
107-
char *
151+
const char *
108152
pg_strtok(int *length)
109153
{
110-
char *local_str; /* working pointer to string */
111-
char *ret_str; /* start of token to return */
154+
const char *local_str; /* working pointer to string */
155+
const char *ret_str; /* start of token to return */
112156

113157
local_str = pg_strtok_ptr;
114158

@@ -166,7 +210,7 @@ pg_strtok(int *length)
166210
* any protective backslashes in the token are removed.
167211
*/
168212
char *
169-
debackslash(char *token, int length)
213+
debackslash(const char *token, int length)
170214
{
171215
char *result = palloc(length + 1);
172216
char *ptr = result;
@@ -198,10 +242,10 @@ debackslash(char *token, int length)
198242
* Assumption: the ascii representation is legal
199243
*/
200244
static NodeTag
201-
nodeTokenType(char *token, int length)
245+
nodeTokenType(const char *token, int length)
202246
{
203247
NodeTag retval;
204-
char *numptr;
248+
const char *numptr;
205249
int numlen;
206250

207251
/*
@@ -269,7 +313,7 @@ nodeTokenType(char *token, int length)
269313
* this should only be invoked from within a stringToNode operation).
270314
*/
271315
void *
272-
nodeRead(char *token, int tok_len)
316+
nodeRead(const char *token, int tok_len)
273317
{
274318
Node *result;
275319
NodeTag type;

src/backend/nodes/readfuncs.c

+21-10
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,14 @@
1717
* never read executor state trees, either.
1818
*
1919
* Parse location fields are written out by outfuncs.c, but only for
20-
* possible debugging use. When reading a location field, we discard
20+
* debugging use. When reading a location field, we normally discard
2121
* the stored value and set the location field to -1 (ie, "unknown").
2222
* This is because nodes coming from a stored rule should not be thought
2323
* to have a known location in the current query's text.
24+
* However, if restore_location_fields is true, we do restore location
25+
* fields from the string. This is currently intended only for use by the
26+
* WRITE_READ_PARSE_PLAN_TREES test code, which doesn't want to cause
27+
* any change in the node contents.
2428
*
2529
*-------------------------------------------------------------------------
2630
*/
@@ -51,7 +55,7 @@
5155

5256
/* And a few guys need only the pg_strtok support fields */
5357
#define READ_TEMP_LOCALS() \
54-
char *token; \
58+
const char *token; \
5559
int length
5660

5761
/* ... but most need both */
@@ -120,12 +124,19 @@
120124
token = pg_strtok(&length); /* get field value */ \
121125
local_node->fldname = nullable_string(token, length)
122126

123-
/* Read a parse location field (and throw away the value, per notes above) */
127+
/* Read a parse location field (and possibly throw away the value) */
128+
#ifdef WRITE_READ_PARSE_PLAN_TREES
129+
#define READ_LOCATION_FIELD(fldname) \
130+
token = pg_strtok(&length); /* skip :fldname */ \
131+
token = pg_strtok(&length); /* get field value */ \
132+
local_node->fldname = restore_location_fields ? atoi(token) : -1
133+
#else
124134
#define READ_LOCATION_FIELD(fldname) \
125135
token = pg_strtok(&length); /* skip :fldname */ \
126136
token = pg_strtok(&length); /* get field value */ \
127137
(void) token; /* in case not used elsewhere */ \
128138
local_node->fldname = -1 /* set field to "unknown" */
139+
#endif
129140

130141
/* Read a Node field */
131142
#define READ_NODE_FIELD(fldname) \
@@ -2804,7 +2815,7 @@ readDatum(bool typbyval)
28042815
Size length,
28052816
i;
28062817
int tokenLength;
2807-
char *token;
2818+
const char *token;
28082819
Datum res;
28092820
char *s;
28102821

@@ -2817,7 +2828,7 @@ readDatum(bool typbyval)
28172828
token = pg_strtok(&tokenLength); /* read the '[' */
28182829
if (token == NULL || token[0] != '[')
28192830
elog(ERROR, "expected \"[\" to start datum, but got \"%s\"; length = %zu",
2820-
token ? (const char *) token : "[NULL]", length);
2831+
token ? token : "[NULL]", length);
28212832

28222833
if (typbyval)
28232834
{
@@ -2847,7 +2858,7 @@ readDatum(bool typbyval)
28472858
token = pg_strtok(&tokenLength); /* read the ']' */
28482859
if (token == NULL || token[0] != ']')
28492860
elog(ERROR, "expected \"]\" to end datum, but got \"%s\"; length = %zu",
2850-
token ? (const char *) token : "[NULL]", length);
2861+
token ? token : "[NULL]", length);
28512862

28522863
return res;
28532864
}
@@ -2860,7 +2871,7 @@ readAttrNumberCols(int numCols)
28602871
{
28612872
int tokenLength,
28622873
i;
2863-
char *token;
2874+
const char *token;
28642875
AttrNumber *attr_vals;
28652876

28662877
if (numCols <= 0)
@@ -2884,7 +2895,7 @@ readOidCols(int numCols)
28842895
{
28852896
int tokenLength,
28862897
i;
2887-
char *token;
2898+
const char *token;
28882899
Oid *oid_vals;
28892900

28902901
if (numCols <= 0)
@@ -2908,7 +2919,7 @@ readIntCols(int numCols)
29082919
{
29092920
int tokenLength,
29102921
i;
2911-
char *token;
2922+
const char *token;
29122923
int *int_vals;
29132924

29142925
if (numCols <= 0)
@@ -2932,7 +2943,7 @@ readBoolCols(int numCols)
29322943
{
29332944
int tokenLength,
29342945
i;
2935-
char *token;
2946+
const char *token;
29362947
bool *bool_vals;
29372948

29382949
if (numCols <= 0)

src/backend/parser/parse_relation.c

-1
Original file line numberDiff line numberDiff line change
@@ -1335,7 +1335,6 @@ addRangeTableEntryForSubquery(ParseState *pstate,
13351335
Assert(pstate != NULL);
13361336

13371337
rte->rtekind = RTE_SUBQUERY;
1338-
rte->relid = InvalidOid;
13391338
rte->subquery = subquery;
13401339
rte->alias = alias;
13411340

src/backend/tcop/postgres.c

+72-2
Original file line numberDiff line numberDiff line change
@@ -633,6 +633,12 @@ pg_parse_query(const char *query_string)
633633
}
634634
#endif
635635

636+
/*
637+
* Currently, outfuncs/readfuncs support is missing for many raw parse
638+
* tree nodes, so we don't try to implement WRITE_READ_PARSE_PLAN_TREES
639+
* here.
640+
*/
641+
636642
TRACE_POSTGRESQL_QUERY_PARSE_DONE(query_string);
637643

638644
return raw_parsetree_list;
@@ -763,7 +769,7 @@ pg_rewrite_query(Query *query)
763769
ShowUsage("REWRITER STATISTICS");
764770

765771
#ifdef COPY_PARSE_PLAN_TREES
766-
/* Optional debugging check: pass querytree output through copyObject() */
772+
/* Optional debugging check: pass querytree through copyObject() */
767773
{
768774
List *new_list;
769775

@@ -776,6 +782,46 @@ pg_rewrite_query(Query *query)
776782
}
777783
#endif
778784

785+
#ifdef WRITE_READ_PARSE_PLAN_TREES
786+
/* Optional debugging check: pass querytree through outfuncs/readfuncs */
787+
{
788+
List *new_list = NIL;
789+
ListCell *lc;
790+
791+
/*
792+
* We currently lack outfuncs/readfuncs support for most utility
793+
* statement types, so only attempt to write/read non-utility queries.
794+
*/
795+
foreach(lc, querytree_list)
796+
{
797+
Query *query = castNode(Query, lfirst(lc));
798+
799+
if (query->commandType != CMD_UTILITY)
800+
{
801+
char *str = nodeToString(query);
802+
Query *new_query = stringToNodeWithLocations(str);
803+
804+
/*
805+
* queryId is not saved in stored rules, but we must preserve
806+
* it here to avoid breaking pg_stat_statements.
807+
*/
808+
new_query->queryId = query->queryId;
809+
810+
new_list = lappend(new_list, new_query);
811+
pfree(str);
812+
}
813+
else
814+
new_list = lappend(new_list, query);
815+
}
816+
817+
/* This checks both outfuncs/readfuncs and the equal() routines... */
818+
if (!equal(new_list, querytree_list))
819+
elog(WARNING, "outfuncs/readfuncs failed to produce equal parse tree");
820+
else
821+
querytree_list = new_list;
822+
}
823+
#endif
824+
779825
if (Debug_print_rewritten)
780826
elog_node_display(LOG, "rewritten parse tree", querytree_list,
781827
Debug_pretty_print);
@@ -812,7 +858,7 @@ pg_plan_query(Query *querytree, int cursorOptions, ParamListInfo boundParams)
812858
ShowUsage("PLANNER STATISTICS");
813859

814860
#ifdef COPY_PARSE_PLAN_TREES
815-
/* Optional debugging check: pass plan output through copyObject() */
861+
/* Optional debugging check: pass plan tree through copyObject() */
816862
{
817863
PlannedStmt *new_plan = copyObject(plan);
818864

@@ -830,6 +876,30 @@ pg_plan_query(Query *querytree, int cursorOptions, ParamListInfo boundParams)
830876
}
831877
#endif
832878

879+
#ifdef WRITE_READ_PARSE_PLAN_TREES
880+
/* Optional debugging check: pass plan tree through outfuncs/readfuncs */
881+
{
882+
char *str;
883+
PlannedStmt *new_plan;
884+
885+
str = nodeToString(plan);
886+
new_plan = stringToNodeWithLocations(str);
887+
pfree(str);
888+
889+
/*
890+
* equal() currently does not have routines to compare Plan nodes, so
891+
* don't try to test equality here. Perhaps fix someday?
892+
*/
893+
#ifdef NOT_USED
894+
/* This checks both outfuncs/readfuncs and the equal() routines... */
895+
if (!equal(new_plan, plan))
896+
elog(WARNING, "outfuncs/readfuncs failed to produce an equal plan tree");
897+
else
898+
#endif
899+
plan = new_plan;
900+
}
901+
#endif
902+
833903
/*
834904
* Print plan if debugging.
835905
*/

src/include/nodes/nodes.h

+4-1
Original file line numberDiff line numberDiff line change
@@ -610,7 +610,10 @@ extern char *bmsToString(const struct Bitmapset *bms);
610610
/*
611611
* nodes/{readfuncs.c,read.c}
612612
*/
613-
extern void *stringToNode(char *str);
613+
extern void *stringToNode(const char *str);
614+
#ifdef WRITE_READ_PARSE_PLAN_TREES
615+
extern void *stringToNodeWithLocations(const char *str);
616+
#endif
614617
extern struct Bitmapset *readBitmapset(void);
615618
extern uintptr_t readDatum(bool typbyval);
616619
extern bool *readBoolCols(int numCols);

src/include/nodes/readfuncs.h

+10-3
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,19 @@
1616

1717
#include "nodes/nodes.h"
1818

19+
/*
20+
* variable in read.c that needs to be accessible to readfuncs.c
21+
*/
22+
#ifdef WRITE_READ_PARSE_PLAN_TREES
23+
extern bool restore_location_fields;
24+
#endif
25+
1926
/*
2027
* prototypes for functions in read.c (the lisp token parser)
2128
*/
22-
extern char *pg_strtok(int *length);
23-
extern char *debackslash(char *token, int length);
24-
extern void *nodeRead(char *token, int tok_len);
29+
extern const char *pg_strtok(int *length);
30+
extern char *debackslash(const char *token, int length);
31+
extern void *nodeRead(const char *token, int tok_len);
2532

2633
/*
2734
* prototypes for functions in readfuncs.c

0 commit comments

Comments
 (0)