Re: transforms

Here is my first patch for the transforms feature. This is a mechanism
to adapt data types to procedural languages. The previous proposal was
here: http://archives.postgresql.org/pgsql-hackers/2012-05/msg00728.php

At the surface, this contains:

- New catalog pg_transform

- CREATE/DROP TRANSFORM

As proof of concepts and hopefully useful applications to-be, I have
included transforms for

- PL/Perl - hstore
- PL/Python - hstore
- PL/Python - ltree

These, along with the documentation for the above-mentioned DDL commands
and catalog can serve as explanations for reviewers.

There are numerous remaining discussion points and possible work items:

*) Currently, each of the above provided transforms is put into a
separate subdirectory under contrib. This might be OK, but it is mainly
because we cannot build more than one MODULE_big per directory. Fixing
that might be more work than it's worth, but if we want to provide more
of these in contrib, it might get crowded.

*) Since we have separate extensions for plperl and plperlu, and also
for plpython2u and plpython3u, we need one extension for adapting each
of these to a given type. You can see under contrib/hstore_plperl what
this looks like. Maybe this isn't fixable or worth fixing.

(With the directory and packaging not finalized, I haven't included any
documentation for these contrib modules yet.)

*) No psql backslash commands yet. Could be added.

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

*) If we want to offer the ability to write transforms to end users,
then we need to install all the header files for the languages and the
types. This actually worked quite well; including hstore.h and plperl.h
for example, gave you what you needed. In other cases, some headers
might need cleaning up. Also, some magic from the plperl and plpython
build systems might need to be exposed, for example to find the header
files. See existing modules for how this currently looks.

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

*) The issue of how to handle arguments of type "internal" was already
discussed at
http://archives.postgresql.org/pgsql-hackers/2012-05/msg00857.php and
following. I have adopted the suggestion there to prohibit calling
functions with arguments of type "internal", but that is probably full
of holes and needs to be reviewed carefully.

*) I'm not very familiar with the Perl C API, so the hstore_plperl
implementation is probably full of memory leaks and weirdnesses. It
needs some help from a PL/Perl expert. (The PL/Python stuff is
hopefully better.)

*) ltree_plpython lacks the transform function from python to ltree,
because of laziness and lack of clear documentation on how to construct
ltree values.

*) I just noticed that the grammar changes broke ECPG. I'll look into
that.

*) The regression tests for the core CREATE/DROP TRANSFORM functionality
is in contrib/hstore_plpython, because the core has no suitable language
available. It's a bit ugly, but I don't know of a better way short of
implementing a fake language for regression testing only.

On Thu, Jun 14, 2012 at 3:42 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> Here is my first patch for the transforms feature.  This is a mechanism
> to adapt data types to procedural languages.  The previous proposal was
> here: http://archives.postgresql.org/pgsql-hackers/2012-05/msg00728.php

When I apply this and go to contrib and do make check, I get:

In file included from hstore_plperl.c:4:0:
../../src/pl/plperl/plperl.h:49:20: fatal error: EXTERN.h: No such
file or directory

Cheers,

Jeff

On Sat, Jun 16, 2012 at 7:15 PM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
> On Thu, Jun 14, 2012 at 3:42 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>> Here is my first patch for the transforms feature.  This is a mechanism
>> to adapt data types to procedural languages.  The previous proposal was
>> here: http://archives.postgresql.org/pgsql-hackers/2012-05/msg00728.php
>
> When I apply this and go to contrib and do make check, I get:
>
> In file included from hstore_plperl.c:4:0:
> ../../src/pl/plperl/plperl.h:49:20: fatal error: EXTERN.h: No such
> file or directory

Ah, that went away when I remembered to ./configure --with-perl

Although the error message seem less than optimal. Aren't test
usually "skipped" when they are missing prerequisites in the config?

Cheers,

Jeff

Hi Peter,

On Friday, June 15, 2012 12:42:12 AM Peter Eisentraut wrote:
> Here is my first patch for the transforms feature. This is a mechanism
> to adapt data types to procedural languages. The previous proposal was
> here: http://archives.postgresql.org/pgsql-hackers/2012-05/msg00728.php
>
> At the surface, this contains:
>
> - New catalog pg_transform
>
> - CREATE/DROP TRANSFORM
Cursory code review:
* pstrndup already exists as pnstrdup (hstore_plperl.c)
* PyString_FromStringAndSize return value not decreffed? PyDict_SetItem
doesn't steal references
* 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 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.
* I am a bit wary that some place can be used to call functions accepting
INTERNAL indirectly, couldn't think of any immediately though. Will look into
this a bit, but I am not experienced in the area, so ...
* 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...

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

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

> *) If we want to offer the ability to write transforms to end users,
> then we need to install all the header files for the languages and the
> types. This actually worked quite well; including hstore.h and plperl.h
> for example, gave you what you needed. In other cases, some headers
> might need cleaning up. Also, some magic from the plperl and plpython
> build systems might need to be exposed, for example to find the header
> files. See existing modules for how this currently looks.
Doesn't look to bad to me. I dont't know how this could be made easier.

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

Greetings,

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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.

Here is an updated patch for the transforms feature. The previous
discussion was here:
http://www.postgresql.org/message-id/1339713732.11971.79.camel@vanquo.pezone.net

Old news: At the surface, this contains:

- New catalog pg_transform

- CREATE/DROP TRANSFORM

As proofs of concept and useful applications, I have included transforms for

- PL/Perl - hstore
- PL/Python - hstore
- PL/Python - ltree

New news: I have tried to address all issues raised during previous
reviews.

The regression tests for hstore_plpython and ltree_plpython don't pass
with Python 3. This needs to be fixed, obviously, but it's an issue
unrelated to the core functionality.

As a demo that this can be used externally as well, I have written a
proof-of-concept transform between plpython and the mpz type in the pgmp
extension (multi-precision arithmetic extension):
https://github.com/petere/pgmp/tree/transforms/plpython

Note: There was a lot of churn lately in src/backend/commands/, and I
had to fix merge conflicts several times, so you need source as of about
right now to apply this patch cleanly.

Side issue/peculiarity: I had to change some shared library linking
flags for OS X. Ordinarily, when linking a dynamically loadable module
on OS X, if there are unresolved symbols, it's an error. But for
example, when we link hstore_plpython, we expect some symbols to be
resolved in hstore.so or plpythonu.so, so we need to ignore unresolved
symbols. This change basically just makes OS X behave like other
platforms in this regard. There might be other portability issues on
other platforms.

Peter,

I tried this patch out. I didn't get as far as testing the
functionality because of errors.

configure/make/make install/make check worked, without asserts. I
believe DF found some errors when he enabled assertions.

When I tried to install the actual transform extensions, though, it blew
up with symbol errors:

transforms=# create extension hstore
transforms-# ;
CREATE EXTENSION

transforms=# create extension ltree
transforms-# ;
CREATE EXTENSION

transforms=# create extension plperl
transforms-# ;
CREATE EXTENSION

transforms=# create extension plpythonu
transforms-# ;
CREATE EXTENSION

transforms=# create extension ltree_plpythonu;
ERROR: could not load library
"/home/josh/pg93/lib/postgresql/ltree_plpython2.so":
/home/josh/pg93/lib/postgresql/ltree_plpython2.so: undefined symbol:
PyList_New
STATEMENT: create extension ltree_plpythonu;

transforms=# create extension hstore_plperl;
ERROR: could not load library
"/home/josh/pg93/lib/postgresql/hstore_plperl.so":
/home/josh/pg93/lib/postgresql/hstore_plperl.so: undefined symbol:
hstoreUniquePairs
STATEMENT: create extension hstore_plperl;

This surprised me, because "make check" for the extensions passed fine.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Peter, all:

So in addition to the bugs I encountered in getting this patch to work,
we have a design issue to work out: how to load all of the transform
functions. Each transform depends on an optional datatype (like hstore)
and an optional external language (like plperl), which can be loaded
into the database in any order.

Currently Peter is punting (as is proper in a new patch) by having a
separate extension for each combination (hstore/plperl, hstore/plpython,
ltree/plpython, etc.). This is obviously not a maintainable approach in
the long run.

In an ideal world, transformations would load automatically as soon as
all of their prerequisites load. For example, if you load hstore and
plpythonU, then hstore_plpython should automatically load whenever the
second extension is loaded.

However, we currently have no mechanism for this, especially since we
don't know what order the user will load the two extensions in.
Realistically, this would require enhancing the extension mechanism to
track transformations.

Discuss?

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

> transforms=# create extension hstore_plperl;
> ERROR: could not load library
> "/home/josh/pg93/lib/postgresql/hstore_plperl.so":
> /home/josh/pg93/lib/postgresql/hstore_plperl.so: undefined symbol:
> hstoreUniquePairs
> STATEMENT: create extension hstore_plperl;
>
> This surprised me, because "make check" for the extensions passed fine.

Oh, this is on Ubuntu 12.10, not OSX. So possibly the fixes you made
to fix linking on OSX broke other platforms.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 13-03-03 06:15 PM, Josh Berkus wrote:
>> transforms=# create extension hstore_plperl;
>> ERROR: could not load library
>> "/home/josh/pg93/lib/postgresql/hstore_plperl.so":
>> /home/josh/pg93/lib/postgresql/hstore_plperl.so: undefined symbol:
>> hstoreUniquePairs
>> STATEMENT: create extension hstore_plperl;
>>
>> This surprised me, because "make check" for the extensions passed fine.
> Oh, this is on Ubuntu 12.10, not OSX. So possibly the fixes you made
> to fix linking on OSX broke other platforms.
>
>

This (creating the extensions) works fine for me on a Ubuntu 10.x system

template1=# create database test;
CREATE DATABASE
template1=# \c test
You are now connected to database "test" as user "ssinger".
test=# create extension hstore;
CREATE EXTENSION
test=# create extension hstore_plpythonu;
ERROR: required extension "plpythonu" is not installed
STATEMENT: create extension hstore_plpythonu;
ERROR: required extension "plpythonu" is not installed
test=# create extension plpythonu;
CREATE EXTENSION
test=# create extension hstore_plpythonu;
CREATE EXTENSION
test=#
test=# create extension plperl;
CREATE EXTENSION
test=# create extension hstore_plperl;
CREATE EXTENSION
test=# create extension plperlu;
CREATE EXTENSION
test=# create extension hstore_plperlu;
CREATE EXTENSION
test=#

> This (creating the extensions) works fine for me on a Ubuntu 10.x system
>

Hmmmm. So I wiped everything out and retried this with clean
directories, and it worked fine. Now I'm not sure how I got the error
in the first place.

Anyway, the feature seems to work as expected:

create function look_up_hstore (
some_hs hstore, which_key text )
returns text
language plpythonu as $f$
return some_hs[which_key]
$f$;

postgres=# create table storit ( it hstore );
CREATE TABLE

postgres=# insert into storit values ( 'f => josh,l=>berkus'
),('f=>jan,l=>wieck'),('f=>tom,l=>lane');
INSERT 0 3
postgres=# select look_up_hstore(it,'f') from storit;
look_up_hstore
----------------
josh
jan
tom
(3 rows)

I'll have to think of other ways to test it out, but right now it seems
to pass muster. I was a little unclear on what Python data structure an
ltree should produce.

Now if only we can work out the combinatorics issue ...

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 13-03-03 08:13 PM, Josh Berkus wrote:
>> This (creating the extensions) works fine for me on a Ubuntu 10.x system
>>

> Now if only we can work out the combinatorics issue ...
>

The plpython2u extensions worked fine but I haven't been able to get
this to work with plpython3u (python 3.1).

create extension hstore_plpython3u;
ERROR: could not load library
"/usr/local/pgsql93git/lib/hstore_plpython3.so":
/usr/local/pgsql93git/lib/hstore_plpython3.so: undefined symbol:
_Py_NoneStruct

Peter mentioned in the submission that the unit tests don't pass with
python3, it doesn't work for meoutside the tests either.

Steve

> Peter mentioned in the submission that the unit tests don't pass with
> python3, it doesn't work for meoutside the tests either.

Also, note that the feature is the concept of Transforms, not the
individual transforms which Peter provides. The idea is to enable a new
kind of extension.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 3/3/13 6:14 PM, Josh Berkus wrote:
> Currently Peter is punting (as is proper in a new patch) by having a
> separate extension for each combination (hstore/plperl, hstore/plpython,
> ltree/plpython, etc.). This is obviously not a maintainable approach in
> the long run.

There is also a {Perl, Python, Ruby, etc.} binding for {libpq, libmysql,
libpng, libyaml, libssl, libgmp, etc.}, each as a separately
downloadable and buildable package. I don't think anyone has ever
seriously considered a system by which if, say, you have Python and
libyaml installed, pyyaml magically appears. Might be nice, but maybe
not. The solution, in practice, is that you download pyyaml, and it
pulls in any required dependencies. This would work the same way.

Peter,

> There is also a {Perl, Python, Ruby, etc.} binding for {libpq, libmysql,
> libpng, libyaml, libssl, libgmp, etc.}, each as a separately
> downloadable and buildable package. I don't think anyone has ever
> seriously considered a system by which if, say, you have Python and
> libyaml installed, pyyaml magically appears. Might be nice, but maybe
> not. The solution, in practice, is that you download pyyaml, and it
> pulls in any required dependencies. This would work the same way.

That would be good too, although we don't currently have that
capability; if I try to install hstore_plpython without plpython
installed, it just errors out. Aside from that, what can we reasonably
do for 9.3 to get this feature in?

Maybe we add a transforms/ subdirectory of contrib, so that it can be as
crowded as we want? Or put the transforms on PGXN for now?

I want to see this feature go in so that the community starts writing
transforms this year instead of next year.

BTW, dependancies seem to be working OK for DROP:

