Re: Re: [BUGS] BUG #14153: Unrecognized node type error when upsert is present in recursive CTE

Peter Geoghegan <pg@heroku.com> writes:
> What I meant is that I think naively adding the choke-point for a
> top-level SelectStmt would do the job (although perhaps we could do
> better). I wasn't suggesting that we'd avoid recursing from there;
> only that we could reasonably avoid recursing from someplace else
> (e.g. InsertStmt) in the hope of finding a SelectStmt to test.
> Offhand, I don't imagine that that would offer better coverage.

I think it would.  The attached patch shows nearly 150 failures in
the current regression tests.  If I remove "case T_InsertStmt" that
drops to two failures: there are two cases in with.sgml that almost
manage to exercise the bug, but not quite because they are not
WITH RECURSIVE.  That doesn't leave me with any warm feeling about
being able to find other similar oversights if we apply this testing
only to top-level SelectStmts.

>> If that sounds like a plausible choke-point, the next question is what
>> to use to enable the debug test.  I propose "#ifdef COPY_PARSE_PLAN_TREES"
>> since that enables similar sanity checking for other parts of
>> backend/nodes/, and we do have at least one buildfarm member using it.

> That's what I was thinking, too. No need to keep it separate.

After cogitating, I did it as attached just for readability's sake.

            regards, tom lane

diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 7bc5be1..92f3276 100644
*** a/src/backend/nodes/nodeFuncs.c
--- b/src/backend/nodes/nodeFuncs.c
*************** query_or_expression_tree_mutator(Node *n
*** 2991,2998 ****
   * Unlike expression_tree_walker, there is no special rule about query
   * boundaries: we descend to everything that's possibly interesting.
   *
!  * Currently, the node type coverage extends to SelectStmt and everything
!  * that could appear under it, but not other statement types.
   */
  bool
  raw_expression_tree_walker(Node *node,
--- 2991,3000 ----
   * Unlike expression_tree_walker, there is no special rule about query
   * boundaries: we descend to everything that's possibly interesting.
   *
!  * Currently, the node type coverage here extends only to DML statements
!  * (SELECT/INSERT/UPDATE/DELETE) and nodes that can appear in them, because
!  * this is used mainly during analysis of CTEs, and only DML statements can
!  * appear in CTEs.
   */
  bool
  raw_expression_tree_walker(Node *node,
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 7d2fedf..5979e76 100644
*** a/src/backend/parser/analyze.c
--- b/src/backend/parser/analyze.c
***************
*** 45,50 ****
--- 45,55 ----
  #include "utils/rel.h"


+ /* Use RAW_EXPRESSION_COVERAGE_TEST in COPY_PARSE_PLAN_TREES builds */
+ #ifdef COPY_PARSE_PLAN_TREES
+ #define RAW_EXPRESSION_COVERAGE_TEST
+ #endif
+
  /* Hook for plugins to get control at end of parse analysis */
  post_parse_analyze_hook_type post_parse_analyze_hook = NULL;

*************** static Query *transformCreateTableAsStmt
*** 74,79 ****
--- 79,87 ----
                             CreateTableAsStmt *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


  /*
*************** transformStmt(ParseState *pstate, Node *
*** 220,225 ****
--- 228,252 ----
  {
      Query       *result;

+     /*
+      * 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.
+      */
+ #ifdef RAW_EXPRESSION_COVERAGE_TEST
+     switch (nodeTag(parseTree))
+     {
+         case T_SelectStmt:
+         case T_InsertStmt:
+         case T_UpdateStmt:
+         case T_DeleteStmt:
+             (void) test_raw_expression_coverage(parseTree, NULL);
+             break;
+         default:
+             break;
+     }
+ #endif    /* RAW_EXPRESSION_COVERAGE_TEST */
+
      switch (nodeTag(parseTree))
      {
              /*
*************** applyLockingClause(Query *qry, Index rti
*** 2713,2715 ****
--- 2740,2764 ----
      rc->pushedDown = pushedDown;
      qry->rowMarks = lappend(qry->rowMarks, rc);
  }
+
+ /*
+  * Coverage testing for raw_expression_tree_walker().
+  *
+  * When enabled, we run raw_expression_tree_walker() over every DML statement
+  * submitted to parse analysis.  Without this provision, that function is only
+  * 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)
+ {
+     if (node == NULL)
+         return false;
+     return raw_expression_tree_walker(node,
+                                       test_raw_expression_coverage,
+                                       context);
+ }
+
+ #endif /* RAW_EXPRESSION_COVERAGE_TEST */