Re: [HACKERS] [PATCH] Generic type subscription - Mailing list pgsql-hackers

Dmitry Dolgov <9erthalion6@gmail.com> writes:
> [ generic_type_subscription_v6.patch ]

Not too surprisingly, this doesn't apply anymore in the wake of commit
ea15e1867.  Could you rebase?  Changes for that should be pretty trivial
I'd expect.

I took an extremely quick look over the patch --- mostly looking
at the header file changes not the code --- and have a couple of
comments:

1. As I mentioned previously, it's a seriously bad idea that ArrayRef
is used for both array subscripting and array assignment.  Let's fix
that while we're at it, rather than setting that mistake in even more
stone by embedding it in a datatype extension API.

2. I'm not very pleased that the subscripting functions have signature
"subscripting(internal) returns internal"; we have more than enough of
those already, and each one is a hazard for plugging the wrong function
into the wrong place.  Worse yet, you're flat out lying about the number
of arguments that these functions actually expect to receive, which is
something that could get broken by any number of plausible future changes.
Can we arrange to do that differently?  I'd prefer something in which the
argument and result types are visibly connected to the actual datatypes
at hand, for instance  array_subscript(anyarray, internal) returns anyelement  array_assign(anyarray, internal,
anyelement)returns anyarray
 
where the "internal" argument is some representation of only the subscript
expressions.  This would allow CREATE TYPE to perform some amount of
checking that the right function(s) had been specified.  (If that means
we use two functions not one per datatype, that's fine with me.)  If that
seems impractical, let's at least pick a signature that doesn't conflict
with any other INTERNAL-using APIs, and preferably has some connection
to what the arguments really are.

3. The contents of ArrayRef are designed on the assumption that the same
typmod and collation values apply to both an array and its elements.
That's okay for standard arrays, but I do not think it holds for every
other container situation.  For example, hstore doesn't have collation
last I checked, but it would likely want to treat its element type as
being text, which does.  So this needs to be generalized.

4. It looks like your work on the node processing infrastructure has been
limited to s/ArrayRef/SubscriptingRef/g, but that's not nearly enough.
SubscriptingRef needs to be regarded as an opportunity to invoke a
user-defined function, which means that it now acts quite a bit like
FuncExpr.  For example, the function-to-be-invoked needs to be checked for
volatility, leakproofness, parallel safety, etc in operations that want to
know about such things.  So check_functions_in_node(), for one, needs to
consider SubscriptingRef, and really you'll have to look at everyplace
that deals with FuncExpr and see if it needs a case for SubscriptingRef
now.  I'd advise adding the OID of the subscripting function to
SubscriptingRef, so that those places don't need to do additional catalog
lookups to get it.

BTW, a different approach that might be worth considering is to say that
the nodetree representation of one of these things *is* a FuncExpr, and
the new magic thing is just that we invent a new CoercionForm value
which causes ruleutils.c to print the expression as a subscripting op.
I'm not quite convinced that that's a good idea --- a "coercion format"
that says "subscript" seems a bit weird --- but it would greatly reduce
the number of places you'd have to touch.
        regards, tom lane