postgres=# drop extension plpythonu;
ERROR: cannot drop extension plpythonu because other objects depend on it
DETAIL: extension hstore_plpythonu depends on extension plpythonu
function look_up_hstore(hstore,text) depends on transform for hstore
language plpythonu
extension ltree_plpythonu depends on extension plpythonu
HINT: Use DROP ... CASCADE to drop the dependent objects too.
STATEMENT: drop extension plpythonu;
ERROR: cannot drop extension plpythonu because other objects depend on it
DETAIL: extension hstore_plpythonu depends on extension plpythonu
function look_up_hstore(hstore,text) depends on transform for hstore
language plpythonu
extension ltree_plpythonu depends on extension plpythonu
HINT: Use DROP ... CASCADE to drop the dependent objects too.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Peter,

I'm still getting intermittent linking failures:

postgres=# drop extension plperl cascade;
NOTICE: drop cascades to extension hstore_plperl
DROP EXTENSION

postgres=# create extension plperl;
CREATE EXTENSION
postgres=# create extension hstore_plperl;
ERROR: could not load library
"/home/josh/pg93/lib/postgresql/hstore_plperl.so":
/home/josh/pg93/lib/postgresql/hstore_plperl.so: undefined symbol:
hstoreUniquePairs
STATEMENT: create extension hstore_plperl;
ERROR: could not load library
"/home/josh/pg93/lib/postgresql/hstore_plperl.so":
/home/josh/pg93/lib/postgresql/hstore_plperl.so: undefined symbol:
hstoreUniquePairs
postgres=#

There appears to be something wonky which breaks when I've been running
9.2, shut it down, and fire up 9.3.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

> postgres=# create extension plperl;
> CREATE EXTENSION
> postgres=# create extension hstore_plperl;
> ERROR: could not load library
> "/home/josh/pg93/lib/postgresql/hstore_plperl.so":
> /home/josh/pg93/lib/postgresql/hstore_plperl.so: undefined symbol:
> hstoreUniquePairs
> STATEMENT: create extension hstore_plperl;
> ERROR: could not load library
> "/home/josh/pg93/lib/postgresql/hstore_plperl.so":
> /home/josh/pg93/lib/postgresql/hstore_plperl.so: undefined symbol:
> hstoreUniquePairs
> postgres=#
>
> There appears to be something wonky which breaks when I've been running
> 9.2, shut it down, and fire up 9.3.

More on this: the problem appears to be that the symbols for hstore are
loaded only if I've just just created the extension in that database:

postgres=# create database plperlh
postgres-# ;
CREATE DATABASE
postgres=# \c plperlh;
You are now connected to database "plperlh" as user "josh".
plperlh=# create extension plperl;
CREATE EXTENSION
plperlh=# create extension hstore;
CREATE EXTENSION
plperlh=# create extension hstore_plperl;
CREATE EXTENSION
plperlh=#

plperlh=# \c postgres
You are now connected to database "postgres" as user "josh".
postgres=# create extension hstore_plperl;
ERROR: could not load library
"/home/josh/pg93/lib/postgresql/hstore_plperl.so":
/home/josh/pg93/lib/postgresql/hstore_plperl.so: undefined symbol:
PL_thr_key
STATEMENT: create extension hstore_plperl;
ERROR: could not load library
"/home/josh/pg93/lib/postgresql/hstore_plperl.so":
/home/josh/pg93/lib/postgresql/hstore_plperl.so: undefined symbol:
PL_thr_key

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 03/05/2013 02:52 PM, Josh Berkus wrote:

> plperlh=# \c postgres
> You are now connected to database "postgres" as user "josh".
> postgres=# create extension hstore_plperl;
> ERROR: could not load library
> "/home/josh/pg93/lib/postgresql/hstore_plperl.so":
> /home/josh/pg93/lib/postgresql/hstore_plperl.so: undefined symbol:
> PL_thr_key
> STATEMENT: create extension hstore_plperl;
> ERROR: could not load library
> "/home/josh/pg93/lib/postgresql/hstore_plperl.so":
> /home/josh/pg93/lib/postgresql/hstore_plperl.so: undefined symbol:
> PL_thr_key

What happens if you set your LD_LIBRARY_PATH to reflect each
installation before you start the database?

JD

>
>
>
>
>

--
Command Prompt, Inc. - http://www.commandprompt.com/
PostgreSQL Support, Training, Professional Services and Development
High Availability, Oracle Conversion, Postgres-XC
@cmdpromptinc - 509-416-6579

> What happens if you set your LD_LIBRARY_PATH to reflect each
> installation before you start the database?

No change, at least not setting it to $PGHOME/lib.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 3/5/13 5:52 PM, Josh Berkus wrote:
> More on this: the problem appears to be that the symbols for hstore are
> loaded only if I've just just created the extension in that database:

I see. This is a problem with any kind of dynamically loadable module
that uses another module. The other module is only loaded either at
creation time or when a function from it is first used (or if a preload
mechanism is used).

At run time, this will sort itself out, because all the required modules
will be loaded. The problem is when you create the "glue" extension and
haven't invoked any code from any of the dependent extension in the
session. Abstractly, the possible solutions are either not to check the
functions when the extension is created (possibly settable by a flag) or
to somehow force a load of all dependent extensions when the new
extension is created. (I say extension here even though dynamically
loadable modules are attached to functions, which makes this even more
confusing.)

In "normal" programming languages, this is normally addressed by placing
explicit load/require/import statements before you do anything else.
What we are doing here is more like an autoload functionality that some
environments have. Those have the same problem.

Peter,

> At run time, this will sort itself out, because all the required modules
> will be loaded. The problem is when you create the "glue" extension and
> haven't invoked any code from any of the dependent extension in the
> session.

Just invoking code doesn't seem to be enough. I tried just using the
Hstore data type, and then loading hstore_plperl, but that still didn't
work. It seems like only CREATE EXTENSION loads *all* the symbols.

> Abstractly, the possible solutions are either not to check the
> functions when the extension is created (possibly settable by a flag) or
> to somehow force a load of all dependent extensions when the new
> extension is created.

The latter would be ideal, but I don't know that we currently have any
mechanism for it.

--Josh

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 2013-03-06 09:53:29 -0800, Josh Berkus wrote:
> Peter,
>
> > At run time, this will sort itself out, because all the required modules
> > will be loaded. The problem is when you create the "glue" extension and
> > haven't invoked any code from any of the dependent extension in the
> > session.
>
> Just invoking code doesn't seem to be enough. I tried just using the
> Hstore data type, and then loading hstore_plperl, but that still didn't
> work. It seems like only CREATE EXTENSION loads *all* the symbols.

Your error looks like youre erroring out in plperl not in hstore?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

> Your error looks like youre erroring out in plperl not in hstore?

Look again.

Peter, is there any way for you to tackle this issue? I have no idea
how to fix it, myself ...

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 2013-03-11 09:42:18 -0700, Josh Berkus wrote:
>
> > Your error looks like youre erroring out in plperl not in hstore?
>
> Look again.

> ERROR: could not load library "/home/josh/pg93/lib/postgresql/hstore_plperl.so":
> /home/josh/pg93/lib/postgresql/hstore_plperl.so: undefined symbol: PL_thr_key

Thats a perl symbol.

> Peter, is there any way for you to tackle this issue? I have no idea
> how to fix it, myself ...

If you add a:

DO LANGUAGE plperlu $$$$;
SELECT NULL::hstore;

to the extension script, does it work?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 03/11/2013 09:50 AM, Andres Freund wrote:
> DO LANGUAGE plperlu $$$$;
> SELECT NULL::hstore;

Aha, so that's what you meant!

Yeah, it works if I reference both extensions before the CREATE EXTENSION.

In that case, seems like that could be added to the extension SQL, no?

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 2013-03-11 09:55:34 -0700, Josh Berkus wrote:
> On 03/11/2013 09:50 AM, Andres Freund wrote:
> > DO LANGUAGE plperlu $$$$;
> > SELECT NULL::hstore;
>
> Aha, so that's what you meant!
>
> Yeah, it works if I reference both extensions before the CREATE EXTENSION.
>
> In that case, seems like that could be added to the extension SQL, no?

If we don't find a better solution, yes. Why don't we lookup type
input/ouput function for parameters and return type during CREATE
FUNCTION? That should solve the issue in a neater way?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, 2013-03-11 at 18:11 +0100, Andres Freund wrote:
> If we don't find a better solution, yes. Why don't we lookup type
> input/ouput function for parameters and return type during CREATE
> FUNCTION? That should solve the issue in a neater way?

A function in general has no particular use for a type's input or output
function.

Also, a type's input/output functions are not necessarily in the same
library as other things about that type that you might want or need.

On 2013-03-11 20:28:05 -0400, Peter Eisentraut wrote:
> On Mon, 2013-03-11 at 18:11 +0100, Andres Freund wrote:
> > If we don't find a better solution, yes. Why don't we lookup type
> > input/ouput function for parameters and return type during CREATE
> > FUNCTION? That should solve the issue in a neater way?
>
> A function in general has no particular use for a type's input or output
> function.

> Also, a type's input/output functions are not necessarily in the same
> library as other things about that type that you might want or need.

In theory they are not necessarily tied together, yes. In practice they
nearly always are in the same shared object.
The lookup for them afaics are the only reason why e.g. the
plperl-hstore transform works outside of CREATE EXTENSION without
explictly doing something ugly like:

On 03/11/2013 09:50 AM, Andres Freund wrote:
> DO LANGUAGE plperlu $$$$;
> SELECT NULL::hstore;

beforehand.

If you have a better idea than putting the above into the extension
script or making a function look up its parameters, I am all ears. It
sure would only be a hack.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> At run time, this will sort itself out, because all the required modules
> will be loaded. The problem is when you create the "glue" extension and
> haven't invoked any code from any of the dependent extension in the
> session. Abstractly, the possible solutions are either not to check the
> functions when the extension is created (possibly settable by a flag) or
> to somehow force a load of all dependent extensions when the new
> extension is created. (I say extension here even though dynamically
> loadable modules are attached to functions, which makes this even more
> confusing.)

I think CREATE EXTENSION should be LOADing all distinct modules
referenced from all required extensions functions. It does sound easy
enough to code, and I can't imagine why we would want to not do that.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On 03/13/2013 09:54 AM, Dimitri Fontaine wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>> At run time, this will sort itself out, because all the required modules
>> will be loaded. The problem is when you create the "glue" extension and
>> haven't invoked any code from any of the dependent extension in the
>> session. Abstractly, the possible solutions are either not to check the
>> functions when the extension is created (possibly settable by a flag) or
>> to somehow force a load of all dependent extensions when the new
>> extension is created. (I say extension here even though dynamically
>> loadable modules are attached to functions, which makes this even more
>> confusing.)
>
> I think CREATE EXTENSION should be LOADing all distinct modules
> referenced from all required extensions functions. It does sound easy
> enough to code, and I can't imagine why we would want to not do that.

Where are we with this? Will the loading issue be fixed, or should we
bump this feature to 9.4?

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Peter, All:

Does anyone feel like fixing the LOAD issue with transforms? I haven't
seen any activity on the patch.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On 6/12/13 1:20 AM, Josh Berkus wrote:
> Peter, All:
>
> Does anyone feel like fixing the LOAD issue with transforms? I haven't
> seen any activity on the patch.

I plan to send in an updated patch.

A transform is an SQL object that supplies to functions for converting
between data types and procedural languages. For example, a transform
could arrange that hstore is converted to an appropriate hash or
dictionary object in PL/Perl or PL/Python.

Externally visible changes:

- new SQL commands CREATE TRANSFORM and DROP TRANSFORM

- system catalog pg_transform

- transform support in PL/Perl and PL/Python

- PL/Perl and PL/Python install their header files for use by external
types

- transforms contrib modules hstore_plperl, hstore_plpython,
ltree_plpython

- regression test mangling for Python 3 was moved to a separate
makefile, for use by extensions

The regression tests for general CREATE TRANSFORM functionality are
under the hstore_plperl module in order to be able to test it on a real
transform implementation, instead of having to create an entire fake
procedural language under src/test/regress/, say.

Dynamic module linking on OS X was changed to allow undefined symbols at
build time. This is necessary so that a transform module can use
symbols from the type and the language modules, if necessary. Other
platforms already behaved this way, but the default on OS X was
different.
---
Continued from 2013-01 commit fest. All known open issues have been fixed.

