Re: [WIP] UNNEST(REFCURSOR): allowing SELECT to consume data from a REFCURSOR

[Daniel Gustafsson]

> On 29 Jul 2021, at 16:45, Daniel Verite  wrote:

> Trying the v7a patch, here are a few comments:

This thread has stalled with no update or response to the above, and the patch
errors out on make check for the plpgsql suite.  I'm marking this Returned with
Feedback, please resubmit an updated patch if you would like to pursue this
further.

--
Daniel Gustafsson   https://vmware.com/

Re: [WIP] UNNEST(REFCURSOR): allowing SELECT to consume data from a REFCURSOR

[Daniel Verite]

   Hi,

Trying the v7a patch, here are a few comments:

* SIGSEGV with ON HOLD cursors.

Reproducer:

declare c cursor with hold for select oid,relname
  from pg_class order by 1 limit 10;

select * from rows_in('c') as x(f1 oid,f2 name);

consumes a bit of time, then crashes and generates a 13 GB core file
without a usable stacktrace:

Core was generated by `postgres: daniel postgres [local] SELECT  '.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x7f4c5b2f3dc9 in ?? ()
(gdb) bt
#0  0x7f4c5b2f3dc9 in ?? ()
#1  0x564567efc505 in ?? ()
#2  0x0001 in ?? ()
#3  0x56456a4b28f8 in ?? ()
#4  0x56456a4b2908 in ?? ()
#5  0x56456a4b2774 in ?? ()
#6  0x56456a4ad218 in ?? ()
#7  0x56456a4b1590 in ?? ()
#8  0x0010 in ?? ()
#9  0x in ?? ()


* rows_in() does not fetch from the current position of the cursor,
but from the start. For instance, I would expect that if doing
FETCH FROM cursor followed by SELECT * FROM rows_in('cursor'), the first
row would be ignored by rows_in(). That seems more convenient and more
principled.


* 
+  
+   This section describes functions that cursors to be manipulated
+   in normal SELECT queries.
+  

A verb seems to be missing.
It should be "function that *allow* cursors to be..." or something
like that?

* 
+   The REFCURSOR must be open, and the query must be a
+   SELECT statement. If the REFCURSOR’s
+   output does not

After  there is a fancy quote (codepoint U+2019). There is
currently no codepoint outside of US-ASCII in *.sgml ref/*.sgml, so
they're probably not welcome.


* Also: does the community wants it as a built-in function in core?
As mentioned in a previous round of review, a function like this in
plpgsql comes close:

create function rows_in(x refcursor) returns setof record as $$
declare
 r record;
begin
  loop
fetch x into r;
exit when not found;
return next r;
  end loop;
end $$ language plpgsql;



Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: https://www.manitou-mail.org
Twitter: @DanielVerite

Re: [WIP] UNNEST(REFCURSOR): allowing SELECT to consume data from a REFCURSOR

[Dent John]

Hi Massimo,

Happy to help. And actually, end user (i.e., developer) feedback on the 
feature’s usefulness is probably one of the more important contributions.

d.

> On 10 Feb 2021, at 08:57, Massimo Fidanza  wrote:
> 
> Hi John,
> 
> I never build postgresql from source, so I must get some information on how 
> to apply your patch and do some test. I can't review your code because I know 
> nothing about Postgresql internals and just basic C. I am mainly a PL/SQL 
> programmer, with experience with PHP, Python and Javascript. If I can give 
> some contribution I will be happy, but I need some help.
> 
> Massimo
> 
> Il giorno dom 7 feb 2021 alle ore 22:35 Dent John  > ha scritto:
> Hi Massimo,
> 
> Thanks for the interest, and my apologies for the late reply.
> 
> I’m not particularly abandoning it, but I don’t have particular reason to 
> make further changes at the moment. Far as I’m concerned it works, and the 
> main question is whether it is acceptable and useful.
> 
> I’d be happy if you have feedback that evolves it or might push it up the 
> queue for commitfest review.
> 
> d.
> 
>> On 18 Jan 2021, at 23:09, Massimo Fidanza > > wrote:
>> 
>> This is an interesting feature, but it seems that the author has abandoned 
>> development, what happens now? Will this be postponed from commitfest to 
>> commitfest and never be taken over by anyone?
>> 
>> Massimo.
>> 
>> Il giorno ven 6 mar 2020 alle ore 22:36 Dent John > > ha scritto:
>> > On 22 Feb 2020, at 10:38, Dent John mailto:de...@qqdd.eu>> 
>> > wrote:
>> > 
>> >> On 18 Feb 2020, at 03:03, Thomas Munro > >> > wrote:
>> >> 
>> >> From the trivialities department, I see a bunch of warnings about
>> >> local declaration placement (we're still using C90 rules for those by
>> >> project policy):
>> >> 
>> >> […]
>> > 
>> > […]
>> 
>> My bad. I missed on declaration. 
>> 
>> Another patch attached.
>> 
>> d.
> 

Re: [WIP] UNNEST(REFCURSOR): allowing SELECT to consume data from a REFCURSOR

[Massimo Fidanza]

Hi John,

I never build postgresql from source, so I must get some information on how
to apply your patch and do some test. I can't review your code because I
know nothing about Postgresql internals and just basic C. I am mainly a
PL/SQL programmer, with experience with PHP, Python and Javascript. If I
can give some contribution I will be happy, but I need some help.

Massimo

Il giorno dom 7 feb 2021 alle ore 22:35 Dent John  ha
scritto:

> Hi Massimo,
>
> Thanks for the interest, and my apologies for the late reply.
>
> I’m not particularly abandoning it, but I don’t have particular reason to
> make further changes at the moment. Far as I’m concerned it works, and the
> main question is whether it is acceptable and useful.
>
> I’d be happy if you have feedback that evolves it or might push it up the
> queue for commitfest review.
>
> d.
>
> On 18 Jan 2021, at 23:09, Massimo Fidanza  wrote:
>
> This is an interesting feature, but it seems that the author has abandoned
> development, what happens now? Will this be postponed from commitfest to
> commitfest and never be taken over by anyone?
>
> Massimo.
>
> Il giorno ven 6 mar 2020 alle ore 22:36 Dent John  ha
> scritto:
>
>> > On 22 Feb 2020, at 10:38, Dent John  wrote:
>> >
>> >> On 18 Feb 2020, at 03:03, Thomas Munro  wrote:
>> >>
>> >> From the trivialities department, I see a bunch of warnings about
>> >> local declaration placement (we're still using C90 rules for those by
>> >> project policy):
>> >>
>> >> […]
>> >
>> > […]
>>
>> My bad. I missed on declaration.
>>
>> Another patch attached.
>>
>> d.
>>
>
>

Re: [WIP] UNNEST(REFCURSOR): allowing SELECT to consume data from a REFCURSOR

[Dent John]

Hi Massimo,

Thanks for the interest, and my apologies for the late reply.

I’m not particularly abandoning it, but I don’t have particular reason to make 
further changes at the moment. Far as I’m concerned it works, and the main 
question is whether it is acceptable and useful.

I’d be happy if you have feedback that evolves it or might push it up the queue 
for commitfest review.

d.

> On 18 Jan 2021, at 23:09, Massimo Fidanza  wrote:
> 
> This is an interesting feature, but it seems that the author has abandoned 
> development, what happens now? Will this be postponed from commitfest to 
> commitfest and never be taken over by anyone?
> 
> Massimo.
> 
> Il giorno ven 6 mar 2020 alle ore 22:36 Dent John  > ha scritto:
> > On 22 Feb 2020, at 10:38, Dent John  wrote:
> > 
> >> On 18 Feb 2020, at 03:03, Thomas Munro  >> > wrote:
> >> 
> >> From the trivialities department, I see a bunch of warnings about
> >> local declaration placement (we're still using C90 rules for those by
> >> project policy):
> >> 
> >> […]
> > 
> > […]
> 
> My bad. I missed on declaration. 
> 
> Another patch attached.
> 
> d.

Re: [WIP] UNNEST(REFCURSOR): allowing SELECT to consume data from a REFCURSOR

[Massimo Fidanza]

This is an interesting feature, but it seems that the author has abandoned
development, what happens now? Will this be postponed from commitfest to
commitfest and never be taken over by anyone?

Massimo.

Il giorno ven 6 mar 2020 alle ore 22:36 Dent John  ha
scritto:

> > On 22 Feb 2020, at 10:38, Dent John  wrote:
> >
> >> On 18 Feb 2020, at 03:03, Thomas Munro  wrote:
> >>
> >> From the trivialities department, I see a bunch of warnings about
> >> local declaration placement (we're still using C90 rules for those by
> >> project policy):
> >>
> >> […]
> >
> > […]
>
> My bad. I missed on declaration.
>
> Another patch attached.
>
> d.
>

Re: [WIP] UNNEST(REFCURSOR): allowing SELECT to consume data from a REFCURSOR

[Dent John]

> On 22 Feb 2020, at 10:38, Dent John  wrote:
> 
>> On 18 Feb 2020, at 03:03, Thomas Munro  wrote:
>> 
>> From the trivialities department, I see a bunch of warnings about
>> local declaration placement (we're still using C90 rules for those by
>> project policy):
>> 
>> […]
> 
> […]

My bad. I missed on declaration. 

Another patch attached.

d.


unnest-refcursor-v7a.patch
Description: Binary data

Re: [WIP] UNNEST(REFCURSOR): allowing SELECT to consume data from a REFCURSOR

[Dent John]

> On 18 Feb 2020, at 03:03, Thomas Munro  wrote:
> 
> From the trivialities department, I see a bunch of warnings about
> local declaration placement (we're still using C90 rules for those by
> project policy):
> 
> refcursor.c:138:3: error: ISO C90 forbids mixed declarations and code
> [-Werror=declaration-after-statement]
>   MemoryContext oldcontext =
> MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
>   ^

Thanks for pointing that out.

I have updated the patch.

denty.



unnest-refcursor-v6a.patch
Description: Binary data

Re: [WIP] UNNEST(REFCURSOR): allowing SELECT to consume data from a REFCURSOR

[Thomas Munro]

On Sat, Jan 25, 2020 at 11:59 PM Dent John  wrote:
> Attached patch addresses these points, so should now apply cleanly agains dev.

>From the trivialities department, I see a bunch of warnings about
local declaration placement (we're still using C90 rules for those by
project policy):

refcursor.c:138:3: error: ISO C90 forbids mixed declarations and code
[-Werror=declaration-after-statement]
   MemoryContext oldcontext =
MemoryContextSwitchTo(funcctx->multi_call_memory_ctx);
   ^

Re: [WIP] UNNEST(REFCURSOR): allowing SELECT to consume data from a REFCURSOR

[Dent John]

> On 19 Jan 2020, at 22:30, Dent John  wrote:
> 
> I have not yet made steps towards documentation, nor yet rebased, so the 
> Makefile chunk will probably still fail.

Attached patch addresses these points, so should now apply cleanly agains dev.

I also changed the OID assigned to ROWS_IN and its support function.

In passing, I noticed there is one existing function that can consume and make 
good use of ROWS_IN’s result when used in the target list, which is 
row_to_json. This is good, as it makes ROWS_IN useful even outside of a change 
to allow results in the FROM to be pipelined. I’ve called out row_to_json 
specifically in the documentation change.

denty.



unnest-refcursor-v5a.patch
Description: Binary data

Re: [WIP] UNNEST(REFCURSOR): allowing SELECT to consume data from a REFCURSOR

[Dent John]

> On 11 Jan 2020, at 12:04, Dent John  wrote:
> 
>> On 10 Jan 2020, at 15:45, Daniel Verite  wrote:
>> 
>> postgres=# select unnest('c'::refcursor);
>> server closed the connection unexpectedly
>>  This probably means the server terminated abnormally
>>  before or while processing the request.
>> The connection to the server was lost. Attempting reset: Failed.
> 
> Okay. That’s pretty bad, isn’t it.

I’ve addressed the issue, which was due to me allocating the TupleDesc in the 
multi_call_memory_ctx, which seemed quite reasonable, but it actually needs to 
be in ecxt_per_query_memory. It seems tSRF-mode queries are much more sensitive 
to the misstep.

A v4 patch is attached, which also renames UNNEST(REFCURSOR) to 
ROWS_IN(REFCURSOR), adds a test case for use in tSRF mode, and makes some minor 
fixes to the support function.

I have not yet made steps towards documentation, nor yet rebased, so the 
Makefile chunk will probably still fail.

denty.


unnest-refcursor-v4.patch
Description: Binary data

Re: [WIP] UNNEST(REFCURSOR): allowing SELECT to consume data from a REFCURSOR

[Dent John]

> On 14 Jan 2020, at 14:53, Daniel Verite  wrote:
> 
> What is the expected result anyway? A single column with a "record"
> type? FWIW I notice that with plpgsql, this is not allowed to happen:

Hmm. How interesting.

I had not really investigated what happens in the case of a function returning 
SETOF (untyped) RECORD in a SELECT clause because, whatever the result, there’s 
no mechanism to access the individual fields.

As you highlight, it doesn’t work at all in plpgsql, and plperl is the same.

However, SQL language functions get away with it. For example, inspired by 
_pg_expandarray():

CREATE OR REPLACE FUNCTION public.my_pg_expandarray(anyarray)
RETURNS SETOF record
LANGUAGE sql
IMMUTABLE PARALLEL SAFE STRICT
AS $function$
select $1[s], s - pg_catalog.array_lower($1,1) + 1
from pg_catalog.generate_series(pg_catalog.array_lower($1,1),
pg_catalog.array_upper($1,1), 1) as g(s)
$function$

postgres=# select my_pg_expandarray (array[0, 1, 2, 3, 4]);
 my_pg_expandarray 
---
 (0,1)
 (1,2)
 (2,3)
 (3,4)
 (4,5)
(5 rows)

Back in the FROM clause, it’s possible to manipulate the individual fields:

postgres=# select b, a from my_pg_expandarray (array[0, 1, 2, 3, 4]) as r(a 
int, b int);
 b | a 
---+---
 1 | 0
 2 | 1
 3 | 2
 4 | 3
 5 | 4
(5 rows)

It’s quite interesting. All the other PLs make explicit checks for 
rsinfo.expectedDesc being non-NULL, but fmgr_sql() explicitly calls out the 
contrary: “[…] note we do not require caller to provide an expectedDesc.” So I 
guess either there’s something special about the SQL PL, or perhaps the other 
PLs are just inheriting a pattern of being cautious.

Either way, though, there’s no way that I can see to "get at” the fields inside 
the anonymous record that is returned when the function is in the SELECT list.

But back to the failure, I still need to make it not crash. I guess it doesn’t 
matter whether I simply refuse to work if called from the SELECT list, or just 
return an anonymous record, like fmgr_sql() does.

d.

Re: [WIP] UNNEST(REFCURSOR): allowing SELECT to consume data from a REFCURSOR

[Daniel Verite]

Dent John wrote:

> It’s crashing when it’s checking that the returned tuple matches the
> declared return type in rsinfo->setDesc. Seems rsinfo->setDesc gets
> overwritten. So I think I have a memory management problem.

What is the expected result anyway? A single column with a "record"
type? FWIW I notice that with plpgsql, this is not allowed to happen:

CREATE FUNCTION cursor_unnest(x refcursor) returns setof record
as $$
declare
 r record;
begin
  loop
fetch x into r;
exit when not found;
return next r;
  end loop;
end $$ language plpgsql;

begin;

declare c cursor for select oid::int as i, relname::text as r from pg_class;

select cursor_unnest('c');

ERROR:  set-valued function called in context that cannot accept a set
CONTEXT:  PL/pgSQL function cursor_unnest(refcursor) line 8 at RETURN NEXT


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

Re: [WIP] UNNEST(REFCURSOR): allowing SELECT to consume data from a REFCURSOR

[Dent John]

> On 10 Jan 2020, at 15:45, Daniel Verite  wrote:
> 
> At a quick glance, I don't see it called in the select-list  in any
> of the regression tests. […]

Yep. I didn’t test it because I figured it wasn’t particularly useful in that 
context. I’ll add some tests for that too once I get to the root of the problem.

> postgres=# select unnest('c'::refcursor);
> server closed the connection unexpectedly
>   This probably means the server terminated abnormally
>   before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.

Okay. That’s pretty bad, isn’t it.

It’s crashing when it’s checking that the returned tuple matches the declared 
return type in rsinfo->setDesc. Seems rsinfo->setDesc gets overwritten. So I 
think I have a memory management problem.

To be honest, I wasn’t fully sure I’d got a clear understanding of what is in 
what memory context, but things seemed to work so I figured it was close. Seems 
I was wrong. I need a bit of time to review. Leave it with me, but I guess 
it’ll take to next weekend before I get more time.

denty.

Re: [WIP] UNNEST(REFCURSOR): allowing SELECT to consume data from a REFCURSOR

[Daniel Verite]

Dent John wrote:

> Yes. That’s at least true if unnest(x) is used in the FROM. If it’s used in
> the SELECT, actually it can get the performance benefit right away

At a quick glance, I don't see it called in the select-list  in any
of the regression tests. When trying it, it appears to crash (segfault):

postgres=# begin;
BEGIN

postgres=# declare c cursor for select oid::int as i, relname::text as r from
pg_class;
DECLARE CURSOR

postgres=# select unnest('c'::refcursor);
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

The build is configured with:
./configure --enable-debug --with-icu --with-perl --enable-depend


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

Re: [WIP] UNNEST(REFCURSOR): allowing SELECT to consume data from a REFCURSOR

[Dent John]

> On 9 Jan 2020, at 17:43, Daniel Verite  wrote:
> 
> […]
> (using plain text instead of HTML messages might help with that).

Thanks. I’ll do that next time.

> […]
> * unnest-refcursor-v3.patch needs a slight rebase because this chunk
> in the Makefile fails
> - regexp.o regproc.o ri_triggers.o rowtypes.o ruleutils.o \
> + refcursor.o regexp.o regproc.o ri_triggers.o rowtypes.o ruleutils.o \

Likewise I’ll make that rebase in the next version.

> * I'm under the impression that refcursor_unnest() is functionally
> equivalent to a plpgsql implementation like this:
> 
> […]
> 
> but it would differ in performance, because internally a materialization step
> could be avoided, but only when the other patch "Allow FunctionScans to
> pipeline results"  gets in?

Yes. That’s at least true if unnest(x) is used in the FROM. If it’s used in the 
SELECT, actually it can get the performance benefit right away. However, in the 
SELECT case, there’s a bit of a gotcha because anonymous records can’t easily 
be manipulated because they have no type information available. So to make a 
useful performance contribution, it does need to be combined with another 
change — either to make it FROM pipeline as in my other patch, or perhaps 
enabling anonymous record types to be cast or otherwise manipulated.

> […]
> /*
> - * UNNEST
> + * UNNEST (array)
>  */
> 
> This chunk looks unnecessary?

It was for purpose of disambiguating. But indeed it is unnecessary. Choosing a 
different name would avoid need for it.

> * some user-facing doc would be needed.

Indeed. I fully intend that. I figured I’d get the concept on a firmer footing 
first.

> * Is it good to overload "unnest" rather than choosing a specific
> function name?

Yeah. I wondered about that. A couple of syntactically obvious ideas were:

SELECT … FROM TABLE (x) (which is what I think Oracle does, but is reserved)

SELECT … FROM CURSOR (x) (which seems likely to confuse, but, surprisingly, 
isn’t actually reserved)

SELECT … FROM FETCH (x) (which I quite like, but is reserved)

SELECT … FROM ROWS_FROM (x) (is okay, but conflicts with our ROWS FROM 
construct)

So I kind of landed on UNNEST(x) out of lack of better idea. EXPAND(x) could be 
an option. Actually ROWS_IN(x) or ROWS_OF(x) might work.

Do you have any preference or suggestion?

Thanks a lot for the feedback.

denty.

Re: [WIP] UNNEST(REFCURSOR): allowing SELECT to consume data from a REFCURSOR

[Daniel Verite]

Dent John wrote:

> I’ve made a revision of this patch. 

Some comments:

* the commitfest app did not extract up the patch from the mail,
possibly because it's buried in the MIME structure of the mail
(using plain text instead of HTML messages might help with that).
The patch has no status in http://commitfest.cputube.org/
probably because of this too.


* unnest-refcursor-v3.patch needs a slight rebase because this chunk
in the Makefile fails
-   regexp.o regproc.o ri_triggers.o rowtypes.o ruleutils.o \
+   refcursor.o regexp.o regproc.o ri_triggers.o rowtypes.o ruleutils.o \


* I'm under the impression that refcursor_unnest() is functionally
equivalent to a plpgsql implementation like this:

create function unnest(x refcursor) returns setof record as $$
declare
 r record;
begin
  loop
fetch x into r;
exit when not found;
return next r;
  end loop;
end $$ language plpgsql;

but it would differ in performance, because internally a materialization step
could be avoided, but only when the other patch "Allow FunctionScans to
pipeline results"  gets in?
Or are performance benefits expected right away with this patch?

*
--- a/src/backend/utils/adt/arrayfuncs.c
+++ b/src/backend/utils/adt/arrayfuncs.c
@@ -5941,7 +5941,7 @@ array_fill_internal(ArrayType *dims, ArrayType *lbs,
 
 
 /*
- * UNNEST
+ * UNNEST (array)
  */

This chunk looks unnecessary?

* some user-facing doc would be needed.

* Is it good to overload "unnest" rather than choosing a specific
function name?


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

Re: [WIP] UNNEST(REFCURSOR): allowing SELECT to consume data from a REFCURSOR

[Dent John]

Hi folks,I’ve made a revision of this patch. The significant change is to access the Portal using Portal APIs rather than through SPI. It seems the persisted state necessary to survive being used to retrieve a row at a time inside an SRF just isn’t a good fit for SPI. It turned out there was upstream machinery in the FunctionScan node that prevented Postgres being able to pipeline SRFs, even if they return ValuePerCall. So, in practice, this patch is of limited benefit without another patch that changes that behaviour (see [1]). Nevertheless, the code is independent so I’m submitting the two changes separately. I’ll push this into the Jan commit fest.denty. [1] https://commitfest.postgresql.org/26/2372/

unnest-refcursor-v3.patch
Description: Binary data

Re: [WIP] UNNEST(REFCURSOR): allowing SELECT to consume data from a REFCURSOR

[Roman Pekar]

Hi John,

Thanks for pushing this, for me it looks like promising start! I need a bit
more time to go through the code (and I'm not an expert in Postgres
internals in any way) but I really appreciate you doing this.

Roman