Re: [PATCH] Add transforms feature

2015-03-22 5:45 GMT+01:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

>
>
> 2015-03-22 3:55 GMT+01:00 Peter Eisentraut <peter_e(at)gmx(dot)net>:
>
>> Here is an updated patch.
>>
>> On 3/17/15 1:11 AM, Pavel Stehule wrote:
>> > 2015-03-17 2:51 GMT+01:00 Peter Eisentraut <peter_e(at)gmx(dot)net
>> > <mailto:peter_e(at)gmx(dot)net>>:
>> >
>> > On 3/12/15 8:12 AM, Pavel Stehule wrote:
>> > > 1. fix missing semicolon pg_proc.h
>> > >
>> > > Oid protrftypes[1]; /* types for which to
>> apply
>> > > transforms */
>> >
>> > Darn, I thought I had fixed that.
>>
>> Fixed.
>>
>> > > 2. strange load lib by in sql scripts:
>> > >
>> > > DO '' LANGUAGE plperl;
>> > > SELECT NULL::hstore;
>> > >
>> > > use load plperl; load hstore; instead
>> >
>> > OK
>>
>> The reason I had actually not used LOAD is that LOAD requires a file
>> name, and the file name of those extensions is an implementation detail.
>> So it is less of a violation to just execute something from those
>> modules rather than reach in and deal with the file directly.
>>
>> It's not terribly pretty either way, I admit. A proper fix would be to
>> switch to lazy symbol resolution, but that would be a much bigger change.
>>
>> > > 3. missing documentation for new contrib modules,
>> >
>> > OK
>>
>> They actually are documented as part of the hstore and ltree modules
>> already.
>>
>> > > 4. pg_dump - wrong comment
>> > >
>> > > +<-----><------>/*
>> > > +<-----><------> * protrftypes was added at v9.4
>> > > +<-----><------> */
>> >
>> > OK
>>
>> Fixed.
>>
>> > > 4. Why guc-use-transforms? Is there some possible negative side
>> effect
>> > > of transformations, so we have to disable it? If somebody don't
>> would to
>> > > use some transformations, then he should not to install some
>> specific
>> > > transformation.
>> >
>> > Well, there was extensive discussion last time around where people
>> > disagreed with that assertion.
>> >
>> >
>> > I don't like it, but I can accept it - it should not to impact a
>> > functionality.
>>
>> Removed.
>>
>> > > 5. I don't understand to motivation for introduction of
>> protrftypes in
>> > > pg_proc and TRANSFORM clause for CREATE FUNCTION - it is not
>> clean from
>> > > documentation, and examples in contribs works without it. Is it
>> this
>> > > functionality really necessary? Missing tests, missing examples.
>> >
>> > Again, this came out from the last round of discussion that people
>> > wanted to select which transforms to use and that the function
>> needs to
>> > remember that choice, so it doesn't depend on whether a transform
>> > happens to be installed or not. Also, it's in the SQL standard
>> that way
>> > (by analogy).
>> >
>> >
>> > I am sorry, I didn't discuss this topic and I don't agree so it is good
>> > idea. I looked to standard, and I found CREATE TRANSFORM part there. But
>> > nothing else.
>> >
>> > Personally I am thinking, so it is terrible wrong idea, unclean,
>> > redundant. If we define TRANSFORM, then we should to use it. Not prepare
>> > bypass in same moment.
>> >
>> > Can be it faster, safer with it? I don't think.
>>
>> Well, I don't think there is any point in reopening this discussion.
>> This is a safety net of sorts that people wanted. You can argue that it
>> would be more fun without it, but nobody else would agree. There is
>> really no harm in keeping it. All the function lookup is mostly cached
>> anyway. The only time this is really important is for pg_dump to be
>> able to accurately restore function behavior.
>>
>
> 1. It add attribute to pg_proc, so impact is not minimal
>
> 2. Minimally it is not tested - there are no any test for this
> functionality
>

I am sorry, there is tests

>
> 3. I'll reread a discuss about this design - Now I am thinking so this
> duality (in design) is wrong - worse in relatively critical part of
> Postgres.
>
> I can mark this patch as "ready for commiter" with objection - It is task
> for commiter, who have to decide.
>
> Regards
>
> Pavel
>
>