Re: Patch for 8.5, transformationHook

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".