Thread: revised patch for PL/PgSQL table functions
This is an improved version of the PL/PgSQL table function
patch. Since the last patch, I've implemented support for returning
ROWTYPE variables and scalar expressions under RETURN NEXT, changed
the grammar a little bit, and added some regression tests.
Remaining to be done:
- write the documentation, add a few more regression tests
- fix the grammar: I'm still using the 'NEXT' hack instead of
'RETURN NEXT'. It may also be better to use 'RETURN xxx AND
RESUME' instead.
- memory management: stop using TopTransactionContext. This
isn't that urgent, IMHO.
Cheers,
Neil
--
Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC
Attachment
Neil Conway <neilc@samurai.com> writes: > This is an improved version of the PL/PgSQL table function patch. Here's another improved patch. I got the parser to accept 'RETURN NEXT' properly (thanks to Frank Ch. Eigler @ RH for his help), although the solution might not be perfect: I changed the scanner to accept 'return next' as a single token (allowing for a variable amount of whitespace between the first and second words, of course). If you have a better suggestion, let me know. I also improved some unrelated areas of the documentation a little bit and made some minor code cleanups. I'd like to see this patch get into 7.3beta1 -- AFAIK, the only thing remaining to be done is to write the docs. If I need to do anything else, let me know. Cheers, Neil -- Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC
Attachment
Neil Conway <neilc@samurai.com> writes:
> Here's another improved patch. I got the parser to accept 'RETURN
> NEXT' properly (thanks to Frank Ch. Eigler @ RH for his help),
> although the solution might not be perfect: I changed the scanner to
> accept 'return next' as a single token (allowing for a variable amount
> of whitespace between the first and second words, of course). If you
> have a better suggestion, let me know.
Consider doing it the way that the main parser converts "UNION JOIN"
into a single token --- viz, there's an outer filter that calls the
lexer an extra time to look ahead one token when necessary. Doing this
in the lexer is really quite messy if you want to do it right (eg,
deal correctly with comments between the two keywords).
> I'd like to see this patch get into 7.3beta1 --
Me too ;-). I am planning to review your PREPARE patch next, then look
at Joe's SRF stuff in general, and then this ...
regards, tom lane
[ Tom Lane is reviewing this patch.]
Your patch has been added to the PostgreSQL unapplied patches list at:
http://candle.pha.pa.us/cgi-bin/pgpatches
I will try to apply it within the next 48 hours.
---------------------------------------------------------------------------
Neil Conway wrote:
> Neil Conway <neilc@samurai.com> writes:
> > This is an improved version of the PL/PgSQL table function patch.
>
> Here's another improved patch. I got the parser to accept 'RETURN
> NEXT' properly (thanks to Frank Ch. Eigler @ RH for his help),
> although the solution might not be perfect: I changed the scanner to
> accept 'return next' as a single token (allowing for a variable amount
> of whitespace between the first and second words, of course). If you
> have a better suggestion, let me know.
>
> I also improved some unrelated areas of the documentation a little bit
> and made some minor code cleanups.
>
> I'd like to see this patch get into 7.3beta1 -- AFAIK, the only thing
> remaining to be done is to write the docs. If I need to do anything
> else, let me know.
>
> Cheers,
>
> Neil
>
> --
> Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
> http://archives.postgresql.org
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Tom Lane <tgl@sss.pgh.pa.us> writes: > Neil Conway <neilc@samurai.com> writes: > > Here's another improved patch. I got the parser to accept 'RETURN > > NEXT' properly > Consider doing it the way that the main parser converts "UNION JOIN" > into a single token --- viz, there's an outer filter that calls the > lexer an extra time to look ahead one token when necessary. Doing this > in the lexer is really quite messy if you want to do it right (eg, > deal correctly with comments between the two keywords). Ah, ok -- thanks for the tip. I've attached a revised patch: rediffed against CVS HEAD, changed the RETURN NEXT parsing as suggested by Tom above, and made some more minor cleanups to PL/PgSQL code. Cheers, Neil -- Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC
Attachment
Neil Conway <neilc@samurai.com> writes:
> I've attached a revised patch: rediffed against CVS HEAD, changed the
> RETURN NEXT parsing as suggested by Tom above, and made some more
> minor cleanups to PL/PgSQL code.
I've applied this patch with some editorializing --- mainly, I didn't
like hardwiring the functionality to plpgsql, so I extended the
ReturnSetInfo interface instead. Now anybody can use the
return-a-tuplestore mechanism for SRFs.
There is a rather nasty bug left (Sir Mordred would likely call it a
DOS possibility ;-)) --- RETURN NEXT doesn't seem to be checking that
the row or record variable it is given actually matches the declared
return type of the plpgsql function. You could probably cause problems
by passing the wrong thing, or even more subtly by passing the right
thing on the first RETURN NEXT and then wrong things later. This
needs to be tightened up.
Another thing to think about is extending plpgsql to support functions
declared to return RECORD, ie, the first value actually passed to RETURN
or RETURN NEXT determines the result tuple type. I won't cry if we
don't get that feature into 7.3, though.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> writes: > I've applied this patch with some editorializing --- mainly, I didn't > like hardwiring the functionality to plpgsql, so I extended the > ReturnSetInfo interface instead. Now anybody can use the > return-a-tuplestore mechanism for SRFs. Thanks Tom. > There is a rather nasty bug left (Sir Mordred would likely call it a > DOS possibility ;-)) --- RETURN NEXT doesn't seem to be checking that > the row or record variable it is given actually matches the declared > return type of the plpgsql function. Yes, I probably should have mentioned that. I considered adding the code to generate a TupleDesc for each call of RETURN NEXT and compare that to the TupleDesc used for the previous RETURN NEXT statement, but that seems to be quite expensive: equalTupleDescs() is not cheap, and neither is TyoeGetTupleDesc() (which is required for returning non-RECORD vars). Since RETURN NEXT will often be called many times within a single function, the performance hit seems unappealing. Is there a better way? Cheers, Neil -- Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC
Neil Conway <neilc@samurai.com> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> There is a rather nasty bug left (Sir Mordred would likely call it a
>> DOS possibility ;-)) --- RETURN NEXT doesn't seem to be checking that
>> the row or record variable it is given actually matches the declared
>> return type of the plpgsql function.
> Yes, I probably should have mentioned that. I considered adding the
> code to generate a TupleDesc for each call of RETURN NEXT and compare
> that to the TupleDesc used for the previous RETURN NEXT statement, but
> that seems to be quite expensive: equalTupleDescs() is not cheap, and
> neither is TyoeGetTupleDesc() (which is required for returning
> non-RECORD vars). Since RETURN NEXT will often be called many times
> within a single function, the performance hit seems unappealing. Is
> there a better way?
Well, you don't need or want a full equalTupleDescs() comparison.
All you care about is that the number and types of the columns match
--- see tupledesc_mismatch() in nodeFunctionscan.c. That doesn't
seem terribly expensive. The hard part seems to be getting the tupdesc
to compare to. For record variables it seems to be available for free,
but I'm less sure about the other two cases. Probably row variables
could be modified to store a tupdesc. Not sure what's the best thing
to do with scalar results.
As for TypeGetTupleDesc, that's a red herring: you shouldn't need to do
that more than once, because it's used to compute a tupdesc for the
declared function return type, which isn't gonna change. I'm not sure
you need do it at all in the row or record cases --- you should be
looking to the actual type of the given data, not the declared function
type (think about RECORD-returning functions).
regards, tom lane
Neil Conway <neilc@samurai.com> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> There is a rather nasty bug left (Sir Mordred would likely call it a
>> DOS possibility ;-)) --- RETURN NEXT doesn't seem to be checking that
>> the row or record variable it is given actually matches the declared
>> return type of the plpgsql function.
> Yes, I probably should have mentioned that.
I've applied a fix for this.
The fix actually uses the "expected tuple desc" that's now passed by
ExecMakeTableFunctionResult as the target descriptor. This should mean
that it'd be possible to support plpgsql functions returning RECORD, but
I didn't have time to look into that. Anyone want to try?
regards, tom lane
Tom Lane wrote: > The fix actually uses the "expected tuple desc" that's now passed by > ExecMakeTableFunctionResult as the target descriptor. This should mean > that it'd be possible to support plpgsql functions returning RECORD, but > I didn't have time to look into that. Anyone want to try? I haven't really looked too closely at Neil plpgsql stuff, so I'm a bit concerned with the spin-up time I'd need to figure this out. But if Neil doesn't show up and volunteer between now and Saturday morning, I'll take a look. Joe
Joe Conway <mail@joeconway.com> writes: > I haven't really looked too closely at Neil plpgsql stuff, so I'm a > bit concerned with the spin-up time I'd need to figure this out. But > if Neil doesn't show up and volunteer between now and Saturday > morning, I'll take a look. I can do this -- I should hopefully be able to get it done by the end of the weekend, but I can't make any promises. I assume that an SRF returning 'RECORD' defined in PL/PgSQL would still need to be called with a column definition list, right? Given that it's about 4AM here and I just took a 30-sec look at Tom's changes to the SRF code, forgive me if this is incorrect: I would think that the PL/PgSQL func would examine ReturnSetInfo.expectedDesc when processing a SETOF RECORD function, and use that to confirm that the RECORD has the appropriate TupleDesc, right? Cheers, Neil -- Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC
Neil Conway <neilc@samurai.com> writes:
> Given that it's about 4AM here and I just took a 30-sec look at Tom's
> changes to the SRF code, forgive me if this is incorrect: I would
> think that the PL/PgSQL func would examine ReturnSetInfo.expectedDesc
> when processing a SETOF RECORD function, and use that to confirm that
> the RECORD has the appropriate TupleDesc, right?
Actually, it does that already: exec_stmt_return_next relies on the
expectedDesc to check the value being output in all cases. So for a
SETOF RECORD function, the additional work required might be as simple
as just opening up the check in plpgsql_compile to allow RECORD return
type. For the non-SETOF case (table function returning a single tuple),
I think exec_stmt_return would work okay as long as plpgsql_compile had
set fn_retistuple true for RECORD.
But I haven't tested it, and there might be other places in plpgsql that
examine the declared return type and would need tweaking. I have other
fish to fry before beta, so no time...
regards, tom lane
Dear Friend,
I need to access/retrieve data from multiple databases
using single query ...
Is there any syntax ...?
Thanks in advance
Regards
Vishnu.
Tom Lane <tgl@sss.pgh.pa.us> writes: > Actually, it does that already: exec_stmt_return_next relies on the > expectedDesc to check the value being output in all cases. So for a > SETOF RECORD function, the additional work required might be as simple > as just opening up the check in plpgsql_compile to allow RECORD return > type. For the non-SETOF case (table function returning a single tuple), > I think exec_stmt_return would work okay as long as plpgsql_compile had > set fn_retistuple true for RECORD. Okay, here's a patch that implements this -- no additional changes to PL/PgSQL were needed, as far as I could tell. I've added some regression tests that cover this new functionality and they seem to work as expected. Unless anyone sees a problem, please apply. Cheers, Neil -- Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC
Attachment
Neil Conway <neilc@samurai.com> writes:
> Okay, here's a patch that implements this -- no additional changes to
> PL/PgSQL were needed, as far as I could tell. I've added some
> regression tests that cover this new functionality and they seem to
> work as expected.
Patch applied, with a couple minor documentation/comment fixes.
At this point I believe we have full table function support (return
a tuple, return a set, return RECORD) for SQL and plpgsql functions.
That seems like a clean stopping point for 7.3. But there's no
support at all for these features in pltcl, plperl, or plpython.
Bruce, would you add a TODO item?
* Add table function support to pltcl, plperl, plpython
regards, tom lane
Tom Lane wrote: > Neil Conway <neilc@samurai.com> writes: > > Okay, here's a patch that implements this -- no additional changes to > > PL/PgSQL were needed, as far as I could tell. I've added some > > regression tests that cover this new functionality and they seem to > > work as expected. > > Patch applied, with a couple minor documentation/comment fixes. > > At this point I believe we have full table function support (return > a tuple, return a set, return RECORD) for SQL and plpgsql functions. > That seems like a clean stopping point for 7.3. But there's no > support at all for these features in pltcl, plperl, or plpython. > Bruce, would you add a TODO item? > > * Add table function support to pltcl, plperl, plpython Added to TODO. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073