Add support for SRF and returning composites to pl/tcl

Hi

I checked this code, and it looks well

0. there are not any reason why we would not to implement this feature -
more, the implementation is simple.

1. there was not problem with patching, compilation
2. the original patch is missing new expected result for regress tests,
fixed in attached patch
3. all regress tests passed
4. the tests and docs is enough for this purpose

I'll mark this patch as ready for commit

Regards

Pavel

2016-10-13 0:06 GMT+02:00 Jim Nasby <Jim.Nasby@bluetreble.com>:

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

Attached is a patch that adds support for SRFs and returning composites
from pl/tcl. This work was sponsored by Flight Aware.

I spent a fair amount of time whacking this around, because I did not
like the fact that you were using the pltcl_proc_desc structs for
call-local data. That would fail nastily in a recursive function.
I ended up making a new struct to represent per-call data, which
allowed reducing the number of global pointers.

I got the code to a state that I liked (attached), and started reviewing
the docs, and then it occurred to me to wonder why you'd chosen to use
Tcl lists to represent composite output values. The precedent established
by input argument handling is that composites are transformed to Tcl
arrays. So shouldn't we use an array to represent a composite result,
too?

I wouldn't necessarily object to allowing either representation, though
I'm not sure how we'd distinguish between them.

regards, tom lane

2016-11-06 2:12 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us>:

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

Attached is a patch that adds support for SRFs and returning composites
from pl/tcl. This work was sponsored by Flight Aware.

I spent a fair amount of time whacking this around, because I did not
like the fact that you were using the pltcl_proc_desc structs for
call-local data. That would fail nastily in a recursive function.
I ended up making a new struct to represent per-call data, which
allowed reducing the number of global pointers.

I got the code to a state that I liked (attached), and started reviewing
the docs, and then it occurred to me to wonder why you'd chosen to use
Tcl lists to represent composite output values. The precedent established
by input argument handling is that composites are transformed to Tcl
arrays. So shouldn't we use an array to represent a composite result,
too?

This can be similar to PLPythonu - one dimensional array is possible to
transform to composite - when composite is expected.

Regards

Pavel

I wrote:

I got the code to a state that I liked (attached), and started reviewing
the docs, and then it occurred to me to wonder why you'd chosen to use
Tcl lists to represent composite output values. The precedent established
by input argument handling is that composites are transformed to Tcl
arrays. So shouldn't we use an array to represent a composite result,
too?

After further nosing around I see that the return-a-tuple-as-a-list
concept is already embedded in pltcl_trigger_handler. So the
inconsistency is already there, and it's not necessarily this patch's
job to fix it. Still seems like we might want to allow using an array
directly rather than insisting on conversion to a list, but that's a
task for a separate patch.

We should, however, make some attempt to ensure that the list-to-tuple
conversion semantics are the same in both cases. In particular I notice
some undocumented behavior around a magic ".tupno" element. Will see
about cleaning that up.

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

On 11/6/16 12:15 PM, Tom Lane wrote:

I wrote:

I got the code to a state that I liked (attached), and started reviewing
the docs, and then it occurred to me to wonder why you'd chosen to use
Tcl lists to represent composite output values. The precedent established
by input argument handling is that composites are transformed to Tcl
arrays. So shouldn't we use an array to represent a composite result,
too?

After further nosing around I see that the return-a-tuple-as-a-list
concept is already embedded in pltcl_trigger_handler. So the
inconsistency is already there, and it's not necessarily this patch's
job to fix it. Still seems like we might want to allow using an array
directly rather than insisting on conversion to a list, but that's a
task for a separate patch.

My understanding is that the TCL community is of mixed feelings when it
comes to arrays vs lists. It does seem worth adding array support though.

We should, however, make some attempt to ensure that the list-to-tuple
conversion semantics are the same in both cases. In particular I notice
some undocumented behavior around a magic ".tupno" element. Will see
about cleaning that up.

Hrm, I completely spaced on the fact that composite returns are
essentially the same thing as trigger returns. ISTM we should be able to
use the same code for both. IIRC those magic elements could end up in
any SPI result, so that handling certainly needs to be the same.

Have you had a chance to look at this or should I?
--
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
855-TREBLE2 (855-873-2532) mobile: 512-569-9461

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

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

Hrm, I completely spaced on the fact that composite returns are
essentially the same thing as trigger returns. ISTM we should be able to
use the same code for both. IIRC those magic elements could end up in
any SPI result, so that handling certainly needs to be the same.

Have you had a chance to look at this or should I?

As things stand in HEAD, the behavior is about the same, but the error
messages are not --- in one case they mention triggers and of course the
other doesn't. There are a couple of other minor things in the way of
unifying the two hunks of code, so I concluded it probably wasn't worth
the trouble. But feel free to take another look if it bugs you.

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

On 11/8/16 8:33 AM, Tom Lane wrote:

