Re: [PATCH] Add transforms feature - Mailing list pgsql-hackers

Hi,

Peter Eisentraut <peter_e@gmx.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
abetter   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
atleast) 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
transformfunctions. I   agree with the need, just wanted to raise awereness about that in   case there's something to
besaid 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
changethe   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
reasonwhy 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