]> granicus.if.org Git - postgresql/commitdiff
Add a debugging option to stress-test outfuncs.c and readfuncs.c.
authorTom Lane <tgl@sss.pgh.pa.us>
Tue, 18 Sep 2018 21:11:54 +0000 (17:11 -0400)
committerTom Lane <tgl@sss.pgh.pa.us>
Tue, 18 Sep 2018 21:11:54 +0000 (17:11 -0400)
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

src/backend/nodes/read.c
src/backend/nodes/readfuncs.c
src/backend/parser/parse_relation.c
src/backend/tcop/postgres.c
src/include/nodes/nodes.h
src/include/nodes/readfuncs.h
src/include/pg_config_manual.h

index a775f9120ee545c10c9d802a0fe9bd58a30cea3e..99ed2f248a2dd9004055268306acf765ea1a29a3 100644 (file)
 
 
 /* Static state for pg_strtok */
-static char *pg_strtok_ptr = NULL;
+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
 
 
 /*
  * stringToNode -
- *       returns a Node with a given legal ASCII representation
+ *       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.
  */
-void *
-stringToNode(char *str)
+static void *
+stringToNodeInternal(const char *str, bool restore_loc_fields)
 {
-       char       *save_strtok;
        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
@@ -51,13 +63,45 @@ stringToNode(char *str)
 
        pg_strtok_ptr = str;            /* point pg_strtok at the string to read */
 
+       /*
+        * 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;
 }
 
+/*
+ * Externally visible entry points
+ */
+void *
+stringToNode(const char *str)
+{
+       return stringToNodeInternal(str, false);
+}
+
+#ifdef WRITE_READ_PARSE_PLAN_TREES
+
+void *
+stringToNodeWithLocations(const char *str)
+{
+       return stringToNodeInternal(str, true);
+}
+
+#endif
+
+
 /*****************************************************************************
  *
  * the lisp token parser
@@ -104,11 +148,11 @@ stringToNode(char *str)
  * code should add backslashes to a string constant to ensure it is treated
  * as a single token.
  */
-char *
+const char *
 pg_strtok(int *length)
 {
-       char       *local_str;          /* working pointer to string */
-       char       *ret_str;            /* start of token to return */
+       const char *local_str;          /* working pointer to string */
+       const char *ret_str;            /* start of token to return */
 
        local_str = pg_strtok_ptr;
 
@@ -166,7 +210,7 @@ pg_strtok(int *length)
  *       any protective backslashes in the token are removed.
  */
 char *
-debackslash(char *token, int length)
+debackslash(const char *token, int length)
 {
        char       *result = palloc(length + 1);
        char       *ptr = result;
@@ -198,10 +242,10 @@ debackslash(char *token, int length)
  *       Assumption: the ascii representation is legal
  */
 static NodeTag
-nodeTokenType(char *token, int length)
+nodeTokenType(const char *token, int length)
 {
        NodeTag         retval;
-       char       *numptr;
+       const char *numptr;
        int                     numlen;
 
        /*
@@ -269,7 +313,7 @@ nodeTokenType(char *token, int length)
  * this should only be invoked from within a stringToNode operation).
  */
 void *
-nodeRead(char *token, int tok_len)
+nodeRead(const char *token, int tok_len)
 {
        Node       *result;
        NodeTag         type;
index 81f568b3ee1135cf9abe542dbd9188f430ca4722..519deab63ab23dba2f9cd82db1b44c7b39a90cf2 100644 (file)
  *       never read executor state trees, either.
  *
  *       Parse location fields are written out by outfuncs.c, but only for
- *       possible debugging use.  When reading a location field, we discard
+ *       debugging use.  When reading a location field, we normally discard
  *       the stored value and set the location field to -1 (ie, "unknown").
  *       This is because nodes coming from a stored rule should not be thought
  *       to have a known location in the current query's text.
+ *       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
+ *       any change in the node contents.
  *
  *-------------------------------------------------------------------------
  */
@@ -51,7 +55,7 @@
 
 /* And a few guys need only the pg_strtok support fields */
 #define READ_TEMP_LOCALS()     \
-       char       *token;              \
+       const char *token;              \
        int                     length
 
 /* ... but most need both */
        token = pg_strtok(&length);             /* get field value */ \
        local_node->fldname = nullable_string(token, length)
 
-/* Read a parse location field (and throw away the value, per notes above) */
+/* 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) \
@@ -2804,7 +2815,7 @@ readDatum(bool typbyval)
        Size            length,
                                i;
        int                     tokenLength;
-       char       *token;
+       const char *token;
        Datum           res;
        char       *s;
 
@@ -2817,7 +2828,7 @@ readDatum(bool typbyval)
        token = pg_strtok(&tokenLength);        /* read the '[' */
        if (token == NULL || token[0] != '[')
                elog(ERROR, "expected \"[\" to start datum, but got \"%s\"; length = %zu",
-                        token ? (const char *) token : "[NULL]", length);
+                        token ? token : "[NULL]", length);
 
        if (typbyval)
        {
@@ -2847,7 +2858,7 @@ readDatum(bool typbyval)
        token = pg_strtok(&tokenLength);        /* read the ']' */
        if (token == NULL || token[0] != ']')
                elog(ERROR, "expected \"]\" to end datum, but got \"%s\"; length = %zu",
-                        token ? (const char *) token : "[NULL]", length);
+                        token ? token : "[NULL]", length);
 
        return res;
 }
@@ -2860,7 +2871,7 @@ readAttrNumberCols(int numCols)
 {
        int                     tokenLength,
                                i;
-       char       *token;
+       const char *token;
        AttrNumber *attr_vals;
 
        if (numCols <= 0)
@@ -2884,7 +2895,7 @@ readOidCols(int numCols)
 {
        int                     tokenLength,
                                i;
-       char       *token;
+       const char *token;
        Oid                *oid_vals;
 
        if (numCols <= 0)
@@ -2908,7 +2919,7 @@ readIntCols(int numCols)
 {
        int                     tokenLength,
                                i;
-       char       *token;
+       const char *token;
        int                *int_vals;
 
        if (numCols <= 0)
@@ -2932,7 +2943,7 @@ readBoolCols(int numCols)
 {
        int                     tokenLength,
                                i;
-       char       *token;
+       const char *token;
        bool       *bool_vals;
 
        if (numCols <= 0)
index bf5df26009a45b137c37089e7142283c01c7a224..60b8de0f95d375b87dc5779e12b7834cc6dab6f4 100644 (file)
@@ -1335,7 +1335,6 @@ addRangeTableEntryForSubquery(ParseState *pstate,
        Assert(pstate != NULL);
 
        rte->rtekind = RTE_SUBQUERY;
-       rte->relid = InvalidOid;
        rte->subquery = subquery;
        rte->alias = alias;
 
index 7a9ada2c719cc38f5d4dfb234090a0b118243dc7..e4c6e3d406e9928158feebf1fbd7fb9445db9afc 100644 (file)
@@ -633,6 +633,12 @@ pg_parse_query(const char *query_string)
        }
 #endif
 
+       /*
+        * Currently, outfuncs/readfuncs support is missing for many raw parse
+        * tree nodes, so we don't try to implement WRITE_READ_PARSE_PLAN_TREES
+        * here.
+        */
+
        TRACE_POSTGRESQL_QUERY_PARSE_DONE(query_string);
 
        return raw_parsetree_list;
@@ -763,7 +769,7 @@ pg_rewrite_query(Query *query)
                ShowUsage("REWRITER STATISTICS");
 
 #ifdef COPY_PARSE_PLAN_TREES
-       /* Optional debugging check: pass querytree output through copyObject() */
+       /* Optional debugging check: pass querytree through copyObject() */
        {
                List       *new_list;
 
@@ -776,6 +782,46 @@ pg_rewrite_query(Query *query)
        }
 #endif
 
+#ifdef WRITE_READ_PARSE_PLAN_TREES
+       /* Optional debugging check: pass querytree through outfuncs/readfuncs */
+       {
+               List       *new_list = NIL;
+               ListCell   *lc;
+
+               /*
+                * We currently lack outfuncs/readfuncs support for most utility
+                * statement types, so only attempt to write/read non-utility queries.
+                */
+               foreach(lc, querytree_list)
+               {
+                       Query      *query = castNode(Query, lfirst(lc));
+
+                       if (query->commandType != CMD_UTILITY)
+                       {
+                               char       *str = nodeToString(query);
+                               Query      *new_query = stringToNodeWithLocations(str);
+
+                               /*
+                                * queryId is not saved in stored rules, but we must preserve
+                                * it here to avoid breaking pg_stat_statements.
+                                */
+                               new_query->queryId = query->queryId;
+
+                               new_list = lappend(new_list, new_query);
+                               pfree(str);
+                       }
+                       else
+                               new_list = lappend(new_list, query);
+               }
+
+               /* This checks both outfuncs/readfuncs and the equal() routines... */
+               if (!equal(new_list, querytree_list))
+                       elog(WARNING, "outfuncs/readfuncs failed to produce equal parse tree");
+               else
+                       querytree_list = new_list;
+       }
+#endif
+
        if (Debug_print_rewritten)
                elog_node_display(LOG, "rewritten parse tree", querytree_list,
                                                  Debug_pretty_print);
@@ -812,7 +858,7 @@ pg_plan_query(Query *querytree, int cursorOptions, ParamListInfo boundParams)
                ShowUsage("PLANNER STATISTICS");
 
 #ifdef COPY_PARSE_PLAN_TREES
-       /* Optional debugging check: pass plan output through copyObject() */
+       /* Optional debugging check: pass plan tree through copyObject() */
        {
                PlannedStmt *new_plan = copyObject(plan);
 
@@ -830,6 +876,30 @@ pg_plan_query(Query *querytree, int cursorOptions, ParamListInfo boundParams)
        }
 #endif
 
+#ifdef WRITE_READ_PARSE_PLAN_TREES
+       /* Optional debugging check: pass plan tree through outfuncs/readfuncs */
+       {
+               char       *str;
+               PlannedStmt *new_plan;
+
+               str = nodeToString(plan);
+               new_plan = stringToNodeWithLocations(str);
+               pfree(str);
+
+               /*
+                * equal() currently does not have routines to compare Plan nodes, so
+                * don't try to test equality here.  Perhaps fix someday?
+                */
+#ifdef NOT_USED
+               /* This checks both outfuncs/readfuncs and the equal() routines... */
+               if (!equal(new_plan, plan))
+                       elog(WARNING, "outfuncs/readfuncs failed to produce an equal plan tree");
+               else
+#endif
+                       plan = new_plan;
+       }
+#endif
+
        /*
         * Print plan if debugging.
         */
index 697d3d7a5fd0be362219572c8fffd401091a2a24..cac6ff0eda49ffd1cefe1f605d43a140e82218b7 100644 (file)
@@ -610,7 +610,10 @@ extern char *bmsToString(const struct Bitmapset *bms);
 /*
  * nodes/{readfuncs.c,read.c}
  */
-extern void *stringToNode(char *str);
+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);
index 491e61c459f535b1ecce10fd8dadaa21e0e63e39..4f0d3c2192d191f98d9fe70eabbac007c56fbe23 100644 (file)
 
 #include "nodes/nodes.h"
 
+/*
+ * variable in read.c that needs to be accessible to readfuncs.c
+ */
+#ifdef WRITE_READ_PARSE_PLAN_TREES
+extern bool restore_location_fields;
+#endif
+
 /*
  * prototypes for functions in read.c (the lisp token parser)
  */
-extern char *pg_strtok(int *length);
-extern char *debackslash(char *token, int length);
-extern void *nodeRead(char *token, int tok_len);
+extern const char *pg_strtok(int *length);
+extern char *debackslash(const char *token, int length);
+extern void *nodeRead(const char *token, int tok_len);
 
 /*
  * prototypes for functions in readfuncs.c
index b309395f11c13deabb4a9d9b1d60252d60ff6016..b0365254f654b6413bb8e49ae797dde10b259b48 100644 (file)
  */
 /* #define COPY_PARSE_PLAN_TREES */
 
+/*
+ * 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