From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Peter Geoghegan <pg(at)heroku(dot)com> |
Cc: | Thomas Alton <thomas(dot)alton(at)gmail(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Re: [BUGS] BUG #14153: Unrecognized node type error when upsert is present in recursive CTE |
Date: | 2016-05-23 20:55:49 |
Message-ID: | 22866.1464036949@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-bugs pgsql-hackers |
Peter Geoghegan <pg(at)heroku(dot)com> writes:
> On Mon, May 23, 2016 at 12:48 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Geoghegan | 2016-05-23 21:07:20 | Re: Re: [BUGS] BUG #14153: Unrecognized node type error when upsert is present in recursive CTE |
Previous Message | Peter Geoghegan | 2016-05-23 20:43:49 | Re: Re: [BUGS] BUG #14153: Unrecognized node type error when upsert is present in recursive CTE |
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Geoghegan | 2016-05-23 21:07:20 | Re: Re: [BUGS] BUG #14153: Unrecognized node type error when upsert is present in recursive CTE |
Previous Message | Peter Geoghegan | 2016-05-23 20:43:49 | Re: Re: [BUGS] BUG #14153: Unrecognized node type error when upsert is present in recursive CTE |