contrib/Makefile | 12 +
contrib/hstore_plperl/.gitignore | 4 +
contrib/hstore_plperl/Makefile | 23 ++
.../hstore_plperl/expected/create_transform.out | 70 +++++
contrib/hstore_plperl/expected/hstore_plperl.out | 170 ++++++++++++
contrib/hstore_plperl/hstore_plperl--1.0.sql | 17 ++
contrib/hstore_plperl/hstore_plperl.c | 90 ++++++
contrib/hstore_plperl/hstore_plperl.control | 6 +
contrib/hstore_plperl/hstore_plperlu--1.0.sql | 17 ++
contrib/hstore_plperl/hstore_plperlu.control | 6 +
contrib/hstore_plperl/sql/create_transform.sql | 45 +++
contrib/hstore_plperl/sql/hstore_plperl.sql | 120 ++++++++
contrib/hstore_plpython/.gitignore | 4 +
contrib/hstore_plpython/Makefile | 30 ++
.../hstore_plpython/expected/hstore_plpython.out | 137 +++++++++
contrib/hstore_plpython/hstore_plpython.c | 116 ++++++++
contrib/hstore_plpython/hstore_plpython2u--1.0.sql | 19 ++
contrib/hstore_plpython/hstore_plpython2u.control | 6 +
contrib/hstore_plpython/hstore_plpython3u--1.0.sql | 19 ++
contrib/hstore_plpython/hstore_plpython3u.control | 6 +
contrib/hstore_plpython/hstore_plpythonu--1.0.sql | 19 ++
contrib/hstore_plpython/hstore_plpythonu.control | 6 +
contrib/hstore_plpython/sql/hstore_plpython.sql | 106 +++++++
contrib/ltree_plpython/.gitignore | 4 +
contrib/ltree_plpython/Makefile | 30 ++
contrib/ltree_plpython/expected/ltree_plpython.out | 42 +++
contrib/ltree_plpython/ltree_plpython.c | 32 +++
contrib/ltree_plpython/ltree_plpython2u--1.0.sql | 12 +
contrib/ltree_plpython/ltree_plpython2u.control | 6 +
contrib/ltree_plpython/ltree_plpython3u--1.0.sql | 12 +
contrib/ltree_plpython/ltree_plpython3u.control | 6 +
contrib/ltree_plpython/ltree_plpythonu--1.0.sql | 12 +
contrib/ltree_plpython/ltree_plpythonu.control | 6 +
contrib/ltree_plpython/sql/ltree_plpython.sql | 34 +++
doc/src/sgml/catalogs.sgml | 73 +++++
doc/src/sgml/hstore.sgml | 18 ++
doc/src/sgml/ltree.sgml | 15 +
doc/src/sgml/ref/allfiles.sgml | 2 +
doc/src/sgml/ref/alter_extension.sgml | 21 ++
doc/src/sgml/ref/comment.sgml | 22 ++
doc/src/sgml/ref/create_transform.sgml | 187 +++++++++++++
doc/src/sgml/ref/drop_transform.sgml | 123 ++++++++
doc/src/sgml/reference.sgml | 2 +
src/Makefile.shlib | 2 +-
src/backend/catalog/Makefile | 1 +
src/backend/catalog/dependency.c | 8 +
src/backend/catalog/objectaddress.c | 76 ++++-
src/backend/catalog/pg_proc.c | 20 ++
src/backend/commands/dropcmds.c | 6 +
src/backend/commands/event_trigger.c | 3 +
src/backend/commands/functioncmds.c | 293 ++++++++++++++++++++
src/backend/nodes/copyfuncs.c | 17 ++
src/backend/nodes/equalfuncs.c | 15 +
src/backend/parser/gram.y | 82 +++++-
src/backend/parser/parse_func.c | 5 +
src/backend/tcop/utility.c | 16 ++
src/backend/utils/adt/ruleutils.c | 11 +-
src/backend/utils/cache/lsyscache.c | 83 ++++++
src/backend/utils/cache/syscache.c | 23 ++
src/bin/pg_dump/common.c | 5 +
src/bin/pg_dump/pg_dump.c | 230 +++++++++++++++
src/bin/pg_dump/pg_dump.h | 11 +
src/bin/pg_dump/pg_dump_sort.c | 13 +-
src/include/catalog/catversion.h | 2 +-
src/include/catalog/dependency.h | 1 +
src/include/catalog/indexing.h | 5 +
src/include/catalog/pg_transform.h | 47 ++++
src/include/commands/defrem.h | 3 +
src/include/nodes/nodes.h | 1 +
src/include/nodes/parsenodes.h | 15 +
src/include/parser/kwlist.h | 2 +
src/include/utils/lsyscache.h | 4 +
src/include/utils/syscache.h | 2 +
src/interfaces/ecpg/preproc/ecpg.tokens | 2 +-
src/interfaces/ecpg/preproc/ecpg.trailer | 5 +-
src/interfaces/ecpg/preproc/ecpg_keywords.c | 2 -
src/pl/plperl/GNUmakefile | 4 +-
src/pl/plperl/plperl.c | 32 ++-
src/pl/plperl/plperl_helpers.h | 2 +
src/pl/plpython/Makefile | 40 +--
src/pl/plpython/plpy_main.c | 1 +
src/pl/plpython/plpy_procedure.c | 6 +-
src/pl/plpython/plpy_procedure.h | 1 +
src/pl/plpython/plpy_spi.c | 3 +-
src/pl/plpython/plpy_typeio.c | 158 +++++++----
src/pl/plpython/plpy_typeio.h | 9 +-
src/pl/plpython/plpy_util.c | 21 +-
src/pl/plpython/plpy_util.h | 1 +
src/pl/plpython/plpython.h | 1 +
src/pl/plpython/regress-python3-mangle.mk | 35 +++
src/test/regress/expected/sanity_check.out | 3 +-
91 files changed, 2891 insertions(+), 144 deletions(-)
create mode 100644 contrib/hstore_plperl/.gitignore
create mode 100644 contrib/hstore_plperl/Makefile
create mode 100644 contrib/hstore_plperl/expected/create_transform.out
create mode 100644 contrib/hstore_plperl/expected/hstore_plperl.out
create mode 100644 contrib/hstore_plperl/hstore_plperl--1.0.sql
create mode 100644 contrib/hstore_plperl/hstore_plperl.c
create mode 100644 contrib/hstore_plperl/hstore_plperl.control
create mode 100644 contrib/hstore_plperl/hstore_plperlu--1.0.sql
create mode 100644 contrib/hstore_plperl/hstore_plperlu.control
create mode 100644 contrib/hstore_plperl/sql/create_transform.sql
create mode 100644 contrib/hstore_plperl/sql/hstore_plperl.sql
create mode 100644 contrib/hstore_plpython/.gitignore
create mode 100644 contrib/hstore_plpython/Makefile
create mode 100644 contrib/hstore_plpython/expected/hstore_plpython.out
create mode 100644 contrib/hstore_plpython/hstore_plpython.c
create mode 100644 contrib/hstore_plpython/hstore_plpython2u--1.0.sql
create mode 100644 contrib/hstore_plpython/hstore_plpython2u.control
create mode 100644 contrib/hstore_plpython/hstore_plpython3u--1.0.sql
create mode 100644 contrib/hstore_plpython/hstore_plpython3u.control
create mode 100644 contrib/hstore_plpython/hstore_plpythonu--1.0.sql
create mode 100644 contrib/hstore_plpython/hstore_plpythonu.control
create mode 100644 contrib/hstore_plpython/sql/hstore_plpython.sql
create mode 100644 contrib/ltree_plpython/.gitignore
create mode 100644 contrib/ltree_plpython/Makefile
create mode 100644 contrib/ltree_plpython/expected/ltree_plpython.out
create mode 100644 contrib/ltree_plpython/ltree_plpython.c
create mode 100644 contrib/ltree_plpython/ltree_plpython2u--1.0.sql
create mode 100644 contrib/ltree_plpython/ltree_plpython2u.control
create mode 100644 contrib/ltree_plpython/ltree_plpython3u--1.0.sql
create mode 100644 contrib/ltree_plpython/ltree_plpython3u.control
create mode 100644 contrib/ltree_plpython/ltree_plpythonu--1.0.sql
create mode 100644 contrib/ltree_plpython/ltree_plpythonu.control
create mode 100644 contrib/ltree_plpython/sql/ltree_plpython.sql
create mode 100644 doc/src/sgml/ref/create_transform.sgml
create mode 100644 doc/src/sgml/ref/drop_transform.sgml
create mode 100644 src/include/catalog/pg_transform.h
create mode 100644 src/pl/plpython/regress-python3-mangle.mk

Peter Eisentraut <peter_e(at)gmx(dot)net> a écrit :
>A transform is an SQL object that supplies to functions for converting
>between data types and procedural languages. For example, a transform
>could arrange that hstore is converted to an appropriate hash or
>dictionary object in PL/Perl or PL/Python.

Nice !

>Continued from 2013-01 commit fest. All known open issues have been
>fixed.

You kept PGXS style makefile...

--
Envoyé de mon téléphone excusez la brièveté.

On 6/14/13 3:46 AM, Cédric Villemain wrote:
> You kept PGXS style makefile...

I know, but that's a separate issue that hasn't been decided yet.

On 06/14/2013 11:11 AM, Peter Eisentraut wrote:
> A transform is an SQL object that supplies to functions for converting
> between data types and procedural languages. For example, a transform
> could arrange that hstore is converted to an appropriate hash or
> dictionary object in PL/Perl or PL/Python.
>
> Externally visible changes:
>
> - new SQL commands CREATE TRANSFORM and DROP TRANSFORM
>
> - system catalog pg_transform
>
> - transform support in PL/Perl and PL/Python
>
> - PL/Perl and PL/Python install their header files for use by external
> types
I wonder if that should be extended to install headers for hstore,
ltree, and while we're at it, intarray as well?

You handle getting access to the headers by using include path flags:

PG_CPPFLAGS = -I$(top_srcdir)/src/pl/plpython $(python_includespec)
-I$(top_srcdir)/contrib/hstore

which is fine for in-tree builds but AFAIK means that pgxs cannot build
these extensions. If the extension modules installed their headers that
problem would go away; you'd just

#include "contrib/hstore/hstore.h"

Many modules already have useful non-static functions that work with raw
types and aren't callable via the fmgr, as well as the macros and
typdefs that make working with them easier. intarray in particular would
be extremely useful to use from other extensions, but in the past I've
landed up copying it and then adding the extra functions I needed
because I couldn't access its header in a PGXS build.

Peter's already answered my main worry on this, which is whether any
platform would prevent us from doing the required runtime symbol lookup
from shared libs loaded at the same level. I knew RTLD_GLOBAL allowed it
for dlopen(...), but wasn't sure about other platforms.

Thoughts? Should contrib modules be able to install headers and use each
others' symbols? I've seen interest in this in the wild from people who
want to use hstore in their own extensions and I've had uses for it
myself with intarray.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 6/14/13 11:48 PM, Craig Ringer wrote:
> I wonder if that should be extended to install headers for hstore,
> ltree, and while we're at it, intarray as well?

Sure, if someone wants to go through and check which headers are
independently usable, and do the necessarily cleanups with necessary.

Peter Eisentraut wrote:
> A transform is an SQL object that supplies to functions for converting
> between data types and procedural languages. For example, a transform
> could arrange that hstore is converted to an appropriate hash or
> dictionary object in PL/Perl or PL/Python.
>
> Externally visible changes:

This is a large patch. Do you intend to push the whole thing as a
single commit, or split it?

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 06/18/2013 04:58 AM, Peter Eisentraut wrote:
> On 6/14/13 11:48 PM, Craig Ringer wrote:
>> I wonder if that should be extended to install headers for hstore,
>> ltree, and while we're at it, intarray as well?
> Sure, if someone wants to go through and check which headers are
> independently usable, and do the necessarily cleanups with necessary.
I can do that if there are no objections. It's only tangental to this
work really, so I'll post a separate thread when I get on to it.

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 6/17/13 5:31 PM, Alvaro Herrera wrote:
> This is a large patch. Do you intend to push the whole thing as a
> single commit, or split it?

I thought about splitting it up, but I didn't find a reasonable way to
do it.

Peter,

I've been playing with the new patch, and haven't been able to reproduce
the load problem created by the original patch. So that seems fixed.

I'm not comfortable with having all of the transform mappings in the
main contrib/ directory though. Can we add a subdirectory called
"transforms" containing all of these?

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Thu, Jun 13, 2013 at 8:11 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:

> A transform is an SQL object that supplies to functions for converting
> between data types and procedural languages. For example, a transform
> could arrange that hstore is converted to an appropriate hash or
> dictionary object in PL/Perl or PL/Python.
>
>
The patch applies and regression including contrib passes in my Mac. I
know I came late, but I have a few questions.

- vs SQL standard
I'm worried about overloading the standard. As document says, SQL standard
defines CREATE TRANSFORM syntax which is exactly the same as this proposal
but for different purpose. The standard feature is the data conversion
between client and server side data type, I guess. I am concerned about it
because in the future if someone wants to implement this SQL standard
feature, there is no way to break thing. I'd be happy if subsequent clause
was different. CREATE TYPE has two different syntax, one for composite
type and one for internal user-defined type (I'm not sure either is defined
in the standard, though) and I think we could do something like that. Or
as someone suggested in the previous thread, it might be a variant of
CAST. CREATE CAST (hstore AS plpython2u) ? Or CREATE LANGUAGE TRANSFORM
might sound better. In either case, I think we are missing the discussion
on the standard overloading.

- dependency loading issue
Although most of the use cases are via CREATE EXTENSION, it is not great to
let users to load the dependency manually. Is it possible to load
hstore.so and plpython2u.so from _PG_init of hstore_plpython2u? Because
the author of transform should certainly know the name of shared library in
the database installation, writing down the shared library names in the
init function sounds reasonable. Or do we still need to consider cases
where plpython2u.so is renamed to something else?

- function types
Although I read the suggestion to use internal type as the argument of
from_sql function, I guess it exposes another security risk. Since we
don't know what SQL type can safely be passed to the from_sql function, we
cannot check if the function is the right one at the time of CREATE
TRANSFORM. Non-super user can add his user defined type and own it, and
create a transform that with from_sql function that takes internal and
crashes with this user-defined type. A possible compromise is let only
super user create transforms, or come up with nice way to allow
func(sql_type) -> internal signature.

- create or replace causes inconsistency
I tried:
* create transform python to hstore (one way transform)
* create function f(h hstore) language python
* create or replace transform hstore to python and python to hstore (both
ways)
* call f() causes error, since it misses hstore to python transform. It
is probably looking at the old definition

- create func -> create transform is not prohibited
I saw your post in the previous discussion:

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

However I don't see this prohibition of create transform if there is
already such function. You are not planning to address this issue?

For now, that's it. I'm going to dig more later.

Thanks,

Hitoshi Harada

On 7/3/13 7:15 PM, Josh Berkus wrote:
> I'm not comfortable with having all of the transform mappings in the
> main contrib/ directory though. Can we add a subdirectory called
> "transforms" containing all of these?

I don't see any value in that. The data types they apply to are in
contrib after all.

On Friday, July 5, 2013, Peter Eisentraut wrote:

> On 7/3/13 7:15 PM, Josh Berkus wrote:
> > I'm not comfortable with having all of the transform mappings in the
> > main contrib/ directory though. Can we add a subdirectory called
> > "transforms" containing all of these?
>
> I don't see any value in that. The data types they apply to are in
> contrib after all.
>
>
I guess his suggestion is contrib/transforms directory, not transforms
directory at top level. Stil you don't see value?

