Re: [HACKERS] [PATCH] Add transforms feature

[Robert Haas]

On Wed, Apr 29, 2015 at 3:21 PM, Jeff Janes  wrote:
> On Wed, Apr 29, 2015 at 11:27 AM, Robert Haas  wrote:
>> On Tue, Apr 28, 2015 at 12:47 PM, Jeff Janes  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Jeff Janes]

On Wed, Apr 29, 2015 at 11:27 AM, Robert Haas  wrote:

> On Tue, Apr 28, 2015 at 12:47 PM, Jeff Janes  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.

Re: [HACKERS] [PATCH] Add transforms feature

[Robert Haas]

On Tue, Apr 28, 2015 at 12:47 PM, Jeff Janes  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Jeff Janes]

On Tue, Apr 7, 2015 at 7:55 PM, Peter Eisentraut  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


transform_unused.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Pavel Stehule]

2015-04-08 4:55 GMT+02:00 Peter Eisentraut :

> 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
commit 7a0139a3d82b0d71682c317ff560799254986504
Author: Pavel Stehule 
Date:   Wed Apr 8 09:50:30 2015 +0200

fix \sf when function has trf types

diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 3ee3b57..5ffb712 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -306,6 +306,7 @@ static text *pg_get_expr_worker(text *expr, Oid relid, const char *relname,
 static int print_function_arguments(StringInfo buf, HeapTuple proctup,
 		 bool print_table_args, bool print_defaults);
 static void print_function_rettype(StringInfo buf, HeapTuple proctup);
+static void print_function_trftypes(StringInfo buf, HeapTuple proctup);
 static void set_rtable_names(deparse_namespace *dpns, List *parent_namespaces,
  Bitmapset *rels_used);
 static bool refname_is_unique(char *refname, deparse_namespace *dpns,
@@ -1945,6 +1946,9 @@ pg_get_functiondef(PG_FUNCTION_ARGS)
 	(void) print_function_arguments(&buf, proctup, false, true);
 	appendStringInfoString(&buf, ")\n RETURNS ");
 	print_function_rettype(&buf, proctup);
+
+	print_function_trftypes(&buf, proctup);
+
 	appendStringInfo(&buf, "\n LANGUAGE %s\n",
 	 quote_identifier(get_language_name(proc->prolang, false)));
 
@@ -2342,6 +2346,30 @@ is_input_argument(int nth, const char *argmodes)
 }
 
 /*
+ * Append used transformated types to specified buffer
+ */
+static void
+print_function_trftypes(StringInfo buf, HeapTuple proctup)
+{
+	Oid	*trftypes;
+	int	ntypes;
+
+	ntypes = get_func_trftypes(proctup, &trftypes);
+	if (ntypes > 0)
+	{
+		int	i;
+
+		appendStringInfoString(buf, "\n TRANSFORM ");
+		for (i = 0; i < ntypes; i++)
+		{
+			if (i != 0)
+appendStringInfoString(buf, ", ");
+appendStringInfo(buf, "FOR TYPE %s", format_type_be(trftypes[i]));
+		}
+	}
+}
+
+/*
  * Get textual representation of a function argument's default value.  The
  * second argument of this function is the argument number among all arguments
  * (i.e. proallargtypes, *not* proargtypes), starting with 1, because that's
diff --git a/src/backend/utils/fmgr/funcapi.c b/src/backend/utils/fmgr/funcapi.c
index 204e124..ebd7ddd 100644
--- a/src/backend/utils/fmgr/funcapi.c
+++ b/src/backend/utils/fmgr/funcapi.c
@@ -877,6 +877,50 @@ get_func_arg_info(HeapTuple procTup,
 	return numargs;
 }
 
+/*
+ * get_func_trftypes
+ *
+ * Returns a number of transformated types used by function.
+ */
+int
+get_func_trftypes(HeapTuple procTup,
+  Oid **p_trftypes)
+{
+
+	Form_pg_proc procStruct = (Form_pg_proc) GETSTRUCT(procTup);
+	Datum		protrftypes;
+	ArrayType  *arr;
+	int			nelems;
+	bool			isNull;
+
+	protrftypes 

Re: [HACKERS] [PATCH] Add transforms feature

[Pavel Stehule]

2015-03-22 3:55 GMT+01:00 Peter Eisentraut :

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

Re: [HACKERS] [PATCH] Add transforms feature

[Pavel Stehule]

2015-03-22 5:45 GMT+01:00 Pavel Stehule :

>
>
> 2015-03-22 3:55 GMT+01:00 Peter Eisentraut :
>
>> 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 > > >:
>> >
>> > 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
>
>

Re: [HACKERS] [PATCH] Add transforms feature

[Pavel Stehule]

2015-03-22 3:55 GMT+01:00 Peter Eisentraut :

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

Re: [HACKERS] [PATCH] Add transforms feature

[Robert Haas]

On Mon, Mar 16, 2015 at 9:51 PM, Peter Eisentraut  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Pavel Stehule]

2015-03-17 2:51 GMT+01:00 Peter Eisentraut :

> 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

Re: [HACKERS] [PATCH] Add transforms feature

[Peter Eisentraut]

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



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Pavel Stehule]

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 :

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

