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