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