--
Hitoshi Harada

On 07/05/2013 09:08 AM, Peter Eisentraut wrote:
> On 7/3/13 7:15 PM, Josh Berkus wrote:
>> I'm not comfortable with having all of the transform mappings in the
>> main contrib/ directory though. Can we add a subdirectory called
>> "transforms" containing all of these?
>
> I don't see any value in that. The data types they apply to are in
> contrib after all.

Well, I had three thoughts on this:

(a) transforms aren't like other contribs, in that they are dependant on
other contribs before you install them.

(b) we can expect maybe a dozen to 18 of them in core based on the data
types there, and I hate to clutter up /contrib, and

(c) I'd like to do a future feature which supports "install all
transforms" functionality, which would be helped by having them in their
own directory.

That's my thinking on it anyway.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> (c) I'd like to do a future feature which supports "install all
> transforms" functionality, which would be helped by having them in their
> own directory.

I think we should install required extensions automatically when they
are available. Also, we will need better tooling to deal with extension
dependencies, see my patch for that from some time ago.

https://commitfest.postgresql.org/action/patch_view?id=727

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On Thu, 2013-07-04 at 02:18 -0700, Hitoshi Harada wrote:
> as someone suggested in the previous thread, it might be a variant of
> CAST. CREATE CAST (hstore AS plpython2u) ? Or CREATE LANGUAGE TRANSFORM
> might sound better. In either case, I think we are missing the discussion
> on the standard overloading.

LANGUAGE isn't a concept limited to the server side in the SQL standard.
I could go with something like CREATE SERVER TRANSFORM.

> - dependency loading issue
> Although most of the use cases are via CREATE EXTENSION, it is not great to
> let users to load the dependency manually. Is it possible to load
> hstore.so and plpython2u.so from _PG_init of hstore_plpython2u? Because
> the author of transform should certainly know the name of shared library in
> the database installation, writing down the shared library names in the
> init function sounds reasonable. Or do we still need to consider cases
> where plpython2u.so is renamed to something else?

I don't like my solution very much either, but I think I like this one
even less. I think the identity of the shared library for the hstore
type is the business of the hstore extension, and other extensions
shouldn't mess with it. The interfaces exposed by the hstore extension
are the types and functions, and that's what we are allowed to use. If
that's not enough, we need to expose more interfaces.

> - function types
> Although I read the suggestion to use internal type as the argument of
> from_sql function, I guess it exposes another security risk. Since we
> don't know what SQL type can safely be passed to the from_sql function, we
> cannot check if the function is the right one at the time of CREATE
> TRANSFORM. Non-super user can add his user defined type and own it, and
> create a transform that with from_sql function that takes internal and
> crashes with this user-defined type. A possible compromise is let only
> super user create transforms, or come up with nice way to allow
> func(sql_type) -> internal signature.

Good point. My original patch allowed func(sql_type) -> internal, but I
took that out because people had security concerns.

I'd be OK with restricting transform creation to superusers in the first
cut.

> - create or replace causes inconsistency
> I tried:
> * create transform python to hstore (one way transform)
> * create function f(h hstore) language python
> * create or replace transform hstore to python and python to hstore (both
> ways)
> * call f() causes error, since it misses hstore to python transform. It
> is probably looking at the old definition

What error exactly? Can you show the full test case?

There might be some caching going on.

> - create func -> create transform is not prohibited
> I saw your post in the previous discussion:
>
> > > * 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.
>
> However I don't see this prohibition of create transform if there is
> already such function. You are not planning to address this issue?

I had planned to implement that, but talking to some people most didn't
think it was useful or desirable. It's still open for debate.

On Fri, 2013-07-05 at 12:04 -0700, Josh Berkus wrote:
> (a) transforms aren't like other contribs, in that they are dependant on
> other contribs before you install them.

That doesn't appear to be a reason for creating subdirectories.

> (b) we can expect maybe a dozen to 18 of them in core based on the data
> types there, and I hate to clutter up /contrib, and

Well, that's a matter of opinion. I'd be more happy with 250 contribs
all on the same level versus a bunch of subdirectories structured based
on personal preferences.

But hey, we disagreed on config.sgml for similar reasons, IIRC. ;-)

> (c) I'd like to do a future feature which supports "install all
> transforms" functionality, which would be helped by having them in their
> own directory.

Installing all transforms by itself is not a sensible operation, because
you only want the transforms for the types and languages that you
actually use or have previously selected for installation.

I understand that this situation is unsatisfactory. But I don't think
any dependency or package management system has solved it. For example,
can any package management system make the following decision: user
install PHP, user installs PostgreSQL client => install php-pgsql
automatically. I don't think so. The only solutions are making PHP
dependent on PostgreSQL (or vice versa), or having the user install
php-pgsql explicitly.

Peter,

On 07/07/2013 12:06 PM, Peter Eisentraut wrote:
> Good point. My original patch allowed func(sql_type) -> internal, but I
> took that out because people had security concerns.
>
> I'd be OK with restricting transform creation to superusers in the first
> cut.

Have we added the ability of non-superusers to create extensions yet?
Until we have that, there's no point in allowing them to load
transforms. Once we have that, we'll need to let them do it.

>>> 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.
>>
>> However I don't see this prohibition of create transform if there is
>> already such function. You are not planning to address this issue?
>
> I had planned to implement that, but talking to some people most didn't
> think it was useful or desirable. It's still open for debate.

I don't think it's desirable. It would be hard to do, and at some level
we need to make a presumption of competence on the part of the DBA. We
should put a warning in the docs, though.

>> (b) we can expect maybe a dozen to 18 of them in core based on the data
>> types there, and I hate to clutter up /contrib, and
>
> Well, that's a matter of opinion. I'd be more happy with 250 contribs
> all on the same level versus a bunch of subdirectories structured based
> on personal preferences.
>
> But hey, we disagreed on config.sgml for similar reasons, IIRC. ;-)

Yeah, I'd like to see some other opinions on this.

>> (c) I'd like to do a future feature which supports "install all
>> transforms" functionality, which would be helped by having them in their
>> own directory.
>
> Installing all transforms by itself is not a sensible operation, because
> you only want the transforms for the types and languages that you
> actually use or have previously selected for installation.

Give me some credit. I'm talking about a script for "install all
transforms for which the dependancies are already installed". That's
certainly entirely doable, and would be made easier by putting the
transforms in their own directory or otherwise flagging them to identify
them as transforms.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Sun, Jul 7, 2013 at 12:06 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On Thu, 2013-07-04 at 02:18 -0700, Hitoshi Harada wrote:
>> as someone suggested in the previous thread, it might be a variant of
>> CAST. CREATE CAST (hstore AS plpython2u) ? Or CREATE LANGUAGE TRANSFORM
>> might sound better. In either case, I think we are missing the discussion
>> on the standard overloading.
>
> LANGUAGE isn't a concept limited to the server side in the SQL standard.
> I could go with something like CREATE SERVER TRANSFORM.

I like it better than the current one.

>> - dependency loading issue
>> Although most of the use cases are via CREATE EXTENSION, it is not great to
>> let users to load the dependency manually. Is it possible to load
>> hstore.so and plpython2u.so from _PG_init of hstore_plpython2u? Because
>> the author of transform should certainly know the name of shared library in
>> the database installation, writing down the shared library names in the
>> init function sounds reasonable. Or do we still need to consider cases
>> where plpython2u.so is renamed to something else?
>
> I don't like my solution very much either, but I think I like this one
> even less. I think the identity of the shared library for the hstore
> type is the business of the hstore extension, and other extensions
> shouldn't mess with it. The interfaces exposed by the hstore extension
> are the types and functions, and that's what we are allowed to use. If
> that's not enough, we need to expose more interfaces.

OK, my idea was worse, because the symbol resolution happens before
_PG_init anyway. But what I feel is, why can't we load dependency
libraries automatically? plpython can load libpython automatically, I
guess. Is it only the matter of LD_LIBRARY_PATH stuff?

>> - function types
>> Although I read the suggestion to use internal type as the argument of
>> from_sql function, I guess it exposes another security risk. Since we
>> don't know what SQL type can safely be passed to the from_sql function, we
>> cannot check if the function is the right one at the time of CREATE
>> TRANSFORM. Non-super user can add his user defined type and own it, and
>> create a transform that with from_sql function that takes internal and
>> crashes with this user-defined type. A possible compromise is let only
>> super user create transforms, or come up with nice way to allow
>> func(sql_type) -> internal signature.
>
> Good point. My original patch allowed func(sql_type) -> internal, but I
> took that out because people had security concerns.
>
> I'd be OK with restricting transform creation to superusers in the first
> cut.

Yeah, I think it's better to limit to superuser.

>> - create or replace causes inconsistency
>> I tried:
>> * create transform python to hstore (one way transform)
>> * create function f(h hstore) language python
>> * create or replace transform hstore to python and python to hstore (both
>> ways)
>> * call f() causes error, since it misses hstore to python transform. It
>> is probably looking at the old definition
>
> What error exactly? Can you show the full test case?
>
> There might be some caching going on.

Here is the full set of what's happening.