As things stand in HEAD, the behavior is about the same, but the error
messages are not --- in one case they mention triggers and of course the
other doesn't. There are a couple of other minor things in the way of
unifying the two hunks of code, so I concluded it probably wasn't worth
the trouble. But feel free to take another look if it bugs you.

I had to add a bit of cruft to pltcl_build_tuple_result but it's not
that bad. tg_tupdesc could potentially be eliminated, but I don't know
if it's really worth it.

Note that this does change some of the trigger error messages, but I
don't think that's really an issue?
--
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
855-TREBLE2 (855-873-2532) mobile: 512-569-9461

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

On 11/8/16 8:33 AM, Tom Lane wrote:

As things stand in HEAD, the behavior is about the same, but the error
messages are not --- in one case they mention triggers and of course the
other doesn't. There are a couple of other minor things in the way of
unifying the two hunks of code, so I concluded it probably wasn't worth
the trouble. But feel free to take another look if it bugs you.

I had to add a bit of cruft to pltcl_build_tuple_result but it's not
that bad. tg_tupdesc could potentially be eliminated, but I don't know
if it's really worth it.

Note that this does change some of the trigger error messages, but I
don't think that's really an issue?

The only thing that seems significant is that we'd change the SQLSTATE
for the "odd number of list items" error: pltcl_trigger_handler has

ereport(ERROR,
(errcode(ERRCODE_E_R_I_E_TRIGGER_PROTOCOL_VIOLATED),
errmsg("trigger's return list must have even number of elements")));

and that would become

ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("column name/value list must have even number of elements")));

But that's probably fine; it's hard to believe anyone is depending on this
particular case --- and I think the argument that this is legitimately
a TRIGGER_PROTOCOL_VIOLATED case is a bit thin anyway.

While I was checking the patch to verify that it didn't change any
behavior, I noticed that it did, and there's a pre-existing bug here:
pltcl_build_tuple_result is applying utf_e2u to the Tcl_GetString results,
but surely that should be utf_u2e instead. Fortunately we haven't shipped
this code yet.

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

On 1/6/17 2:45 PM, Tom Lane wrote:

The only thing that seems significant is that we'd change the SQLSTATE
for the "odd number of list items" error: pltcl_trigger_handler has

(errcode(ERRCODE_E_R_I_E_TRIGGER_PROTOCOL_VIOLATED),
errmsg("trigger's return list must have even number of elements")));

and that would become

(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("column name/value list must have even number of elements")));

But that's probably fine; it's hard to believe anyone is depending on this
particular case --- and I think the argument that this is legitimately
a TRIGGER_PROTOCOL_VIOLATED case is a bit thin anyway.

Yeah, I forgot to mention that, but I did look at both cases and felt
the same.

While I was checking the patch to verify that it didn't change any
behavior, I noticed that it did, and there's a pre-existing bug here:
pltcl_build_tuple_result is applying utf_e2u to the Tcl_GetString results,
but surely that should be utf_u2e instead. Fortunately we haven't shipped
this code yet.

Ugh.

For this patch lets just fix that (see attached). Moving forward, I
think it'd be good to improve this...

There's only 2 cases of Tcl_GetString that don't then call utf_u2e (or,
amusingly, UTF_U2E); perhaps we should just get rid of all direct use of
TclGetString and replace it with something like pltcl_get_utf() and
pltcl_get_ascii()?

Oh, hell, now I see that there's an actual difference between utf_* and
UTF_*. And AFAICT, those macros don't completely solve things either; if
you make multiple calls you're still leaking memory but wouldn't know
it. Granted, existing calls seem to all be properly wrapped, but that
seems pretty error prone.

Looking at plpython, it seems that it doesn't go through any of this
trouble: if you're using python 3 it just treats everything as if it
needs encoding, and the conversion routines handle freeing things.
AFAICT that's true for identifiers as well.

So I'm thinking that we should just forbid the use of direct Tcl string
functions and pass everything though our own functions that will handle
UTF8. There are a bunch of places where we're handing static C strings
(that are clearly plain ASCII) to TCL, so we should probably continue to
support that. But in the other direction I don't think it's worth it.

TCL does have a separate string append operation as well, so we'll need
to either provide that or do all the appending using our string
functions and then pass that along.
--
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
855-TREBLE2 (855-873-2532)

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

On 1/6/17 2:45 PM, Tom Lane wrote:

While I was checking the patch to verify that it didn't change any
behavior, I noticed that it did, and there's a pre-existing bug here:
pltcl_build_tuple_result is applying utf_e2u to the Tcl_GetString results,
but surely that should be utf_u2e instead. Fortunately we haven't shipped
this code yet.

Ugh.

For this patch lets just fix that (see attached).

I already fixed it in HEAD.

Moving forward, I think it'd be good to improve this...

Yeah, it's pretty ugly, but I'm not sure what would be a better design.

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