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