Re: Patch for 8.5, transformationHook

On Sun, Sep 13, 2009 at 10:32 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Aug 11, 2009 at 12:09 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> 2009/8/10 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>>> On Sun, Jul 26, 2009 at 9:29 AM, Pavel Stehule<pavel(dot)stehule(at)gmail(dot)com> wrote:
>>>>> new patch add new contrib "transformations" with three modules
>>>>> anotation, decode and json.
>>>
>>>> These are pretty good examples, but the whole thing still feels a bit
>>>> grotty to me.  The set of syntax transformations that can be performed
>>>> with a hook of this type is extremely limited - in particular, it's
>>>> the set of things where the parser thinks it's valid and that the
>>>> structure is reasonably similar to what you have in mind, but the
>>>> meaning is somewhat different.  The fact that two of your three
>>>> examples require your named and mixed parameters patch seems to me to
>>>> be evidence of that.
>>>
>>> I finally got around to looking at these examples, and I still don't
>>> find them especially compelling.  Both the decode and the json example
>>> could certainly be done with regular function definitions with no need
>>> for this hook.  The => to AS transformation maybe not, but so what?
>>> The reason we don't have that one in core is not technological.
>>>
>>> The really fundamental problem with this hook is that it can't do
>>> anything except create syntactic sugar, and a pretty darn narrow class
>>> of syntactic sugar at that.  Both the raw parse tree and the transformed
>>> tree still have to be valid within the core system's understanding.
>>> What's more, since there's no hook in ruleutils.c, what is going to come
>>> out of the system (when dumping, examining a view, etc) is the
>>> transformed expression --- so you aren't really hiding any complexity
>>> from the user, you're just providing a one-time shorthand that will be
>>> expanded into a notation he also has to be familiar with.
>>>
>>
>> I agree - so this could be a problem
>>
>>> Now you could argue that we've partly created that restriction by
>>> insisting that the hook be in transformFuncCall and not transformExpr.
>>> But that only restricts the subset of raw parse trees that you can play
>>> with; it doesn't change any of the other restrictions.
>>>
>>> Lastly, I don't think the problem of multiple hook users is as easily
>>> solved as Pavel claims.  These contrib modules certainly fail to solve
>>> it.  Try unloading (or re-LOADing) them in a different order than they
>>> were loaded.
>>>
>>
>> There are two possible solution
>>
>> a) all modules should be loaded only from configuration
>> b) modules should be loaded in transformation time - transformation of
>> functions should be substituted some registered function for some
>> functions. This little bit change sense of this patch. But it's enough
>> for use cases like DECODE, JSON, SOAP. It's mean one new column to
>> pg_proc - like protransformfunc.
>>
>> ???
>> Pavel
>>
>>> So on the whole I still think this is a solution looking for a problem,
>>> and that any problems it could solve are better solved elsewhere.
>
> I am in the process of looking through the patches to be assigned for
> the September CommitFest, and it seems to me that we really haven't
> made any progress here since the last CommitFest.  Jeff Davis provided
> a fairly good summary of the issues:
>
> http://archives.postgresql.org/message-id/1249784508.9256.892.camel@jdavis
>
> I don't think we really gain much by assigning yet another reviewer to
> this patch.  The patch is simple enough and doesn't really need any
> further code review AFAICS, but nobody except the patch author seems
> confident that this is all that useful.[1] I'm biased by the fact that
> I reviewed this patch and didn't particularly like it either, but I
> think we need more than to think about committing this in the face of
> Tom Lane's opinion (which I share, FWIW) that this is of very limited
> usefulness.
>
> ...Robert
>
> [1] Indeed, the few supportive responses were along the lines of "oh -
> this should help with X" to which the response was, in at least two
> cases, "well actually no it won't".

Based on the above reasoning, and hearing no contrary hue and cry from
the masses, I am marking this patch as rejected.

...Robert