Re: array support patch phase 1 patch - Mailing list pgsql-patches

Joe Conway <mail@joeconway.com> writes:
> This patch is starting to get large enough that it is a challenge to
> keep in sync with cvs, and it is reasonably complete as a package, so I
> was hoping it could be reviewed and committed as "array support phase
> 1".

I looked over this patch a little bit, mostly at the anyarray/anyelement
changes; I didn't study the ArrayExpr stuff (except to notice that yeah,
the grammar change is very ugly; can't it be made independent of the
number of levels?).

I like the anyarray/anyelement stuff with the exception of the
actualRetType field added to FunctionCallInfoData.  In the first place,
that's the wrong place for such data; FunctionCallInfoData should only
contain data that will change for each call, which resolved type won't.
This should go into FmgrInfo, perhaps, or maybe we could treat it as a
call-context item (though I think we might have conflicts with existing
uses of the context link).  A bigger gripe is that passing only
actualRetType isn't future-proof.  It doesn't help functions that take
anyelement but return a fixed type; they won't know what the argument type
is.  And it won't scale if we end up adding anyarray2/anyelement2/etc to
allow multiple generic parameter types; at most one of the actual
parameter types could be made visible to the function.  I see your concern
here, but I think the only adequate solution is to make sure the function
can learn the types of all its arguments, as well as the assigned return
type.  (Of course it could deduce the latter from the former, but we may
as well save it the effort, especially since it would have to repeat
parse-time catalog lookups in many interesting cases.)

A notion that I've toyed with more than once is to allow a function
to get at the parse tree representation for its call (ie, the FuncExpr
or OpExpr node).  If we did that, a function could learn all these
types by examining the parse tree, and it could learn other things
besides.  A disadvantage is that functions would have to be ready to
cope with all the wooliness of parse trees.  It'd probably be bright
to make some convenience functions "get my return type", "get type
of my n'th argument" to localize this logic as much as possible.

In short then, what about passing an Expr* link in FmgrInfo as a
substitute for actualRetType?  (An additional advantage of this way
is that it's obvious whether or not the information has been provided,
which it would not be for, say, functions invoked via DirectFunctionCallN.
The patch as it stands fails silently if a polymorphic function is called
from a call site that doesn't provide the needed info.)

            regards, tom lane