Re: [HACKERS] [PATCH] Add transforms feature

[Pavel Stehule]

Hi

I am checking this patch, but it is broken still

Regards

Pavel




2015-02-13 8:14 GMT+01:00 Michael Paquier :

>
>
> On Mon, Dec 22, 2014 at 12:19 PM, Michael Paquier <
> michael.paqu...@gmail.com> wrote:
>
>> On Mon, Dec 15, 2014 at 3:10 PM, Peter Eisentraut 
>> 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
>

Re: [HACKERS] [PATCH] Add transforms feature

[Michael Paquier]

On Mon, Dec 22, 2014 at 12:19 PM, Michael Paquier  wrote:

> On Mon, Dec 15, 2014 at 3:10 PM, Peter Eisentraut  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

Re: [HACKERS] [PATCH] Add transforms feature

[Michael Paquier]

On Mon, Dec 15, 2014 at 3:10 PM, Peter Eisentraut  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Tom Lane]

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Andres Freund]

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Andres Freund]

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

Re: [HACKERS] [PATCH] Add transforms feature

[Robert Haas]

On Fri, Jan 10, 2014 at 10:40 PM, Peter Eisentraut  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Peter Eisentraut]

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.



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Peter Eisentraut]

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.



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Tom Lane]

Robert Haas  writes:
> On Tue, Dec 10, 2013 at 10:35 PM, Peter Eisentraut  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Robert Haas]

On Wed, Dec 11, 2013 at 9:19 AM, Hannu Krosing  wrote:
> On 12/11/2013 01:40 PM, Robert Haas wrote:
>> On Tue, Dec 10, 2013 at 10:35 PM, Peter Eisentraut  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Hannu Krosing]

On 12/11/2013 01:40 PM, Robert Haas wrote:
> On Tue, Dec 10, 2013 at 10:35 PM, Peter Eisentraut  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Ü



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Robert Haas]

On Tue, Dec 10, 2013 at 10:35 PM, Peter Eisentraut  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Hannu Krosing]

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Ü



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Dimitri Fontaine]

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Peter Eisentraut]

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.




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Hannu Krosing]

On 11/15/2013 05:04 PM, Dimitri Fontaine wrote:
> Hi,
>
> Peter Eisentraut  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  FOR  LANGUAGE  
(...details...);

2) use it when declaring a function

CREATE function (
IN   [[USING] [TRANSFORM] ],
INOUT   [[USING] [IN] [TRANSFORM] ] [[USING] 
[OUT] [TRANSFORM] ],
OUT   [[USING] [TRANSFORM] ],

 ... 
) LANGUAGE  $$

$$;

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Ü



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Hannu Krosing]

On 11/20/2013 10:58 PM, Robert Haas wrote:
> On Wed, Nov 20, 2013 at 11:51 AM, Peter Eisentraut  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Ü



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Hannu Krosing]

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Ü



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Dimitri Fontaine]

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Robert Haas]

On Wed, Nov 20, 2013 at 11:51 AM, Peter Eisentraut  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Peter Eisentraut]

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   (...details...);

and then there is

SET DEFAULT TRANSFORM GROUP 
SET TRANSFORM GROUP FOR TYPE  

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  LANGUAGE  (...details...);

So you could consider LANGUAGE  to be the implicit transform group
of language , 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 pe

Re: [HACKERS] [PATCH] Add transforms feature

[Dimitri Fontaine]

Hi,

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: CREATE TRANSFORM syntax (was Re: [HACKERS] [PATCH] Add transforms feature)

[Josh Berkus]

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

CREATE TRANSFORM syntax (was Re: [HACKERS] [PATCH] Add transforms feature)

[Peter Eisentraut]

On Mon, 2013-07-08 at 23:00 -0700, Hitoshi Harada wrote:
> On Sun, Jul 7, 2013 at 12:06 PM, Peter Eisentraut 
> 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?



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Josh Berkus]

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Hitoshi Harada]

On Thu, Jul 4, 2013 at 2:18 AM, Hitoshi Harada  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Hitoshi Harada]

On Sun, Jul 7, 2013 at 12:06 PM, Peter Eisentraut  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 
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.


--

Re: [HACKERS] [PATCH] Add transforms feature

[Josh Berkus]

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Peter Eisentraut]

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.




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Peter Eisentraut]

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.




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Dimitri Fontaine]

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Josh Berkus]

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Hitoshi Harada]

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

Re: [HACKERS] [PATCH] Add transforms feature

[Peter Eisentraut]

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.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Hitoshi Harada]

On Thu, Jun 13, 2013 at 8:11 PM, 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.
>
>
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

Re: [HACKERS] [PATCH] Add transforms feature

[Josh Berkus]

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Peter Eisentraut]

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.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Craig Ringer]

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



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Alvaro Herrera]

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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Peter Eisentraut]

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.



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Craig Ringer]

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



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Peter Eisentraut]

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.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Add transforms feature

[Cédric Villemain]

Peter Eisentraut  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é.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers