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

On Sat, May 21, 2016 at 5:03 PM, Peter Geoghegan <pg@heroku.com> wrote:
> On Sat, May 21, 2016 at 4:28 PM,  <thomas.alton@gmail.com> wrote:
>> ERROR:  XX000: unrecognized node type: 920
>> LOCATION:  raw_expression_tree_walker, nodeFuncs.c:3410
>>
>> I expected the query run successfully and return one row with 'a'.
>
> This is clearly a bug.
>
> 920 is IndexElem, which can appear in a raw parse tree in 9.5, due to
> ON CONFLICT's use of inference reusing what was previously only used
> for CREATE INDEX. (It does not make it into the post-parse analysis
> tree, though).

Attached patch fixes this issue by adding the appropriate
raw_expression_tree_walker handler. This isn't the first quasi-utility
node to need such a handler, so the fix is simple.


--
Peter Geoghegan

Peter Geoghegan <pg@heroku.com> writes:
> Attached patch fixes this issue by adding the appropriate
> raw_expression_tree_walker handler. This isn't the first quasi-utility
> node to need such a handler, so the fix is simple.

It seems unlikely to me that recursing into the name lists is helpful
here: those are not going to contain any data that is interpretable
without context.  Did you have a reason to do that?
        regards, tom lane

On Mon, May 23, 2016 at 12:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> It seems unlikely to me that recursing into the name lists is helpful
> here: those are not going to contain any data that is interpretable
> without context.  Did you have a reason to do that?

I saw no reason to avoid the extra cycles. A noticeable omission has a
cost: it gets noticed. Given this code path is likely to hardly ever
be hit, this mechanical approach seemed best. That's all.

-- 
Peter Geoghegan

Peter Geoghegan <pg@heroku.com> writes:
> On Mon, May 23, 2016 at 12:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> It seems unlikely to me that recursing into the name lists is helpful
>> here: those are not going to contain any data that is interpretable
>> without context.  Did you have a reason to do that?

> I saw no reason to avoid the extra cycles. A noticeable omission has a
> cost: it gets noticed. Given this code path is likely to hardly ever
> be hit, this mechanical approach seemed best. That's all.

I agree that performance isn't much of a concern, but code bloat and
inconsistency with other cases are valid concerns.  That function does
not recurse into name lists in, for example, the A_Expr and FuncCall
cases.

Also, related to this complaint though not this patch, it's disturbing
that this oversight wasn't detected long ago.  My first thought was to add
some conditionally-compiled debugging code that just performs a no-op
traverse of every raw parse tree produced by the grammar.  However that
doesn't work because raw_expression_tree_walker doesn't promise to support
everything, only DML statements.  Thoughts?
        regards, tom lane

On Mon, May 23, 2016 at 12:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I saw no reason to avoid the extra cycles. A noticeable omission has a
>> cost: it gets noticed. Given this code path is likely to hardly ever
>> be hit, this mechanical approach seemed best. That's all.
>
> I agree that performance isn't much of a concern, but code bloat and
> inconsistency with other cases are valid concerns.  That function does
> not recurse into name lists in, for example, the A_Expr and FuncCall
> cases.

Okay. Noted.

> Also, related to this complaint though not this patch, it's disturbing
> that this oversight wasn't detected long ago.  My first thought was to add
> some conditionally-compiled debugging code that just performs a no-op
> traverse of every raw parse tree produced by the grammar.  However that
> doesn't work because raw_expression_tree_walker doesn't promise to support
> everything, only DML statements.  Thoughts?

Why couldn't the debug code be executed conditionally, too? Since
raw_expression_tree_walker() promises to work for "SelectStmt and
everything that could appear under it", I don't think it would be
difficult to find a choke point for that. Perhaps there is some
subtlety I've missed, since what I propose seems too obvious. FWIW, I
don't think it would much matter if this debug code was not reliably
executed for every possible SelectStmt. Just limiting it to top-level
SelectStmts would have easily caught this bug.

-- 
Peter Geoghegan