test=# create extension hstore;
CREATE EXTENSION
test=# create extension plpython2u;
CREATE EXTENSION
test=# CREATE FUNCTION hstore_to_plpython2(val internal) RETURNS internal
test-# LANGUAGE C STRICT IMMUTABLE
test-# AS '$libdir/hstore_plpython2', 'hstore_to_plpython';
CREATE FUNCTION
test=#
test=# CREATE FUNCTION plpython2_to_hstore(val internal) RETURNS hstore
test-# LANGUAGE C STRICT IMMUTABLE
test-# AS 'hstore_plpython2', 'plpython_to_hstore';
CREATE FUNCTION
test=#
test=# CREATE TRANSFORM FOR hstore LANGUAGE plpython2u (
test(# FROM SQL WITH FUNCTION hstore_to_plpython2(internal),
test(# TO SQL WITH FUNCTION plpython2_to_hstore(internal)
test(# );
CREATE TRANSFORM
test=# create or replace function f(h hstore) returns hstore as $$
test$# h['b'] = 10
test$# return h
test$# $$ language plpython2u;
CREATE FUNCTION
test=# select f('a=>1');
f
---------------------
"a"=>"1", "b"=>"10"
(1 row)

test=# CREATE OR REPLACE TRANSFORM FOR hstore LANGUAGE plpython2u (
test(# TO SQL WITH FUNCTION plpython2_to_hstore(internal)
test(# );
CREATE TRANSFORM
test=# select f('a=>1');
f
---------------------
"a"=>"1", "b"=>"10"
(1 row)

test=# \c
You are now connected to database "test" as user "haradh1".
test=# select f('a=>1');
ERROR: TypeError: 'str' object does not support item assignment
CONTEXT: Traceback (most recent call last):
PL/Python function "f", line 2, in <module>
h['b'] = 10
PL/Python function "f"

After reconnecting to the database with \c, the function behavior
changed because we did CREATE OR REPLACE. If we go with dependency
between function and transform, we shouldn't support OR REPLACE, I
guess. plpython can throw away the cache, but I feel like not
supporting OR REPLACE in this case.

--
Hitoshi Harada

On Thu, Jul 4, 2013 at 2:18 AM, Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com> wrote:
> For now, that's it. I'm going to dig more later.
>

After looking into rest of the change,

- TYPTYPE_DOMAIN is not supported. Why did you specifically disallow it?
- ParseFuncOrColumn now prohibits to find function returning internal,
but is it effective? I think it's already disallowed, or there is no
way to call it.
- get_transform_oid and get_transform are redundant

--
Hitoshi Harada

On 07/08/2013 12:00 PM, Josh Berkus wrote:
>>> (b) we can expect maybe a dozen to 18 of them in core based on the data
>>> >> types there, and I hate to clutter up /contrib, and
>> >
>> > Well, that's a matter of opinion. I'd be more happy with 250 contribs
>> > all on the same level versus a bunch of subdirectories structured based
>> > on personal preferences.
>> >
>> > But hey, we disagreed on config.sgml for similar reasons, IIRC. ;-)
> Yeah, I'd like to see some other opinions on this.
>

Well, based on the total lack of opinions from anyone on -hackers, I'd
say we leave your patch alone and leave the transforms in their current
directory location. We can always move them later.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Mon, 2013-07-08 at 23:00 -0700, Hitoshi Harada wrote:
> On Sun, Jul 7, 2013 at 12:06 PM, Peter Eisentraut <peter_e(at)gmx(dot)net>
> wrote:
> > On Thu, 2013-07-04 at 02:18 -0700, Hitoshi Harada wrote:
> >> as someone suggested in the previous thread, it might be a variant
> of
> >> CAST. CREATE CAST (hstore AS plpython2u) ? Or CREATE LANGUAGE
> TRANSFORM
> >> might sound better. In either case, I think we are missing the
> discussion
> >> on the standard overloading.
> >
> > LANGUAGE isn't a concept limited to the server side in the SQL
> standard.
> > I could go with something like CREATE SERVER TRANSFORM.
>
> I like it better than the current one.

I had started to work on making this adjustment, but found the result
very ugly. It also created a confusing association with CREATE SERVER,
which is something different altogether.

My next best idea is CREATE TRANSFORM FOR hstore SERVER LANGUAGE plperl,
which preserves the overall idea but still distinguishes server from
client languages.

Comments?

On 08/13/2013 07:16 PM, Peter Eisentraut wrote:
> My next best idea is CREATE TRANSFORM FOR hstore SERVER LANGUAGE plperl,
> which preserves the overall idea but still distinguishes server from
> client languages.
>
> Comments?

My thinking is that TRANSFORMS will almost certainly be managed by
installer/puppet scripts by users, so it doesn't really matter how ugly
the syntax is, as long as it's unambiguous.

Which is a roundabout way of saying "whatever syntax you implement is
fine with me from a usability perspective".

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Here is a new patch which addresses some of the issues you found.

> - vs SQL standard
>
After reviewing this, there is actually no conflict with the SQL
standard, because the CREATE TRANSFORM syntax in the SQL standard uses
an additional transform group clause which I don't use. So there is no
ambiguity. Hence, I left the syntax unchanged.

> - function types

I changed this so that CREATE TRANSFORM requires owning the from-SQL and
to-SQL functions.

> - create or replace causes inconsistency
>
Fixed this by adding cache invalidation in PL/Python.

Rebased patch. No changes except that merge conflicts were resolved,
and I had to add some Data::Dumper tweaks to the regression tests so
that the results came out in consistent order on different versions of
Perl.

Hi,

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Rebased patch. No changes except that merge conflicts were resolved,
> and I had to add some Data::Dumper tweaks to the regression tests so
> that the results came out in consistent order on different versions of
> Perl.

I just spent some time reading that patch, here's my first round of
review:

- Documentation style seems to be to be different from the "man page"
or "reference docs" style that we use elsewhere, and is instead
deriving the general case from examples. Reads strange.

- The internal datatype argument and return type discussion for
function argument looks misplaced, but I don't have a better
proposition for that.

- The code looks good, I didn't spot any problem when reading it.
Given your Jenkins installation I didn't try (yet at least) to use
the patch and compile and run it locally.

Interesting note might be the addition of two new system caches, one
for the PL languages and another one for the transform functions. I
agree with the need, just wanted to raise awereness about that in
case there's something to be said on that front.

- Do we need an ALTER TRANSFORM command?

Usually we have at least an Owner for the new objects and a command
to change the owner. Then should we be able to change the
function(s) used in a transform?

- Should transform live in a schema?

At first sight, no reason why, but see next point about a use case
that we might be able to solve doing that.

- SQL Standard has something different named the same thing,
targetting client side types apparently. Is there any reason why we
would want to stay away from using the same name for something
really different in PostgreSQL?

On the higher level design, the big question here is about selective
behavior. As soon as you CREATE TRANSFORM FOR hstore LANGUAGE plperl
then any plperl function will now receive its hstore arguments as a
proper perl hash rather than a string.

Any pre-existing plperl function with hstore arguments or return type
then needs to be upgraded to handle the new types nicely, and some of
those might not be under the direct control of the DBA running the
CREATE TRANSFORM command, when using some plperl extensions for example.

A mechanism allowing for the transform to only be used in some functions
but not others might be useful. The simplest such mechanism I can think
of is modeled against the PL/Java classpath facility as specified in the
SQL standard: you attach a classpath per schema.

We could design transforms to only "activate" in the schema they are
created, thus allowing plperl functions to co-exist where some will
receive proper hash for hstores and other will continue to get plain
text.

Should using the schema to that ends be frowned upon, then we need a way
to register each plperl function against using or not using the
transform facility, defaulting to not using anything. Maybe something
like the following:

CREATE FUNCTION foo(hash hstore, x ltree)
RETURNS hstore
LANGUAGE plperl
USING TRANSFORM FOR hstore, ltree
AS $$ … $$;

Worst case, that I really don't think we need, would be addressing that
per-argument:

CREATE FUNCTION foo (hash hstore WITH TRANSFORM, kv hstore) …

I certainly hope we don't need that, and sure can't imagine use cases
for that level of complexity at the time of writing this review.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On 11/15/13, 11:04 AM, Dimitri Fontaine wrote:
> - Documentation style seems to be to be different from the "man page"
> or "reference docs" style that we use elsewhere, and is instead
> deriving the general case from examples. Reads strange.

Which specific section do you have in mind? It's hard to explain this
feature in abstract terms, I think.

> - The internal datatype argument and return type discussion for
> function argument looks misplaced, but I don't have a better
> proposition for that.

OK, maybe I'll put that in parentheses or a separate paragraph.

> - Do we need an ALTER TRANSFORM command?
>
> Usually we have at least an Owner for the new objects and a command
> to change the owner. Then should we be able to change the
> function(s) used in a transform?

We don't have ALTER CAST either, and no one's been too bothered about
that. It's possible, of course.

> - Should transform live in a schema?
>
> At first sight, no reason why, but see next point about a use case
> that we might be able to solve doing that.

Transforms don't have a name, so I don't quite see what you mean here.

> - SQL Standard has something different named the same thing,
> targetting client side types apparently. Is there any reason why we
> would want to stay away from using the same name for something
> really different in PostgreSQL?

Let's review that, as there as been some confusion about that. The SQL
standard syntax is

CREATE TRANSFORM FOR <type> <groupname> (...details...);

and then there is

SET DEFAULT TRANSFORM GROUP <groupname>
SET TRANSFORM GROUP FOR TYPE <type> <groupname>

This is essentially an elaborate way to have custom input/output
formats, like DateStyle or bytea_output.

(You can find examples of this in the IBM DB2 documentation. Some of
their clients apparently set a certain transform group automatically,
allowing you to set per-interface output formats.)

The proposed syntax in the other hand is

CREATE TRANSFORM FOR <type> LANGUAGE <lang> (...details...);

So you could consider LANGUAGE <lang> to be the implicit transform group
of language <lang>, if you like.

Or you could consider that this is a situation like VIEW vs.
MATERERIALIZED VIEW: they sound the same, they are a bit alike, but the
implementation details are different.

All obvious synonyms of "transform" (conversion, translation, etc.) are
already in use.

> On the higher level design, the big question here is about selective
> behavior. As soon as you CREATE TRANSFORM FOR hstore LANGUAGE plperl
> then any plperl function will now receive its hstore arguments as a
> proper perl hash rather than a string.
>
> Any pre-existing plperl function with hstore arguments or return type
> then needs to be upgraded to handle the new types nicely, and some of
> those might not be under the direct control of the DBA running the
> CREATE TRANSFORM command, when using some plperl extensions for example.

I had proposed disallowing installing a transform that would affect
existing functions. That was rejected or deemed unnecessary. You can't
have it both ways. ;-)

> A mechanism allowing for the transform to only be used in some functions
> but not others might be useful. The simplest such mechanism I can think
> of is modeled against the PL/Java classpath facility as specified in the
> SQL standard: you attach a classpath per schema.

Anything that's a problem per-database would also be a problem per schema.

> Should using the schema to that ends be frowned upon, then we need a way
> to register each plperl function against using or not using the
> transform facility, defaulting to not using anything. Maybe something
> like the following:
>
> CREATE FUNCTION foo(hash hstore, x ltree)
> RETURNS hstore
> LANGUAGE plperl
> USING TRANSFORM FOR hstore, ltree
> AS $$ … $$;

This is a transition problem. Nobody is required to install the
transforms into their existing databases. They probably shouldn't.

How many people actually use hstore with PL/Perl or PL/Python now?
Probably not many, because it's weird.

I like to think about how this works for new development: Here is my
extension type, here is how it interfaces with languages. Once you have
established that, you don't want to have to repeat that every time you
write a function. That's error prone and cumbersome. And anything
that's set per schema or higher is a dependency tracking and caching mess.

Also, extension types should work the same as built-in types.
Eventually, I'd like to rip out the hard-coded data type support in
PL/Python and replace it with built-in transforms. Even if we don't
actually do it, conceptually it should be possible. Now if we require
"USING TRANSFORM FOR int, bytea" every time, we'd have taken a big step
back. Effectively, we already have built-in transforms in PL/Python.
We have added a few more over the years. It's been a bit of a pain from
time to time. At least, with this feature we'd be moving this decision
into user space and give people a way to fix things. (Incidentally, if
you add a lot of transforms, you are probably dealing with a strongly
typed language. And a strongly typed language is more likely to cleanly
catch type errors resulting from changes in the transforms.)

On Wed, Nov 20, 2013 at 11:51 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> This is a transition problem. Nobody is required to install the
> transforms into their existing databases. They probably shouldn't.

Sure, but that's like saying "nobody's required to use this
behavior-changing GUC, so it's OK to have a behavior-changing GUC".

The point I think Dimitri is making, which IMHO is entirely valid, is
that the feature as currently designed is database-wide. You either
get this behavior for all of your functions, or you get it for none of
them, and that might well not be what you want. For example, it's
easy to imagine that you might want to install extensions A and B. A
expects that a certain transform is loaded into the database, and B
expects that it isn't. You now have created a situation where
extensions A and B can't be used together. That sucks.

If the transform were a property of particular function argument
positions, this wouldn't be a problem. You could declare, in effect,
that a certain function takes a transformed hstore, and this other one
takes a non-transformed hstore. Now life is good. But that's not
what is being proposed.

You could argue that such a level of granularity is overkill, but
frankly I've never had a real good feeling about the likelihood of
these transforms getting installed in the first place. In theory, if
you're using hstore and you're using plperl, you ought to also install
hstore-plperl-transform, but I bet a significant percentage of people
won't. So I don't foresee a finite transition period after which
databases without transforms go away and all code is written with the
assumption that transforms are available; instead, I foresee different
people assuming different things and ending up with mutually
incompatible code bases.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

Allow me to temporarily skip important questions that you asked so that
we can focus on the main problem here. As soon as we decide how to
handle any kind of selectivity for the transforms, then I'm back to
answering the other things.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Let's review that, as there as been some confusion about that. The SQL
> standard syntax is

Thanks for that. I see that you reused parts of the concept and keywords
and implemented something specific to PostgreSQL from there. As there's
no collision within the command sets, I think that problem is cleared.

> I had proposed disallowing installing a transform that would affect
> existing functions. That was rejected or deemed unnecessary. You can't
> have it both ways. ;-)

Well I'm not so sure, that's the point.

>> A mechanism allowing for the transform to only be used in some functions
>> but not others might be useful. The simplest such mechanism I can think
>> of is modeled against the PL/Java classpath facility as specified in the
>> SQL standard: you attach a classpath per schema.
>
> Anything that's a problem per-database would also be a problem per
> schema.

But a smaller problem, and as problem get smaller they get easier to
reason about and fix too. In the example given by Robert Haas in the
same thread, you could install extension A in schema A using the
transforms for hstore and plperl and extension B in schema B not using
the same transforms.

I'm not saying using the schema that way is awesome, just that we have
solid precedent in the standard and in pljava, and it looks easy enough
to implement (we already have search_path invalidations IIRC).

> This is a transition problem. Nobody is required to install the
> transforms into their existing databases. They probably shouldn't.

I disagree.

> How many people actually use hstore with PL/Perl or PL/Python now?
> Probably not many, because it's weird.
>
> I like to think about how this works for new development: Here is my
> extension type, here is how it interfaces with languages. Once you have
> established that, you don't want to have to repeat that every time you
> write a function. That's error prone and cumbersome. And anything
> that's set per schema or higher is a dependency tracking and caching mess.

The problem is installing a set of extensions where some of them are
already using the new transform feature and some of them are not. We
need a way to cater with that, I think.

> Also, extension types should work the same as built-in types.
> Eventually, I'd like to rip out the hard-coded data type support in
> PL/Python and replace it with built-in transforms. Even if we don't
> actually do it, conceptually it should be possible. Now if we require

I like that idea a lot. I don't see how you can make it work by default,
as like Robert I think the transition phase you're talking about will
never end.

> "USING TRANSFORM FOR int, bytea" every time, we'd have taken a big step
> back. Effectively, we already have built-in transforms in PL/Python.

For core types only.

> We have added a few more over the years. It's been a bit of a pain from
> time to time. At least, with this feature we'd be moving this decision
> into user space and give people a way to fix things. (Incidentally, if
> you add a lot of transforms, you are probably dealing with a strongly
> typed language. And a strongly typed language is more likely to cleanly
> catch type errors resulting from changes in the transforms.)

I think use space will want to be able to use code written using
different sets of transforms for the same set of types, rather than
being forced into upgrading their whole code at once.

It reminds me of the python 2 to python 3 upgrade path. This is not
solved yet, with libs that are python2 only and others that are python3
only, and OS that would ship some but not others.

We already have the problem that shipping contrib by default is frowned
upon at some places, then they miss out of plenty of awesome extensions.
Transforms with no granularity is only going to make it worse, I think.

So we have to choose the granularity:

- per database (current patch),
- per schema (standard has precedents with pljava,
- per extension (forcing users into using extensions, not good),
- per function (hard to maintain),
- something else.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On 11/12/2013 12:21 PM, Peter Eisentraut wrote:
> A transform is an SQL object that supplies to functions for converting
> between data types and procedural languages.
How hard would it be to extend this to add transforms directly
between pairs of procedural languages ?

One example would be calling a pl/v8 function from pl/python
and converting directly between integers in both, without going
through PostgreSQL type.

Another and maybe even more interesting would be automatic
null-transforms between two pl/python functions.

Cheers

--
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ

On 11/20/2013 10:58 PM, Robert Haas wrote:
> On Wed, Nov 20, 2013 at 11:51 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>> This is a transition problem. Nobody is required to install the
>> transforms into their existing databases. They probably shouldn't.
> Sure, but that's like saying "nobody's required to use this
> behavior-changing GUC, so it's OK to have a behavior-changing GUC".
>
> The point I think Dimitri is making, which IMHO is entirely valid, is
> that the feature as currently designed is database-wide. You either
> get this behavior for all of your functions, or you get it for none of
> them, and that might well not be what you want. For example, it's
> easy to imagine that you might want to install extensions A and B. A
> expects that a certain transform is loaded into the database, and B
> expects that it isn't. You now have created a situation where
> extensions A and B can't be used together. That sucks.
>
> If the transform were a property of particular function argument
> positions, this wouldn't be a problem. You could declare, in effect,
> that a certain function takes a transformed hstore, and this other one
> takes a non-transformed hstore. Now life is good. But that's not
> what is being proposed.
You mean something like

CREATE FUNCTION f(i int, h1 hstore USING TRANSFORM x, h2 hstore) ...

where h1 would go through transform x and 1 and h2
would use "default transform" ?

Cheers

--
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ

On 11/15/2013 05:04 PM, Dimitri Fontaine wrote:
> Hi,
>
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
>> Rebased patch. No changes except that merge conflicts were resolved,
>> and I had to add some Data::Dumper tweaks to the regression tests so
>> that the results came out in consistent order on different versions of
>> Perl.
....
> On the higher level design, the big question here is about selective
> behavior. As soon as you CREATE TRANSFORM FOR hstore LANGUAGE plperl
> then any plperl function will now receive its hstore arguments as a
> proper perl hash rather than a string.
>
> Any pre-existing plperl function with hstore arguments or return type
> then needs to be upgraded to handle the new types nicely, and some of
> those might not be under the direct control of the DBA running the
> CREATE TRANSFORM command, when using some plperl extensions for example.
>
> A mechanism allowing for the transform to only be used in some functions
> but not others might be useful. The simplest such mechanism I can think
> of is modeled against the PL/Java classpath facility as specified in the
> SQL standard: you attach a classpath per schema.
If we start adding granularity, then why not go all the way?

I mean, we could do it in the following way

1) create named transforms

CREATE [DEFAULT] TRANSFORM <xformname> FOR <type> LANGUAGE <lang> (...details...);

2) use it when declaring a function

CREATE function <funcname>(
IN <argname> <type> [[USING] [TRANSFORM] <xformname>],
INOUT <argname> <type> [[USING] [IN] [TRANSFORM] <xformname>] [[USING] [OUT] [TRANSFORM] <xformname>],
OUT <argname> <type> [[USING] [TRANSFORM] <xformname>],

...
) LANGUAGE <lang> $$
<funcdef>
$$;

This approach allows full flexibility in using "old" packages, especially
if we define old transform behaviour as "DEFAULT TRANSFORM"

Default transforms also allow easy way for rewriting current type i/o
conversions between languages into transforms.

There are immediately a few transforms that I would find useful

A) pass field data to language as pairs of (typeoid, typebin)

