Horton

Thread

Commits

  1. Merge two copies of tuple-building code in pltcl.c.

  1. Add support for SRF and returning composites to pl/tcl

    Jim Nasby <jim.nasby@bluetreble.com> — 2016-10-12T22:06:13Z

    Attached is a patch that adds support for SRFs and returning composites 
from pl/tcl. This work was sponsored by Flight Aware.
-- 
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

  2. Re: Add support for SRF and returning composites to pl/tcl

    Pavel Stehule <pavel.stehule@gmail.com> — 2016-11-05T20:39:47Z

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

> Attached is a patch that adds support for SRFs and returning composites
> from pl/tcl. This work was sponsored by Flight Aware.
> --
> 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
>
>

  3. Re: Add support for SRF and returning composites to pl/tcl

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-11-06T01:12:20Z

    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

  4. Re: Add support for SRF and returning composites to pl/tcl

    Pavel Stehule <pavel.stehule@gmail.com> — 2016-11-06T05:09:58Z

    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 wouldn't necessarily object to allowing either representation, though
> I'm not sure how we'd distinguish between them.
>
>                         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
>
>

  5. Re: Add support for SRF and returning composites to pl/tcl

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-11-06T18:15:28Z

    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

  6. Re: Add support for SRF and returning composites to pl/tcl

    Jim Nasby <jim.nasby@bluetreble.com> — 2016-11-07T22:09:57Z

    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

  7. Re: Add support for SRF and returning composites to pl/tcl

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-11-08T14:33:34Z

    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

  8. Re: Add support for SRF and returning composites to pl/tcl

    Jim Nasby <jim.nasby@bluetreble.com> — 2016-11-10T00:41:40Z

    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

  9. Re: Add support for SRF and returning composites to pl/tcl

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-06T20:45:11Z

    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

  10. Re: Add support for SRF and returning composites to pl/tcl

    Jim Nasby <jim.nasby@bluetreble.com> — 2017-01-08T04:57:45Z

    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)

  11. Re: Add support for SRF and returning composites to pl/tcl

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-08T05:30:14Z

    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