Peter Geoghegan <pg@heroku.com> writes:
> On Mon, May 23, 2016 at 12:48 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Also, related to this complaint though not this patch, it's disturbing
>> that this oversight wasn't detected long ago.  My first thought was to add
>> some conditionally-compiled debugging code that just performs a no-op
>> traverse of every raw parse tree produced by the grammar.  However that
>> doesn't work because raw_expression_tree_walker doesn't promise to support
>> everything, only DML statements.  Thoughts?

> Why couldn't the debug code be executed conditionally, too? Since
> raw_expression_tree_walker() promises to work for "SelectStmt and
> everything that could appear under it", I don't think it would be
> difficult to find a choke point for that. Perhaps there is some
> subtlety I've missed, since what I propose seems too obvious. FWIW, I
> don't think it would much matter if this debug code was not reliably
> executed for every possible SelectStmt. Just limiting it to top-level
> SelectStmts would have easily caught this bug.

Um, I think not --- you need the case cited by the OP, namely an INSERT
ON CONFLICT in a CTE in a SelectStmt.  If we'd had any of those in the
regression tests, we'd have found the bug, but we don't; and it's not
an obvious combination to try.  We would have found it if there were
a reason to run raw_expression_tree_walker() on bare InsertStmts,
but currently there is not.

Possibly we could get adequate coverage by inserting the debug code
into the first four switch cases in transformStmt().

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.
        regards, tom lane

On Mon, May 23, 2016 at 1:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Um, I think not --- you need the case cited by the OP, namely an INSERT
> ON CONFLICT in a CTE in a SelectStmt.  If we'd had any of those in the
> regression tests, we'd have found the bug, but we don't; and it's not
> an obvious combination to try.  We would have found it if there were
> a reason to run raw_expression_tree_walker() on bare InsertStmts,
> but currently there is not.

I don't follow. If we had coverage of that in the regression tests,
then that would have shown the bug, of course. It would have shown the
bug without any new debugging infrastructure being required (or being
enabled).

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.

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

-- 
Peter Geoghegan

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 */

I wrote:
> Peter Geoghegan <pg@heroku.com> writes:
>>> 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.

And after further thought, I decided that that was penny-wise and
pound-foolish; it's more readable if the #define is just an independent
pg_config_manual.h entry.  The only work it'd save is the need to update
a buildfarm animal or two to add the new #define, which is not exactly
a huge cost.
        regards, tom lane

On Mon, May 23, 2016 at 4:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> And after further thought, I decided that that was penny-wise and
> pound-foolish; it's more readable if the #define is just an independent
> pg_config_manual.h entry.  The only work it'd save is the need to update
> a buildfarm animal or two to add the new #define, which is not exactly
> a huge cost.

You also have to be aware of the new thing, which a lot of hackers
will not be, if awareness of COPY_PARSE_PLAN_TREES is anything to go
by.

-- 
Peter Geoghegan

Peter Geoghegan <pg@heroku.com> writes:
> On Mon, May 23, 2016 at 4:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> And after further thought, I decided that that was penny-wise and
>> pound-foolish; it's more readable if the #define is just an independent
>> pg_config_manual.h entry.  The only work it'd save is the need to update
>> a buildfarm animal or two to add the new #define, which is not exactly
>> a huge cost.

> You also have to be aware of the new thing, which a lot of hackers
> will not be, if awareness of COPY_PARSE_PLAN_TREES is anything to go
> by.

I doubt that anybody ever enables that in manual builds anyway.
The important thing is to have at least one buildfarm animal using it,
which there soon will be.
        regards, tom lane

On Mon, May 23, 2016 at 4:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> You also have to be aware of the new thing, which a lot of hackers
>> will not be, if awareness of COPY_PARSE_PLAN_TREES is anything to go
>> by.
>
> I doubt that anybody ever enables that in manual builds anyway.
> The important thing is to have at least one buildfarm animal using it,
> which there soon will be.

I manually enable it sometimes. And, I've pointed out bugs that that
would have caught to other hackers during review a couple of times.

That's my view. I'm not going to make a fuss about it.

-- 
Peter Geoghegan