this is useful for speed, especially if you do not want to use many
of the passed arguments on most invocations

B) pass field data in as (typeoid, typebin), except do not de-toast
values but
pass in the toast ids, so the function is free to use only parts of
toasted values as it needs

C) pass field data in as string, probably the default behaviour for
languages like pl/tcl and pl/sh

D) and then of course just having a sensible transforms for extension
types like the current patch provides.

> Worst case, that I really don't think we need, would be addressing that
> per-argument:
>
> CREATE FUNCTION foo (hash hstore WITH TRANSFORM, kv hstore) …
>
> I certainly hope we don't need that, and sure can't imagine use cases
> for that level of complexity at the time of writing this review.
>
A typical use case would be to have a "short" hstore always passed in as
dictionary
and have another possibly large hstore passed in as toast pointer.

And if we want to have all type conversions between postgres and pls
re-written
as transforms, then we do need named transforms, not just one per (pl,
type) pair.

Also, if we allow flexibility, the it is probably a good idea to
implement full flexibility
first and then look at making usage easy after that, instead of adding
flexibility in
small steps.

Once we have per-argument transforms in place, we can look at setting
per-schema
defaults for ease of use.

As large part of this is actually abstracting i/o conversions out of pl
function code,
I think we should look at allowing the conversion functions to be
written in the
target pl language in addition to C.

I'll see if I can resurrect my patch for support of "cstring" and
"internal" types in pl/python
function defs for this.

--
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ

On Tue, 2013-11-26 at 18:10 +0100, Dimitri Fontaine wrote:
> The problem is installing a set of extensions where some of them are
> already using the new transform feature and some of them are not. We
> need a way to cater with that, I think.

Here is an idea. Add a GUC that basically says something like
use_transforms = on|off. You can then attach that to individual
functions, which is the right granularity, because only the function
knows whether its code expects transforms or not. But you can use the
full power of GUC to configure it any way you want.

The only thing this doesn't give you is per-argument granularity, but I
think the use cases for that are slim, and we don't have a good existing
mechanism to attach arbitrary attributes to function arguments.

Actually, I'd take this two steps further.

First, make this parameter per-language, so something like
plpython.use_transforms. Then it's up to the language implementation
how they want to deal with this. A future new language could just
ignore the whole issue and require transforms from the start.

Second, depending on the choice of the language, this parameter could
take three values: ignore | if available | require. That would allow
users to set various kinds of strictness, for example if they want to be
alerted that a language cannot deal with a particular type.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Here is an idea. Add a GUC that basically says something like
> use_transforms = on|off. You can then attach that to individual
> functions, which is the right granularity, because only the function
> knows whether its code expects transforms or not. But you can use the
> full power of GUC to configure it any way you want.

+1

> The only thing this doesn't give you is per-argument granularity, but I
> think the use cases for that are slim, and we don't have a good existing
> mechanism to attach arbitrary attributes to function arguments.

+1

> Actually, I'd take this two steps further.
>
> First, make this parameter per-language, so something like
> plpython.use_transforms. Then it's up to the language implementation
> how they want to deal with this. A future new language could just
> ignore the whole issue and require transforms from the start.

I'm not sure about this level of granularity, but why not.

> Second, depending on the choice of the language, this parameter could
> take three values: ignore | if available | require. That would allow
> users to set various kinds of strictness, for example if they want to be
> alerted that a language cannot deal with a particular type.

My understanding is that it always can deal with any particular type if
you consider text based input/output, right?

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

On 12/06/2013 07:25 AM, Peter Eisentraut wrote:
> On Tue, 2013-11-26 at 18:10 +0100, Dimitri Fontaine wrote:
>> The problem is installing a set of extensions where some of them are
>> already using the new transform feature and some of them are not. We
>> need a way to cater with that, I think.
> Here is an idea. Add a GUC that basically says something like
> use_transforms = on|off. You can then attach that to individual
> functions, which is the right granularity, because only the function
> knows whether its code expects transforms or not. But you can use the
> full power of GUC to configure it any way you want.
It would requite the "old" extensions to be modified to have
(SET use_transforms = off) in all their definitions so that they
would not accidentally be called with use_transforms = on
from "new" functions, but else it seems like a good way to get
it done without too much effort.

> The only thing this doesn't give you is per-argument granularity, but I
> think the use cases for that are slim, and we don't have a good existing
> mechanism to attach arbitrary attributes to function arguments.
Agreed. And we are quite unlikely to need multiple transforms for
the same type in the same language.
> Actually, I'd take this two steps further.
>
> First, make this parameter per-language, so something like
> plpython.use_transforms. Then it's up to the language implementation
> how they want to deal with this. A future new language could just
> ignore the whole issue and require transforms from the start.
I do not really see much need for this, as it will need to be set for
each individual function anyway.

Actually what we could do is just declare a new "language" for this
so functions declared with "LANGUAGE plpythonu" will not be using
transforms and those with "LANGUAGE plpythonuxf" will use it.

This would only need one extra function to be defined in source
code, namely the compile function for the "new" language".

Some not-transforms-related wild ideas follow :)

Adding a new language would also be a good way to fix the bad syntax
choices in pl/python which require code manipulation before compiling .

I came up with this idea after seeing how pl/jsv8 supports multiple
JavaScript-based languages (standard JavaScript, CoffeeScript, LiveScript)
from the same codebase.

Taking the plv8 ideas further we could also create a JavaScript-based
"sandboxed python" using thins like skulpt and pyjamas which compile
python source code to JavaScript VM and inherit all the sandboxing of
v8.

Cheers

--
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ

On Fri, 2013-12-06 at 11:28 +0100, Dimitri Fontaine wrote:
> > Here is an idea. Add a GUC that basically says something like
> > use_transforms = on|off. You can then attach that to individual
> > functions, which is the right granularity, because only the function
> > knows whether its code expects transforms or not. But you can use
> the
> > full power of GUC to configure it any way you want.

Here is an updated patch that implements this, makes some of the
documentation improvements that you suggested, and rebases everything.

On Tue, Dec 10, 2013 at 10:35 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On Fri, 2013-12-06 at 11:28 +0100, Dimitri Fontaine wrote:
>> > Here is an idea. Add a GUC that basically says something like
>> > use_transforms = on|off. You can then attach that to individual
>> > functions, which is the right granularity, because only the function
>> > knows whether its code expects transforms or not. But you can use
>> the
>> > full power of GUC to configure it any way you want.
>
> Here is an updated patch that implements this, makes some of the
> documentation improvements that you suggested, and rebases everything.

I'm still kinda unimpressed by this. Behavior-changing GUC, uggh.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 12/11/2013 01:40 PM, Robert Haas wrote:
> On Tue, Dec 10, 2013 at 10:35 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>> On Fri, 2013-12-06 at 11:28 +0100, Dimitri Fontaine wrote:
>>>> Here is an idea. Add a GUC that basically says something like
>>>> use_transforms = on|off. You can then attach that to individual
>>>> functions, which is the right granularity, because only the function
>>>> knows whether its code expects transforms or not. But you can use
>>> the
>>>> full power of GUC to configure it any way you want.
>> Here is an updated patch that implements this, makes some of the
>> documentation improvements that you suggested, and rebases everything.
> I'm still kinda unimpressed by this. Behavior-changing GUC, uggh.
>
It should work ok if we could somehow check that the GUC is set
on the function and fall back to session GUC in case it is not.

Not sure if this is possible though.

The need from this arises from calling other functions from a new func.
At the moment if there is a new function defined as

CREATE FUNCTION f_uses_xforms() AS $$ ... $$ SET use_transforms=on;

calls a legacy function which will break if transforms are used then the
_old_ function declaration needs to be modified to add (use_transforms=off)

It is much easier than debugging/rewriting the function, but this is
something I'd like us to be able to avoid.

PS. maybe we could resurrect the WITH (attribute, ...) available in
CREATE FUNCTION syntax for passing function-specific flags ?

Cheers

--
Hannu Krosing
PostgreSQL Consultant
Performance, Scalability and High Availability
2ndQuadrant Nordic OÜ

On Wed, Dec 11, 2013 at 9:19 AM, Hannu Krosing <hannu(at)2ndquadrant(dot)com> wrote:
> On 12/11/2013 01:40 PM, Robert Haas wrote:
>> On Tue, Dec 10, 2013 at 10:35 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>>> On Fri, 2013-12-06 at 11:28 +0100, Dimitri Fontaine wrote:
>>>>> Here is an idea. Add a GUC that basically says something like
>>>>> use_transforms = on|off. You can then attach that to individual
>>>>> functions, which is the right granularity, because only the function
>>>>> knows whether its code expects transforms or not. But you can use
>>>> the
>>>>> full power of GUC to configure it any way you want.
>>> Here is an updated patch that implements this, makes some of the
>>> documentation improvements that you suggested, and rebases everything.
>> I'm still kinda unimpressed by this. Behavior-changing GUC, uggh.
>>
> It should work ok if we could somehow check that the GUC is set
> on the function and fall back to session GUC in case it is not.
>
> Not sure if this is possible though.
>
> The need from this arises from calling other functions from a new func.
> At the moment if there is a new function defined as
>
> CREATE FUNCTION f_uses_xforms() AS $$ ... $$ SET use_transforms=on;
>
> calls a legacy function which will break if transforms are used then the
> _old_ function declaration needs to be modified to add (use_transforms=off)

Yeah, exactly.

> It is much easier than debugging/rewriting the function, but this is
> something I'd like us to be able to avoid.
>
> PS. maybe we could resurrect the WITH (attribute, ...) available in
> CREATE FUNCTION syntax for passing function-specific flags ?

It's a thought. Or you could put some annotation in the function
body, as we do in PL/pgsql with the #option syntax.

Of course, making everyone decorate their new functions with
references to the transforms they want to use isn't wonderful either,
but it might be good at least to have the option. You could allow the
use of all installed transforms by default, but let people say WITH
(transforms='') if they don't want to use them or WITH
(transforms='comma, separated, list') if the want to require certain
ones.

Unfortunately, that'll probably mean that virtually all portable code
for procedural languages has to include some form of this incantation,
just as any nearly any PL/pgsql function has to include SET
search_path = '' if it wants to be not trivially subvertable. It's
annoying to grow more such decoration, but the alternative seems to be
hoping that nobody wants to write portable code that uses non-core
types, and that doesn't seem better.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, Dec 10, 2013 at 10:35 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>> Here is an updated patch that implements this, makes some of the
>> documentation improvements that you suggested, and rebases everything.

> I'm still kinda unimpressed by this. Behavior-changing GUC, uggh.

We should have learned by now that those are usually a bad idea.
In this case, we've got changes in the behavior of function calling,
which seems like not only a nightmare for debugging but a fertile
source of security issues.

regards, tom lane

On Wed, 2013-12-11 at 11:07 -0500, Tom Lane wrote:
> We should have learned by now that those are usually a bad idea.
> In this case, we've got changes in the behavior of function calling,
> which seems like not only a nightmare for debugging but a fertile
> source of security issues.

I note that this is the same mechanism that we have elaborately designed
for *avoiding* security issues from search_path.

On Wed, 2013-12-11 at 09:47 -0500, Robert Haas wrote:
> Of course, making everyone decorate their new functions with
> references to the transforms they want to use isn't wonderful either,
> but it might be good at least to have the option. You could allow the
> use of all installed transforms by default, but let people say WITH
> (transforms='') if they don't want to use them or WITH
> (transforms='comma, separated, list') if the want to require certain
> ones.

I'll try to implement something like that.

On Fri, Jan 10, 2014 at 10:40 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On Wed, 2013-12-11 at 11:07 -0500, Tom Lane wrote:
>> We should have learned by now that those are usually a bad idea.
>> In this case, we've got changes in the behavior of function calling,
>> which seems like not only a nightmare for debugging but a fertile
>> source of security issues.
>
> I note that this is the same mechanism that we have elaborately designed
> for *avoiding* security issues from search_path.

