Peter Geoghegan <p...@heroku.com> writes: > On Mon, May 23, 2016 at 12:48 PM, Tom Lane <t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers