>>>>> "Tom" == Tom Lane <t...@sss.pgh.pa.us> writes:
Tom> +1 for not ignoring rowMarks, but this patch seems like a hack to Tom> me. Why didn't you just add RowMarkClause as one of the known Tom> alternative expression node types? >> Because it's not an expression and nothing anywhere else in the >> backend treats it as such? Tom> Places such as outfuncs.c and copyfuncs.c don't draw a Tom> distinction, and I don't see why pg_stat_statements should either. Tom> It would just be one more place we'd have to fix if we ever allow Tom> RowMarkClause in other places --- or, perhaps more realistically, Tom> if the structure of Query.rowMarks becomes more complex than "flat Tom> list of RowMarkClauses". None of these possibilities seem even slightly realistic. Tom> The other places you mention generally have some specific semantic Tom> knowledge about rowmarks, and would have to be touched anyway if Tom> any changes like that happen. But the jumbling code in Tom> pg_stat_statements has no knowledge of any of that, it's just Tom> interested in traversing the tree. So I'd put it on the same Tom> semantic level as outfuncs.c. JumbleExpr's header comments specifically say it's intended to handle the same kinds of things as expression_tree_walker (barring planner-only constructs), and RowMarkClause is not one of those things. If it had been named JumbleNodeTree or whatever then I might agree with you, but right now the organization of the code is more or less a straight parallel to query_tree_walker / range_table_walker / expression_tree_walker, and it seems to me that adding RowMarkClause as an "expression" node would just distort that parallel for no adequate reason. -- Andrew (irc:RhodiumToad)
