Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> Here's the patch with my edits (more comments and the while/if change).
A couple minor thoughts:
* I dislike using castNode() in places where the code has just explicitly
verified the node type, which is true in both places where you used it
here. The assertion is just bulking the code up to no purpose, and it
creates an unnecessary discrepancy between older and newer code.
* As you have it here, a construct such as
ConvertRowtypeExpr(ConvertRowtypeExpr(ConvertRowtypeExpr(Const)))
will laboriously perform each of the intermediate steps, which seems
like exactly the case we're trying to prevent at runtime. I wonder
whether it is worth stripping off ConvertRowtypeExpr's before the
recursive eval_const_expressions_mutator call to prevent that.
You'd still want the check after the call, to handle situations where
something more complex got simplified to a ConvertRowtypeExpr; and this
would also complicate getting the convertformat right. So perhaps it's
not worth the trouble, but I thought I'd mention it.
* I find the hardwired logic about how to merge distinct convertformat
values a bit troublesome. Maybe use Min() instead? Although there
is not currently any expectation about the ordering of that enum ...
regards, tom lane
