On Sat, Jun 11, 2011 at 02:11:55PM -0400, Tom Lane wrote: > Noah Misch <n...@leadboat.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
