Re: first cut at PL/PgSQL table functions

Neil Conway <neilc@samurai.com> writes:

[ new improved address, eh? ]

> The attached patch is my first attempt at implementing table functions
> for PL/PgSQL. The approach I used is (apparently) the method suggested
> by Tom:

>         - nodeFunctionscan.c special-cases set-returning PL/PgSQL
>           functions.

<<itch>> No, that wasn't something I'd ever have suggested.  The
nodeFunctionscan.c code should let the called function *tell* it which
return convention the function wants to use.  We don't want to prevent,
say, dblink.c from using this feature once it's in.

The cleanest way to handle this would be to add an additional field to
the ReturnSetInfo struct, into which the callee could store an enum
value indicating his desired convention.  (We'd initialize the field
to the default mode before call, so as to provide backwards
compatibility for existing SRFs that don't know to do this.)

This will need a little work since the ReturnSetInfo struct is presently
a few layers down in execQual.c, and thus not easily manipulated by
nodeFunctionscan.c.  I'm not sure whether we should modify execQual.c
to provide a more general API, or change things so that
nodeFunctionscan.c executes the function call directly (using execQual.c
only to evaluate the function parameters).  Comments welcome.

>         - memory allocation: the tuple store needs to be accessible
>           after the PL/PgSQL function returns, so it must be allocated
>           in a sufficiently long-lived memory context (e.g. SPI_palloc
>           won't work). I chose to just piggy back on
>           TopTransactionContext, but there's likely a better way to do
>           this...

It would probably be appropriate for ReturnSetInfo to include a field
showing which context to create the returned tuplestore in.  Or it might
do to just have the function return it in the calling context, in which
case SPI_palloc *would* work.  I need to look at the memory management
in nodeFunctionscan.c and see whether it needs revisions...

>         - syntax: the current patch uses 'NEXT', not 'RETURN
>           NEXT'. That's because the latter syntax causes the parsing a
>           regular RETURN statement to go haywire, for some reason I
>           haven't yet determined. Any ideas on how to solve this
>           (and/or use alternative syntax) would be welcome.

The plpgsql parser is pretty grotty, but I imagine we can fix this.
Will look.  In the meantime I agree you shouldn't let this block
progress on other issues.


Now that I've finally dug out from under Rod Taylor's patches, I'm
hoping to start reviewing other stuff that's in my to-look-at queue,
including all the SRF code.  Should be able to give you a hand soon.

            regards, tom lane