And it works like crap.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, 2014-01-10 at 22:46 -0500, Peter Eisentraut wrote:
> On Wed, 2013-12-11 at 09:47 -0500, Robert Haas wrote:
> > Of course, making everyone decorate their new functions with
> > references to the transforms they want to use isn't wonderful either,
> > but it might be good at least to have the option. You could allow the
> > use of all installed transforms by default, but let people say WITH
> > (transforms='') if they don't want to use them or WITH
> > (transforms='comma, separated, list') if the want to require certain
> > ones.
>
> I'll try to implement something like that.

So it turns out the SQL standard actually specifies it more or less that
way.

What I have implemented now is that you can attach a clause TRANSFORM
{ ALL | NONE | FOR TYPE xxx, ... } to CREATE FUNCTION, which is recorded
in pg_proc, so there is no interference by run-time parameters. I kept
the use_transforms parameter to set the default value (again, at CREATE
FUNCTION time). pg_dump never dumps TRANSFORM ALL, always the resolved
type list, so you get the exact behavior back after restores or
upgrades.

The only place where this currently breaks is SPI calls inside
functions, because CREATE FUNCTION doesn't know about them. That can be
worked around by providing an explicit type list, but that would in turn
interfere with what you want to do with the parameter list. Also, there
is no way to attach a TRANSFORM clause to inline calls (DO).

The attached patch will probably fail to apply because of the pg_proc
changes. So if you want to try it out, look into the header for the Git
hash it was based off. I'll produce a properly merged version when this
approach is validated. Also, some implementation details could probably
take some revising after a couple of nights of sleep. ;-)

On 2014-01-15 21:13:18 -0500, Peter Eisentraut wrote:
> The attached patch will probably fail to apply because of the pg_proc
> changes. So if you want to try it out, look into the header for the Git
> hash it was based off. I'll produce a properly merged version when this
> approach is validated. Also, some implementation details could probably
> take some revising after a couple of nights of sleep. ;-)

You've slept since? ;)

So, I am only doign a look through the patch, to see where it has gone
in the past year.

> index 4e476c3..5ac9f05 100644
> --- a/src/Makefile.shlib
> +++ b/src/Makefile.shlib
> @@ -133,7 +133,7 @@ ifeq ($(PORTNAME), darwin)
> else
> # loadable module
> DLSUFFIX = .so
> - LINK.shared = $(COMPILER) -bundle -multiply_defined suppress
> + LINK.shared = $(COMPILER) -bundle -multiply_defined suppress -Wl,-undefined,dynamic_lookup
> endif
> BUILD.exports = $(AWK) '/^[^\#]/ {printf "_%s\n",$$1}' $< >$@
> exports_file = $(SHLIB_EXPORTS:%.txt=%.list)

Hm. Do the linkers on other platforms support this behaviour? Linux
does, by default, but I have zap clue about the rest.

Why do we need this? I guess because a transform from e.g. hstore to $pl
needs symbols out of hstore.so and the $pl?

I wonder if it woudln't be better to rely on explicit symbol lookups for
this. That'd avoid the need for the order dependency and linker changes
like this.

> + case OBJECT_TRANSFORM:
> + {
> + TypeName *typename = (TypeName *) linitial(objname);
> + char *langname = (char *) linitial(objargs);
> + Oid type_id = typenameTypeId(NULL, typename);
> + Oid lang_id = get_language_oid(langname, false);
> +
> + address.classId = TransformRelationId;
> + address.objectId =
> + get_transform_oid(type_id, lang_id, missing_ok);
> + address.objectSubId = 0;
> + }
> + break;

Hm. I wonder if missing_ok should be forwarded to get_language_oid() and
(by changing the way things are done atm) to typenameTypeId?

> + case OCLASS_TRANSFORM:
> + {
> + HeapTuple trfTup;
> + Form_pg_transform trfForm;
> +
> + trfTup = SearchSysCache1(TRFOID,
> + ObjectIdGetDatum(object->objectId));
> + if (!HeapTupleIsValid(trfTup))
> + elog(ERROR, "could not find tuple for transform %u",
> + object->objectId);
> +
> + trfForm = (Form_pg_transform) GETSTRUCT(trfTup);
> +
> + appendStringInfo(&buffer, _("transform for %s language %s"),
> + format_type_be(trfForm->trftype),
> + get_language_name(trfForm->trflang, false));
> +
> + ReleaseSysCache(trfTup);
> + break;
> + }
> +

Why deviate from the usual 'cache lookup failed for ..'? elog doesn't
translate so it's not particular relevant, but ...

> referenced.objectSubId = 0;
> recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
>
> + /* dependency on transform used by return type, if any */
> + if ((trfid = get_transform_oid(returnType, languageObjectId, true)))
> + {
> + referenced.classId = TransformRelationId;
> + referenced.objectId = trfid;
> + referenced.objectSubId = 0;
> + recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
> + }
> +

Should be compared to InvalidOid imo, rather than implicitly assuming
that InvalidOid evaluates to false.

> +/*
> + * CREATE TRANSFORM
> + */
> +Oid
> +CreateTransform(CreateTransformStmt *stmt)
> +{
...
> + if (!pg_type_ownercheck(typeid, GetUserId()))
> + aclcheck_error_type(ACLCHECK_NOT_OWNER, typeid);
> +
> + aclresult = pg_type_aclcheck(typeid, GetUserId(), ACL_USAGE);
> + if (aclresult != ACLCHECK_OK)
> + aclcheck_error_type(aclresult, typeid);
> +
> + /*
> + * Get the language
> + */
> + langid = get_language_oid(stmt->lang, false);
> +
> + aclresult = pg_language_aclcheck(langid, GetUserId(), ACL_USAGE);
> + if (aclresult != ACLCHECK_OK)
> + aclcheck_error(aclresult, ACL_KIND_LANGUAGE, stmt->lang);

Hm. Is USAGE really sufficient here? Should we possibly make it
dependant on lanpltrusted like CreateFunction() does?

> + if (stmt->fromsql)
> + {
> + fromsqlfuncid = LookupFuncNameTypeNames(stmt->fromsql->funcname, stmt->fromsql->funcargs, false);
> +
> + if (!pg_proc_ownercheck(fromsqlfuncid, GetUserId()))
> + aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_PROC, NameListToString(stmt->fromsql->funcname));

Why isn't EXECUTE sufficient here?

> + aclresult = pg_proc_aclcheck(fromsqlfuncid, GetUserId(), ACL_EXECUTE);
> + if (aclresult != ACLCHECK_OK)
> + aclcheck_error(aclresult, ACL_KIND_PROC, NameListToString(stmt->fromsql->funcname));
> +
> + tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(fromsqlfuncid));
> + if (!HeapTupleIsValid(tuple))
> + elog(ERROR, "cache lookup failed for function %u", fromsqlfuncid);
> + procstruct = (Form_pg_proc) GETSTRUCT(tuple);
> + if (procstruct->prorettype != INTERNALOID)
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> + errmsg("return data type of FROM SQL function must be \"internal\"")));
> + check_transform_function(procstruct);
> + ReleaseSysCache(tuple);

So, this can be used to call a function that takes INTERNAL, and returns
INTERNAL. Isn't that normally reserved for superusers? I think this at
the very least needs to be an ownercheck on the function?

> @@ -66,6 +66,7 @@ CATALOG(pg_proc,1255) BKI_BOOTSTRAP BKI_ROWTYPE_OID(81) BKI_SCHEMA_MACRO
> text proargnames[1]; /* parameter names (NULL if no names) */
> pg_node_tree proargdefaults;/* list of expression trees for argument
> * defaults (NULL if none) */
> + Oid protrftypes[1] /* types for which to apply transforms */
> text prosrc; /* procedure source text */
> text probin; /* secondary procedure info (can be NULL) */
> text proconfig[1]; /* procedure-local GUC settings */

