Re: Identifying no-op length coercions

On Sat, Jun 11, 2011 at 02:11:55PM -0400, Tom Lane wrote:
> Noah Misch <noah(at)leadboat(dot)com> writes:
> > We originally discussed having the transform function take and return Expr
> > nodes. It turns out that simplify_function() does not have the Expr, probably
> > because the particular choice of node type among the many that can convey a
> > function call does not matter to it. The same should be true of a transform
> > function -- it should do the same thing whether the subject call arrives from
> > a FuncExpr or an OpExpr. Therefore, I changed the transform function
> > signature to "Expr *protransform(List *args)".
>
> That seems like the wrong thing. For one thing, it makes it quite
> impossible to share one transform support function among multiple
> functions/operators, since it won't know which one the argument list
> is for. More generally, I foresee the transform needing to know
> most of the other information that might be in the FuncExpr or OpExpr.
> It certainly would need access to the collation, and it would probably
> like to be able to copy the parse location to whatever it outputs,
> and so forth and so on. So I really think passing the node to the
> function is the most future-proof way to do it. If that means you
> need to rejigger things a bit in clauses.c, so be it.

Good points. I'm thinking, then, add an Expr argument to simplify_function()
and have the CoerceViaIO branch of eval_const_expressions_mutator() pass NULL
for both its simplify_function() calls. If simplify_function() gets a NULL Expr
for a function that has a protransform, synthesize a FuncExpr based on its other
arguments before calling the transform function. Seem reasonable? Granted,
that would be dead code until someone applies a transform function to a type I/O
function, which could easily never happen. Perhaps just ignore the transform
function when we started with a CoerceViaIO node?

> > The large pg_proc.h diff mostly just adds protransform = 0 to every function.
> > Due to the resulting patch size, I've compressed it. There are new/otherwise
> > changed DATA lines for OIDs 669 and 3097 only.
>
> The chances that this part will still apply cleanly by the time anyone
> gets around to committing it are nil. I'd suggest you break it into two
> separate patches, one that modifies the existing lines to add the
> protransform = 0 column and then one to apply the remaining deltas in
> the file. I envision the eventual committer doing the first step
> on-the-fly (perhaps with an emacs macro, at least that's the way I
> usually do it) and then wanting a patchable diff for the rest. Or if
> you wanted to be really helpful, you could provide a throwaway perl
> script to do the modifications of the existing lines ...

That would be better; I'll do it for the next version.

> I haven't read the actual patch, these are just some quick reactions
> to your description.

Thanks,
nm