revised patch for PL/PgSQL table functions

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

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

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

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