Horton

Thread

Commits

  1. Fix mishandling of tSRFs at different nesting levels.

  1. TRAP: FailedAssertion("!(hassrf)", File: "nodeProjectSet.c", Line: 180)

    Erik Rijkers <er@xs4all.nl> — 2017-02-01T23:09:03Z

    Something is broken in HEAD:


drop table if exists t;
create table t(c text);
insert into t (c) values ( 'abc' ) ;

select
           regexp_split_to_array(
           regexp_split_to_table(
                 c
               , chr(13) || chr(10)  )
               , '","' )
                                       as a
      ,
           regexp_split_to_table(
                   c
                 , chr(13) || chr(10)
          )
                                       as rw
from t
;

TRAP: FailedAssertion("!(hassrf)", File: "nodeProjectSet.c", Line: 180)


I realise the regexp* functions aren't doing anything particularly 
useful anymore here; they did in the more complicated original (which I 
had used for years).

thanks,

Erik Rijkers

  2. Re: TRAP: FailedAssertion("!(hassrf)", File: "nodeProjectSet.c", Line: 180)

    Andres Freund <andres@anarazel.de> — 2017-02-01T23:36:53Z

    Hi,

On 2017-02-02 00:09:03 +0100, Erik Rijkers wrote:
> Something is broken in HEAD:

Hm. Indeed.

> drop table if exists t;
> create table t(c text);
> insert into t (c) values ( 'abc' ) ;
> 
> select
>           regexp_split_to_array(
>           regexp_split_to_table(
>                 c
>               , chr(13) || chr(10)  )
>               , '","' )
>                                       as a
>      ,
>           regexp_split_to_table(
>                   c
>                 , chr(13) || chr(10)
>          )
>                                       as rw
> from t
> ;
> 
> TRAP: FailedAssertion("!(hassrf)", File: "nodeProjectSet.c", Line: 180)

> I realise the regexp* functions aren't doing anything particularly useful
> anymore here; they did in the more complicated original (which I had used
> for years).

The issue is that we're generating ProjectSet nodes instead of Result
for the top-level nodes - but we currently assume that ProjectSet nodes
actually contain sets.   I'm tentatively in favor of generating proper
Result nodes in this case, rather than allowing this in ProjectSet - on
the other hand, it works after removing that Assert().

Tom, do you have an opinion?

Greetings,

Andres Freund

  3. Re: TRAP: FailedAssertion("!(hassrf)", File: "nodeProjectSet.c", Line: 180)

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-02-02T03:32:52Z

    Andres Freund <andres@anarazel.de> writes:
> On 2017-02-02 00:09:03 +0100, Erik Rijkers wrote:
>> Something is broken in HEAD:

> Hm. Indeed.

> The issue is that we're generating ProjectSet nodes instead of Result
> for the top-level nodes - but we currently assume that ProjectSet nodes
> actually contain sets.   I'm tentatively in favor of generating proper
> Result nodes in this case, rather than allowing this in ProjectSet - on
> the other hand, it works after removing that Assert().

> Tom, do you have an opinion?

Yes, it's broken.  split_pathtarget_at_srfs seems to be doing the right
thing, but then something later is recombining the last two steps.
I should be able to find it soon.

Does it really work without the Assert?  I'd think you'd get srf-
where-not-supported errors if the SRF isn't at top level.

			regards, tom lane

  4. Re: TRAP: FailedAssertion("!(hassrf)", File: "nodeProjectSet.c", Line: 180)

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-02-02T04:27:36Z

    I wrote:
> Andres Freund <andres@anarazel.de> writes:
>> Tom, do you have an opinion?

> Yes, it's broken.  split_pathtarget_at_srfs seems to be doing the right
> thing, but then something later is recombining the last two steps.

Ah, no, I take that back: split_pathtarget_at_srfs is doing the wrong
thing.  It's generating the desired list of PathTargets, but it's
mistakenly concluding that the last one contains_srfs, which leads to
making a ProjectSet plan node for it instead of Result.  The problem
is specific to targetlists like

	f(g(x)), g(x)

where g() is a SRF and f() can be any sort of non-set-returning
expression.  split_pathtarget_at_srfs() examines the f() node,
sees that it's not a SRF, and calls split_pathtarget_walker which
finds the g(x) subexpression and correctly assigns that to the next
level down.  But then split_pathtarget_at_srfs finds the top-level
g(x) occurrence and concludes that this level contains_srfs.

Nothing's broken quite yet, but when we get to setrefs.c, it replaces
*both* occurrences of g(x) with Vars referencing the g(x) output from
the next level down.  So now we have the tlist of the upper ProjectSet
node as "f(Var), Var" and ProjectSet complains that it has no SRF to
evaluate.

I think the appropriate fix is that, once split_pathtarget_at_srfs() has
computed a tentative list of SRFs it needs to evaluate, it has to make a
second pass to see if any of them match expressions that were assigned to
the next level down.  This is pretty annoying, but we'd only have to do it
if target_contains_srfs and context.nextlevel_contains_srfs are both true,
which will be a negligibly small fraction of queries in practice.