I wonder if this shouldn't rather be a array that lists the transform to
be used for every single column akin to proargtypes or such. That's
going to make life easier for pl developers.
> /**********************************************************************
> @@ -1260,6 +1264,7 @@ static SV *plperl_call_perl_func(plperl_proc_desc *desc,
> bool *isnull)
> {
> FmgrInfo tmp;
> + Oid funcid;
>
> /* we might recurse */
> check_stack_depth();
> @@ -1283,6 +1288,8 @@ static SV *plperl_call_perl_func(plperl_proc_desc *desc,
> /* must call typinput in case it wants to reject NULL */
> return InputFunctionCall(finfo, NULL, typioparam, typmod);
> }
> + else if ((funcid = get_transform_tosql(typid, current_call_data->prodesc->lang_oid)))
> + return OidFunctionCall1(funcid, PointerGetDatum(sv));
> else if (SvROK(sv))
> {
> /* handle references */

Am I missing something here? You're not looking at the proc's
transforms, but just lookup the general ones? Same for output and such.

Looks like you forgot to update this.

> for (i = 0; i < desc->nargs; i++)
> {
> if (fcinfo->argnull[i])
> @@ -2055,9 +2075,16 @@ static SV *plperl_call_perl_func(plperl_proc_desc *desc,
> else
> {
> SV *sv;
> + Oid funcid;
>
> if (OidIsValid(desc->arg_arraytype[i]))
> sv = plperl_ref_from_pg_array(fcinfo->arg[i], desc->arg_arraytype[i]);
> + else if (list_member_oid(current_call_data->prodesc->trftypes, argtypes[i]))
> + {
> + funcid = get_transform_fromsql(argtypes[i], current_call_data->prodesc->lang_oid);
> + Assert(funcid); // TODO
> + sv = (SV *) DatumGetPointer(OidFunctionCall1(funcid, fcinfo->arg[i]));
> + }

This is the behaviour I'd really would like to avoid. Searching an array
for every parameter sucks. I think this really should be a vector with
one element per argument. Yes, that's going to require special handling
of the return type, but whatever.

So, I lost my motiviation here. I like this version *much* more than the
state of a year ago. I think there's a fair amount of work required
here, but it seems to be dilligence that's required, not redesign. But I
still don't think that's doable within the next couple of days?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2014-04-05 00:21:47 +0200, Andres Freund wrote:
> On 2014-01-15 21:13:18 -0500, Peter Eisentraut wrote:
> > The attached patch will probably fail to apply because of the pg_proc
> > changes. So if you want to try it out, look into the header for the Git
> > hash it was based off. I'll produce a properly merged version when this
> > approach is validated. Also, some implementation details could probably
> > take some revising after a couple of nights of sleep. ;-)
>
> You've slept since? ;)
>
> So, I am only doign a look through the patch, to see where it has gone
> in the past year.
> ...

So, unless somebody protests PDQ, I am going to mark this "Returned with
Feedback".

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

I have an updated patch for this. At the end of the 2014-01 commit
fest, it seems that the design was generally considered OK.

This patch is rebased, has some updates and some bug fixes.

Responses to the last review below.

On 4/4/14 6:21 PM, Andres Freund wrote:
>> index 4e476c3..5ac9f05 100644
>> --- a/src/Makefile.shlib
>> +++ b/src/Makefile.shlib
>> @@ -133,7 +133,7 @@ ifeq ($(PORTNAME), darwin)
>> else
>> # loadable module
>> DLSUFFIX = .so
>> - LINK.shared = $(COMPILER) -bundle -multiply_defined suppress
>> + LINK.shared = $(COMPILER) -bundle -multiply_defined suppress -Wl,-undefined,dynamic_lookup
>> endif
>> BUILD.exports = $(AWK) '/^[^\#]/ {printf "_%s\n",$$1}' $< >$@
>> exports_file = $(SHLIB_EXPORTS:%.txt=%.list)
>
> Hm. Do the linkers on other platforms support this behaviour? Linux
> does, by default, but I have zap clue about the rest.

I think all other platforms do this by default, or can be made to do so.

> Why do we need this? I guess because a transform from e.g. hstore to $pl
> needs symbols out of hstore.so and the $pl?
>
> I wonder if it woudln't be better to rely on explicit symbol lookups for
> this. That'd avoid the need for the order dependency and linker changes
> like this.

That seems quite difficult. For example, hstore has things like
HS_COUNT() and HS_VAL(), which aren't even symbols.

>> + case OBJECT_TRANSFORM:
>> + {
>> + TypeName *typename = (TypeName *) linitial(objname);
>> + char *langname = (char *) linitial(objargs);
>> + Oid type_id = typenameTypeId(NULL, typename);
>> + Oid lang_id = get_language_oid(langname, false);
>> +
>> + address.classId = TransformRelationId;
>> + address.objectId =
>> + get_transform_oid(type_id, lang_id, missing_ok);
>> + address.objectSubId = 0;
>> + }
>> + break;
>
> Hm. I wonder if missing_ok should be forwarded to get_language_oid() and
> (by changing the way things are done atm) to typenameTypeId?

done

>> + case OCLASS_TRANSFORM:
>> + {
>> + HeapTuple trfTup;
>> + Form_pg_transform trfForm;
>> +
>> + trfTup = SearchSysCache1(TRFOID,
>> + ObjectIdGetDatum(object->objectId));
>> + if (!HeapTupleIsValid(trfTup))
>> + elog(ERROR, "could not find tuple for transform %u",
>> + object->objectId);
>> +
>> + trfForm = (Form_pg_transform) GETSTRUCT(trfTup);
>> +
>> + appendStringInfo(&buffer, _("transform for %s language %s"),
>> + format_type_be(trfForm->trftype),
>> + get_language_name(trfForm->trflang, false));
>> +
>> + ReleaseSysCache(trfTup);
>> + break;
>> + }
>> +
>
> Why deviate from the usual 'cache lookup failed for ..'? elog doesn't
> translate so it's not particular relevant, but ...

That's how the surrounding code does it.

>> referenced.objectSubId = 0;
>> recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
>>
>> + /* dependency on transform used by return type, if any */
>> + if ((trfid = get_transform_oid(returnType, languageObjectId, true)))
>> + {
>> + referenced.classId = TransformRelationId;
>> + referenced.objectId = trfid;
>> + referenced.objectSubId = 0;
>> + recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
>> + }
>> +
>
> Should be compared to InvalidOid imo, rather than implicitly assuming
> that InvalidOid evaluates to false.

I think it's widely assumed that InvalidOid is false.

>> +/*
>> + * CREATE TRANSFORM
>> + */
>> +Oid
>> +CreateTransform(CreateTransformStmt *stmt)
>> +{
> ...
>> + if (!pg_type_ownercheck(typeid, GetUserId()))
>> + aclcheck_error_type(ACLCHECK_NOT_OWNER, typeid);
>> +
>> + aclresult = pg_type_aclcheck(typeid, GetUserId(), ACL_USAGE);
>> + if (aclresult != ACLCHECK_OK)
>> + aclcheck_error_type(aclresult, typeid);
>> +
>> + /*
>> + * Get the language
>> + */
>> + langid = get_language_oid(stmt->lang, false);
>> +
>> + aclresult = pg_language_aclcheck(langid, GetUserId(), ACL_USAGE);
>> + if (aclresult != ACLCHECK_OK)
>> + aclcheck_error(aclresult, ACL_KIND_LANGUAGE, stmt->lang);
>
> Hm. Is USAGE really sufficient here? Should we possibly make it
> dependant on lanpltrusted like CreateFunction() does?

It could be done, but I don't see why it's necessary. If the language
isn't trusted, why grant USAGE on it?

>> + if (stmt->fromsql)
>> + {
>> + fromsqlfuncid = LookupFuncNameTypeNames(stmt->fromsql->funcname, stmt->fromsql->funcargs, false);
>> +
>> + if (!pg_proc_ownercheck(fromsqlfuncid, GetUserId()))
>> + aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_PROC, NameListToString(stmt->fromsql->funcname));
>
> Why isn't EXECUTE sufficient here?

because of the below

>> + aclresult = pg_proc_aclcheck(fromsqlfuncid, GetUserId(), ACL_EXECUTE);
>> + if (aclresult != ACLCHECK_OK)
>> + aclcheck_error(aclresult, ACL_KIND_PROC, NameListToString(stmt->fromsql->funcname));
>> +
>> + tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(fromsqlfuncid));
>> + if (!HeapTupleIsValid(tuple))
>> + elog(ERROR, "cache lookup failed for function %u", fromsqlfuncid);
>> + procstruct = (Form_pg_proc) GETSTRUCT(tuple);
>> + if (procstruct->prorettype != INTERNALOID)
>> + ereport(ERROR,
>> + (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
>> + errmsg("return data type of FROM SQL function must be \"internal\"")));
>> + check_transform_function(procstruct);
>> + ReleaseSysCache(tuple);
>
> So, this can be used to call a function that takes INTERNAL, and returns
> INTERNAL. Isn't that normally reserved for superusers? I think this at
> the very least needs to be an ownercheck on the function?

exactly, see above

>> @@ -66,6 +66,7 @@ CATALOG(pg_proc,1255) BKI_BOOTSTRAP BKI_ROWTYPE_OID(81) BKI_SCHEMA_MACRO
>> text proargnames[1]; /* parameter names (NULL if no names) */
>> pg_node_tree proargdefaults;/* list of expression trees for argument
>> * defaults (NULL if none) */
>> + Oid protrftypes[1] /* types for which to apply transforms */
>> text prosrc; /* procedure source text */
>> text probin; /* secondary procedure info (can be NULL) */
>> text proconfig[1]; /* procedure-local GUC settings */
>
> I wonder if this shouldn't rather be a array that lists the transform to
> be used for every single column akin to proargtypes or such. That's
> going to make life easier for pl developers.

That would allow using different transforms for different arguments of
the same type, which I don't want to allow, and PLs might not be able to
handle.

I understand where you are coming from, but the alternative seems worse.

>> /**********************************************************************
>> @@ -1260,6 +1264,7 @@ static SV *plperl_call_perl_func(plperl_proc_desc *desc,
>> bool *isnull)
>> {
>> FmgrInfo tmp;
>> + Oid funcid;
>>
>> /* we might recurse */
>> check_stack_depth();
>> @@ -1283,6 +1288,8 @@ static SV *plperl_call_perl_func(plperl_proc_desc *desc,
>> /* must call typinput in case it wants to reject NULL */
>> return InputFunctionCall(finfo, NULL, typioparam, typmod);
>> }
>> + else if ((funcid = get_transform_tosql(typid, current_call_data->prodesc->lang_oid)))
>> + return OidFunctionCall1(funcid, PointerGetDatum(sv));
>> else if (SvROK(sv))
>> {
>> /* handle references */
>
> Am I missing something here? You're not looking at the proc's
> transforms, but just lookup the general ones? Same for output and such.
>
> Looks like you forgot to update this.

fixed

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On 4/4/14 6:21 PM, Andres Freund wrote:

> + /* dependency on transform used by return type, if any */
> + if ((trfid = get_transform_oid(returnType, languageObjectId, true)))

>> Should be compared to InvalidOid imo, rather than implicitly assuming
>> that InvalidOid evaluates to false.

> I think it's widely assumed that InvalidOid is false.

That's not the point; the point is that some nonzero number of compilers
(and probably Coverity too) will warn about this construct, on the not
unreasonable grounds that = might be a typo for ==. (Those extra parens
might satisfy gcc, but not other tools.) Please put in an explicit
comparison of the assignment result, as is done in approximately 100% of
the other places where this idiom appears in Postgres.

regards, tom lane

On Mon, Dec 15, 2014 at 3:10 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> fixed
This patch needs a rebase, it does not apply correctly in a couple of
places on latest HEAD (699300a):
./src/include/catalog/catversion.h.rej
./src/include/catalog/pg_proc.h.rej
./src/pl/plpython/plpy_procedure.c.rej
Regards,
--
Michael

On Mon, Dec 22, 2014 at 12:19 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com
> wrote:

> On Mon, Dec 15, 2014 at 3:10 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> > fixed
> This patch needs a rebase, it does not apply correctly in a couple of
> places on latest HEAD (699300a):
> ./src/include/catalog/catversion.h.rej
> ./src/include/catalog/pg_proc.h.rej
> ./src/pl/plpython/plpy_procedure.c.rej
>

I moved this patch to 2015-02 to not lose track of it and because it did
not receive much reviews...
--
Michael

Hi

I am checking this patch, but it is broken still

Regards

Pavel

2015-02-13 8:14 GMT+01:00 Michael Paquier <michael(dot)paquier(at)gmail(dot)com>:

>
>
> On Mon, Dec 22, 2014 at 12:19 PM, Michael Paquier <
> michael(dot)paquier(at)gmail(dot)com> wrote:
>
>> On Mon, Dec 15, 2014 at 3:10 PM, Peter Eisentraut <peter_e(at)gmx(dot)net>
>> wrote:
>> > fixed
>> This patch needs a rebase, it does not apply correctly in a couple of
>> places on latest HEAD (699300a):
>> ./src/include/catalog/catversion.h.rej
>> ./src/include/catalog/pg_proc.h.rej
>> ./src/pl/plpython/plpy_procedure.c.rej
>>
>
> I moved this patch to 2015-02 to not lose track of it and because it did
> not receive much reviews...
> --
> Michael
>

On 3/6/15 9:56 AM, Pavel Stehule wrote:
> Hi
>
> I am checking this patch, but it is broken still

Here is an updated patch.

(Note that because of the large pg_proc.h changes, it is likely to get
outdated again soon. But for reviewing, you can always apply it against
an older version of master.)

Hi

I am looking to code.

Some small issues:

1. fix missing semicolon pg_proc.h

Oid protrftypes[1]; /* types for which to apply
transforms */

2. strange load lib by in sql scripts:

DO '' LANGUAGE plperl;
SELECT NULL::hstore;

use load plperl; load hstore; instead

3. missing documentation for new contrib modules,

4. pg_dump - wrong comment

+<-----><------>/*
+<-----><------> * protrftypes was added at v9.4
+<-----><------> */

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.

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.

Regards

Pavel

2015-03-08 16:34 GMT+01:00 Peter Eisentraut <peter_e(at)gmx(dot)net>:

> On 3/6/15 9:56 AM, Pavel Stehule wrote:
> > Hi
> >
> > I am checking this patch, but it is broken still
>
> Here is an updated patch.
>
> (Note that because of the large pg_proc.h changes, it is likely to get
> outdated again soon. But for reviewing, you can always apply it against
> an older version of master.)
>
>

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.

> 2. strange load lib by in sql scripts:
>
> DO '' LANGUAGE plperl;
> SELECT NULL::hstore;
>
> use load plperl; load hstore; instead

OK

> 3. missing documentation for new contrib modules,

OK

> 4. pg_dump - wrong comment
>
> +<-----><------>/*
> +<-----><------> * protrftypes was added at v9.4
> +<-----><------> */

OK

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

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

2015-03-17 2:51 GMT+01:00 Peter Eisentraut <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.
>
> > 2. strange load lib by in sql scripts:
> >
> > DO '' LANGUAGE plperl;
> > SELECT NULL::hstore;
> >
> > use load plperl; load hstore; instead
>
> OK
>
> > 3. missing documentation for new contrib modules,
>
> OK
>
> > 4. pg_dump - wrong comment
> >
> > +<-----><------>/*
> > +<-----><------> * protrftypes was added at v9.4
> > +<-----><------> */
>
> OK
>
> > 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.

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

Regards

Pavel

On Mon, Mar 16, 2015 at 9:51 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>> 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 think we need to the ability to control it per-function, but having
a global disabling knob on top of that doesn't seem especially useful.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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.

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

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

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

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

On 3/22/15 5:46 AM, Pavel Stehule wrote:
> 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.

Following our off-list conversation, here is a new patch that removes
the TRANSFORM ALL/NONE clauses and requires an explicit list.

2015-04-08 4:55 GMT+02:00 Peter Eisentraut <peter_e(at)gmx(dot)net>:

> On 3/22/15 5:46 AM, Pavel Stehule wrote:
> > 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.
>
> Following our off-list conversation, here is a new patch that removes
> the TRANSFORM ALL/NONE clauses and requires an explicit list.
>

Nice, thank you very much

I checked the description of this feature in other databases and in
standard. There is little bit different situations - because there is not
possibility to use external languages without transformations. In
PostgreSQL we living without transformations long years, so we have to
solve a co-existence old code (or functions without the using
transformations) and new code (functions that use transformations). We have
to solve a possible compatibility issues. The last design requires to
specify explicitly list of types that are transformed. Nothing
transformation is used by default (implicitly). So old code have to work
without any issues and new code is clearly marked. Now I don't afraid of
introduction of transformations. It is safe.

Review
----------
1. What it does? - it introduce a secondary way, how to pass values between
PostgreSQL and PL languages.

2. Does we would this feature? Surely - we would. It is safe way for
passing complex types more cleverly and use it in PL more comfortably.
Enhancing work with hstore in PLPerl, PLPython is long desired feature.

3. It can enforce some compatibility issues? No, last design is safe - the
using transformation of any type must be explicitly enabled. It is clean
what types will be transformed, and when transformations is required and
when not.

4. I was able to apply patch cleanly - there are no compilation warnings.

5. This feature is documented well - new SQL statements, new system tables.

6. Code is clean and well documented.

7. This feature has enough regress tests - all tests passed without
problems.

8. It requires pg_dump support. I checked it - no problems

I have not any objection. I'll mark it as ready for commit.

Minor issue is missing support for \sf command in psql. I wrote small patch
that fix it.

Thank you very much for on this pretty nice feature.

Regards

Pavel

On Tue, Apr 7, 2015 at 7:55 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:

> On 3/22/15 5:46 AM, Pavel Stehule wrote:
> > 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.
>
> Following our off-list conversation, here is a new patch that removes
> the TRANSFORM ALL/NONE clauses and requires an explicit list.
>

Hi Peter,

This commit is causing a compiler warning for me in non-cassert builds:

funcapi.c: In function 'get_func_trftypes':
funcapi.c:890: warning: unused variable 'procStruct'

Adding PG_USED_FOR_ASSERTS_ONLY seems to fix it.

Cheers,

Jeff

On Tue, Apr 28, 2015 at 12:47 PM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
> This commit is causing a compiler warning for me in non-cassert builds:
>
> funcapi.c: In function 'get_func_trftypes':
> funcapi.c:890: warning: unused variable 'procStruct'
>
> Adding PG_USED_FOR_ASSERTS_ONLY seems to fix it.

I took a stab at fixing this via a slightly different method. Let me
know whether that worked.

Thanks,

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Apr 29, 2015 at 11:27 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Tue, Apr 28, 2015 at 12:47 PM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
> > This commit is causing a compiler warning for me in non-cassert builds:
> >
> > funcapi.c: In function 'get_func_trftypes':
> > funcapi.c:890: warning: unused variable 'procStruct'
> >
> > Adding PG_USED_FOR_ASSERTS_ONLY seems to fix it.
>
> I took a stab at fixing this via a slightly different method. Let me
> know whether that worked.
>

It worked, thanks.

On Wed, Apr 29, 2015 at 3:21 PM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
> On Wed, Apr 29, 2015 at 11:27 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Tue, Apr 28, 2015 at 12:47 PM, Jeff Janes <jeff(dot)janes(at)gmail(dot)com> wrote:
>> > This commit is causing a compiler warning for me in non-cassert builds:
>> >
>> > funcapi.c: In function 'get_func_trftypes':
>> > funcapi.c:890: warning: unused variable 'procStruct'
>> >
>> > Adding PG_USED_FOR_ASSERTS_ONLY seems to fix it.
>>
>> I took a stab at fixing this via a slightly different method. Let me
>> know whether that worked.
>
> It worked, thanks.

Cool.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company