BUG #13960: plpython fails with certain function names

Jim.Nasby@BlueTreble.com writes:

If a Postgres function contains characters that are illegal for python
identifiers, compilation fails. Error message is not very helpful either:

Hm, how much do we really care? The example seems kinda artificial.

One possibility is to simply strip out invalid characters[1].

No, because then you would get collisions, ie function names that look
different to PG would look the same to python. Bad news.

(Actually, don't we have that issue anyway because of schemas? I wonder
why we are exposing the PG name of the function to python at all.)

regards, tom lane

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

On 2/14/16 6:49 PM, Tom Lane wrote:

Jim.Nasby@BlueTreble.com writes:

If a Postgres function contains characters that are illegal for python
identifiers, compilation fails. Error message is not very helpful either:

Hm, how much do we really care? The example seems kinda artificial.

I did run across it creating cast functions programmatically, roughly as

format( 'CREATE FUNCTION "ndarray_to_%1s"', input_type::regtype )

which blows up when you feed it something like float. The actual example
was doing something similar but for an array.

I agree it's not exactly a high priority thing to support, but the error
text is completely useless unless you happen to know that plpython is
creating an actual python function.

One possibility is to simply strip out invalid characters[1].

No, because then you would get collisions, ie function names that look
different to PG would look the same to python. Bad news.

IIRC we append the regproc OID to ensure it's unique.

(Actually, don't we have that issue anyway because of schemas? I wonder
why we are exposing the PG name of the function to python at all.)

Not sure. Maybe useful in a call stack inside python? I can't really
think of what else you'd use it for. (I assume you're suggesting just
call the function names something like plpython_<regproc::oid>.)
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

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

Jim Nasby <Jim.Nasby@bluetreble.com> writes:

On 2/14/16 6:49 PM, Tom Lane wrote:

No, because then you would get collisions, ie function names that look
different to PG would look the same to python. Bad news.

IIRC we append the regproc OID to ensure it's unique.

Ah, okay, problem solved.

(Actually, don't we have that issue anyway because of schemas? I wonder
why we are exposing the PG name of the function to python at all.)

Not sure. Maybe useful in a call stack inside python? I can't really
think of what else you'd use it for. (I assume you're suggesting just
call the function names something like plpython_<regproc::oid>.)

Yeah, that's what I was thinking about. But yes, if we append the OID
anyway then we might as well just strip all non-alphanumeric chars
from the name. Safe and you still get some debuggability.

regards, tom lane

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

On 2/14/16 7:09 PM, Tom Lane wrote:

Yeah, that's what I was thinking about. But yes, if we append the OID
anyway then we might as well just strip all non-alphanumeric chars
from the name. Safe and you still get some debuggability.

Attached. I opted not to modify the name in-place. If it's safe to
modify in place, might want to just replace invalid characters with '_'
instead of making a copy.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

Jim Nasby <Jim.Nasby@bluetreble.com> writes:

On 2/14/16 7:09 PM, Tom Lane wrote:

Yeah, that's what I was thinking about. But yes, if we append the OID
anyway then we might as well just strip all non-alphanumeric chars
from the name. Safe and you still get some debuggability.

Attached. I opted not to modify the name in-place. If it's safe to
modify in place, might want to just replace invalid characters with '_'
instead of making a copy.

I like the idea of replacing invalid characters with '_'. It's definitely
not safe to scribble on the pg_proc tuple, but we could get the same
result with a few wasted cycles by rescanning the procName string after
building it, as per attached.

regards, tom lane

On 2/16/16 7:49 PM, Tom Lane wrote:

I like the idea of replacing invalid characters with '_'. It's definitely
not safe to scribble on the pg_proc tuple, but we could get the same
result with a few wasted cycles by rescanning the procName string after
building it, as per attached.

Heck, I didn't even think about that. Yeah, it's going to scan another
20 bytes or so, but this certainly isn't performance critical.

BTW, I didn't bother checking this with python 3.5, but I can't fathom
how that would matter here.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

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

Jim Nasby <Jim.Nasby@bluetreble.com> writes:

BTW, I didn't bother checking this with python 3.5, but I can't fathom
how that would matter here.

Me either, but the buildfarm will soon tell us if it does.

regards, tom lane

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

Tom Lane wrote, On 2016-02-17 02:49:

+ 	/* Replace any not-legal-in-Python-names characters with '_' */
+ 	for (ptr = procName; *ptr; ptr++)
+ 	{
+ 		if (!((*ptr >= 'A' && *ptr <= 'Z') ||
+ 			  (*ptr >= 'a' && *ptr <= 'z') ||
+ 			  (*ptr >= '0' && *ptr <= '9')))
+ 			*ptr = '_';
+ 	}
+ 

That may blow on names that start with a digit. Maybe

+ (*ptr >= '0' && *ptr <= '9' && ptr != procName)))

Making the testcase start with a digit sounds like a good idea.

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

On 2/17/16 8:36 PM, Pedro Gimeno wrote:

Tom Lane wrote, On 2016-02-17 02:49:

+ 	/* Replace any not-legal-in-Python-names characters with '_' */
+ 	for (ptr = procName; *ptr; ptr++)
+ 	{
+ 		if (!((*ptr >= 'A' && *ptr <= 'Z') ||
+ 			  (*ptr >= 'a' && *ptr <= 'z') ||
+ 			  (*ptr >= '0' && *ptr <= '9')))
+ 			*ptr = '_';
+ 	}
+

That may blow on names that start with a digit. Maybe

+ (*ptr >= '0' && *ptr <= '9' && ptr != procName)))

Making the testcase start with a digit sounds like a good idea.

Postgres prefaces the name of the python function with a fixed string,
so it will never start with a digit.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com

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