Re: transforms - Mailing list pgsql-hackers

I haven't had the time to finish all the issues with this, but I want to
keep the discussion going in the meantime and provide an updated patch.

On mån, 2012-06-18 at 17:33 +0200, Andres Freund wrote:

> Cursory code review:
> * pstrndup already exists as pnstrdup (hstore_plperl.c)

Fixed.

> * PyString_FromStringAndSize return value not decreffed? PyDict_SetItem
> doesn't steal references

Fixed.

> * In plpython_to_hstore I would move the 'pairs' and some related variables in
> the PG_TRY block, so the reader doesn't need to check whether it should be
> volatile
> * In the same function items needs to be volatile to fit into longjmp
> semantics

I'll recheck that later.

> * I don't think recording dependencies on transforms used when creating
> functions is a good idea as the transform might get created after the
> functions already exists. That seems to be a pretty confusing behaviour.

We need the dependencies, because otherwise dropping a transform would
break or silently alter the behavior of functions that depend on it.
That sounds like my worst nightmare, thinking of some applications that
would be affected by that.  But your point is a good one.  I think this
could be addressed by prohibiting the creation of a transform that
affects functions that already exist.

Because the legacy behavior of PL implementations of defaulting to a
string representation conversion, we would technically need a dependency
on the absence of a transform object to make this airtight.  In the far
future, I could imagine removing this default behavior, meaning you
couldn't create the function if no suitable transforms exist for all
argument and return types.

> * I forsee the need for multiple transforms for the same type/language pair to
> coexist. The user would need to manually "choose"/"call" the transform in that
> case. This currently isn't easily possible...

I thought about this briefly at the beginning, but see under "worst
nightmare" above.  Also, having a configuration setting for this or
something would prevent any PL functions from being immutable.  We don't
allow multiple casts or multiple in/out functions either, which are
related concepts.  If you want different behavior, you should define a
different type or different language.

> > *) No psql backslash commands yet.  Could be added.
> Doesn't really seem necessary to me. Not many people will need to look at this
> and the list of commands already is rather long.

I'm going to leave this out for now.

> > *) Permissions: Transforms don't have owners, a bit like casts.
> > Currently, you are allowed to drop a transform if you own both the type
> > and the language.  That might be too strict, maybe own the type and have
> > privileges on the language would be enough.
> Seems sensible enough to me.

I have made this change.

> > *) There is currently some syntax schizophrenia.  The grammar accepts
> >
> > CREATE TRANSFORM FOR hstore LANGUAGE plperl (
> >     FROM SQL WITH hstore_to_plperl,
> >     TO SQL WITH plperl_to_hstore
> > );
> >
> > but pg_dump produces
> >
> > CREATE TRANSFORM FOR hstore LANGUAGE plperl (
> >     FROM SQL WITH hstore_to_plperl(hstore),
> >     TO SQL WITH plperl_to_hstore(internal)
> > );
> >
> > The SQL standard allows both.  (In the same way that it allows 'DROP
> > FUNCTION foo' without arguments, if it is not ambigious.)  Precedent is
> > that CREATE CAST requires arguments, but CREATE LANGUAGE does not.
> I don't find that problematic personally.

I have fixed the syntax to include argument types, so the dump output
and the input grammar is consistent.


Other changes:

- Fixed ecpg grammar to work again with this.

- Changed extension naming to be more consistent.

- Build additional contrib modules conditionally depending on whether
--with-perl or --with-python were configured.  (complaint from Jeff
Janes)

- Fixed Python 3.


Things I still want to do:

- Refactor the regression test framework for Python 3 so that contrib
modules or external extensions don't have to repeat the magic in
src/pl/plpython/Makefile.  (Python 3 with hstore_plpython and
ltree_plpython works, but the tests don't run.)

- Refactor pyobject_to_string(), which is currently kind of copied and
pasted from plpython, but should instead be exported by plpython in some
suitable way.

- Refactor shared library building so that I can have, say, hstore,
hstore_plperl, and hstore_plpython in one directory, rather than in
three.  The reason being, if someone has a new type in a repository on
github or something, I don't want them to have to make three separate
projects or some crazy subdirectory structure in order to add some PL
support for their type.  This will require some deep Makefile.shlib
hacking, but I think it's worth trying to make this simple(r).


So, it's quite likely that this patch won't get finished in this commit
fest.