Or we could take out that Assert in nodeProjectSet.c.  But that seems
like a hack not a fix.

I'm pretty tired but I'll work on a fix tomorrow.

			regards, tom lane

  5. Re: TRAP: FailedAssertion("!(hassrf)", File: "nodeProjectSet.c", Line: 180)

    Andres Freund <andres@anarazel.de> — 2017-02-02T04:39:53Z

    On 2017-02-01 23:27:36 -0500, Tom Lane wrote:
> I wrote:
> > Andres Freund <andres@anarazel.de> writes:
> >> Tom, do you have an opinion?
> 
> > Yes, it's broken.  split_pathtarget_at_srfs seems to be doing the right
> > thing, but then something later is recombining the last two steps.
> 
> Ah, no, I take that back: split_pathtarget_at_srfs is doing the wrong
> thing.

Yea, that's what I thought.


> Nothing's broken quite yet, but when we get to setrefs.c, it replaces
> *both* occurrences of g(x) with Vars referencing the g(x) output from
> the next level down.  So now we have the tlist of the upper ProjectSet
> node as "f(Var), Var" and ProjectSet complains that it has no SRF to
> evaluate.

But I'd missed part of that subtlety.


> I think the appropriate fix is that, once split_pathtarget_at_srfs() has
> computed a tentative list of SRFs it needs to evaluate, it has to make a
> second pass to see if any of them match expressions that were assigned to
> the next level down.

> This is pretty annoying, but we'd only have to do it
> if target_contains_srfs and context.nextlevel_contains_srfs are both true,
> which will be a negligibly small fraction of queries in practice.

Hm.  Can't really come up with something better, but I'm kinda tired
too...


> Or we could take out that Assert in nodeProjectSet.c.  But that seems
> like a hack not a fix.

Yea, I don't like that much.


> I'm pretty tired but I'll work on a fix tomorrow.

Cool.

Greetings,

Andres Freund

  6. Re: TRAP: FailedAssertion("!(hassrf)", File: "nodeProjectSet.c", Line: 180)

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-02-02T17:04:29Z

    Andres Freund <andres@anarazel.de> writes:
> On 2017-02-01 23:27:36 -0500, Tom Lane wrote:
>> I think the appropriate fix is that, once split_pathtarget_at_srfs() has
>> computed a tentative list of SRFs it needs to evaluate, it has to make a
>> second pass to see if any of them match expressions that were assigned to
>> the next level down.  This is pretty annoying, but we'd only have to do it
>> if target_contains_srfs and context.nextlevel_contains_srfs are both true,
>> which will be a negligibly small fraction of queries in practice.

> Hm.  Can't really come up with something better, but I'm kinda tired
> too...

I wrote a patch along that line, and was just about ready to commit it
when I realized that really this is all wrong.  Fixing it this way
handles the case of

regression=# select generate_series(1,3), generate_series(1,3) + 1;
 generate_series | ?column? 
-----------------+----------
               1 |        2
               2 |        3
               3 |        4
(3 rows)

which is what you got before v10, because the two SRFs ran in lockstep
despite being at different expression nesting levels.  However, consider

regression=# select generate_series(1,3), generate_series(2,4) + 1;
 generate_series | ?column? 
-----------------+----------
               1 |        3
               2 |        3
               3 |        3
               1 |        4
               2 |        4
               3 |        4
               1 |        5
               2 |        5
               3 |        5
(9 rows)

That's *not* what you got before:

regression=# select generate_series(1,3), generate_series(2,4) + 1;
 generate_series | ?column? 
-----------------+----------
               1 |        3
               2 |        4
               3 |        5
(3 rows)

Really the problem here is that split_pathtarget_at_srfs is completely
wrong about how to assign SRFs to different levels in a stack of
ProjectSet nodes.  It's doing that according to each SRF's top-down
nesting level, but it needs to do it bottom-up, so that a SRF is evaluated
in the k'th step if there are k-1 nested levels of SRFs in its arguments.

This is doable, but I think the routine will have to be completely
rewritten not just hacked around the edges.  Off to do that ...

			regards, tom lane

  7. Re: TRAP: FailedAssertion("!(hassrf)", File: "nodeProjectSet.c", Line: 180)

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-02-02T21:44:22Z

    Erik Rijkers <er@xs4all.nl> writes:
> Something is broken in HEAD:

Fixed, thanks for the report!

			regards, tom lane

  8. Re: TRAP: FailedAssertion("!(hassrf)", File: "nodeProjectSet.c", Line: 180)

    Erik Rijkers <er@xs4all.nl> — 2017-02-02T21:54:58Z

    On 2017-02-02 22:44, Tom Lane wrote:
> Erik Rijkers <er@xs4all.nl> writes:
>> Something is broken in HEAD:
> 
> Fixed, thanks for the report!
> 

Indeed, the complicated version of the script runs again as before.

Thank you very much,

Erik Rijkers