Re: [PATCH] Add transforms feature

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

ok, please, can comment the reason in test. little bit more verbose than
"make sure the prerequisite libraries are loaded". There is not clean, why
"LOAD" should not be used.

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

So I reread discussion about this topic and I can see some benefits of it.
Still - what I dislike is the behave of TRANSFORM ALL. The fact, so newly
installed transformations are not used for registered functions (and
reregistration is needed) is unhappy. I understand, so it can depends on
implementation requirements.

Isn't better doesn't support "TRANSFORM ALL" clause? If somebody would to
use transformations - then he have to explicitly enable it by "TRANSFORM
FOR TYPE" ? It is safe and without possible user unexpectations.

Small issue - explicitly setted transformation types should be visible in
\sf and \df+ commands.

Regards

Pavel