enable_incremental_sort changes query behavior

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
56 messages Options
123
Reply | Threaded
Open this post in threaded view
|

enable_incremental_sort changes query behavior

Jaime Casanova-4
Hi,

With sqlsmith I found a query that gives this error:
ERROR:  ORDER/GROUP BY expression not found in targetlist

I noted the query (sql query below, sorry it uses custom tables i
couldn't replicate with regression tables) because it doesn't include
an ORDER/GROUP BY clause.

--- 0 ----
select distinct
          subq_0.c1 as c0,
          ref_0.radi_usua_radi as c1,
          ref_0.radi_nume_asoc as c2,
          subq_0.c1 as c3,
          case when (cast(null as pg_lsn) >=
pg_catalog.pg_last_wal_receive_lsn())
              and (true = pg_catalog.pg_rotate_logfile_old()) then
ref_0.radi_usua_rem else ref_0.radi_usua_rem end
             as c4,
          cast(nullif((select hist_codi from public.hist_eventos_2
limit 1 offset 4)
              ,
            pg_catalog.pg_stat_get_buf_alloc()) as int8) as c5
        from
          public.radicado_2 as ref_0,
          lateral (select
                ref_0.radi_text_temp as c0,
                ref_0.radi_usua_actu as c1
              from
                public.hist_eventos_1 as ref_1
              where cast(nullif(cast(null as float4),
                  cast(null as float4)) as float4) >= pg_catalog.pi()) as subq_0
        where ref_0.radi_usua_dest is not NULL;
--- 0 ----

Attached the stack trace produced until de elog that produces the message.

But if I set enable_incremental_sort to off the query gets executed
without problems (attached the explain produced for that case)

--
Jaime Casanova                      www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

query_plan.txt.txt (2K) Download Attachment
gdb_stack_trace.txt (17K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: enable_incremental_sort changes query behavior

James Coleman
On Sat, Sep 26, 2020 at 2:49 PM Jaime Casanova
<[hidden email]> wrote:

>
> Hi,
>
> With sqlsmith I found a query that gives this error:
> ERROR:  ORDER/GROUP BY expression not found in targetlist
>
> I noted the query (sql query below, sorry it uses custom tables i
> couldn't replicate with regression tables) because it doesn't include
> an ORDER/GROUP BY clause.
>
> --- 0 ----
> select distinct
>           subq_0.c1 as c0,
>           ref_0.radi_usua_radi as c1,
>           ref_0.radi_nume_asoc as c2,
>           subq_0.c1 as c3,
>           case when (cast(null as pg_lsn) >=
> pg_catalog.pg_last_wal_receive_lsn())
>               and (true = pg_catalog.pg_rotate_logfile_old()) then
> ref_0.radi_usua_rem else ref_0.radi_usua_rem end
>              as c4,
>           cast(nullif((select hist_codi from public.hist_eventos_2
> limit 1 offset 4)
>               ,
>             pg_catalog.pg_stat_get_buf_alloc()) as int8) as c5
>         from
>           public.radicado_2 as ref_0,
>           lateral (select
>                 ref_0.radi_text_temp as c0,
>                 ref_0.radi_usua_actu as c1
>               from
>                 public.hist_eventos_1 as ref_1
>               where cast(nullif(cast(null as float4),
>                   cast(null as float4)) as float4) >= pg_catalog.pi()) as subq_0
>         where ref_0.radi_usua_dest is not NULL;
> --- 0 ----
>
> Attached the stack trace produced until de elog that produces the message.
>
> But if I set enable_incremental_sort to off the query gets executed
> without problems (attached the explain produced for that case)

Thanks for the report.

Is there by an chance an index on ref_0.radi_text_temp?

And if you set enable_hashagg = off what plan do you get (or error)?

Thanks,
James


Reply | Threaded
Open this post in threaded view
|

Re: enable_incremental_sort changes query behavior

Jaime Casanova-4
On Wed, 30 Sep 2020 at 21:21, James Coleman <[hidden email]> wrote:
>
> On Sat, Sep 26, 2020 at 2:49 PM Jaime Casanova
> <[hidden email]> wrote:
> >
> > Hi,
> >
> > With sqlsmith I found a query that gives this error:
> > ERROR:  ORDER/GROUP BY expression not found in targetlist
> >
[...]
> >
> > But if I set enable_incremental_sort to off the query gets executed
> > without problems (attached the explain produced for that case)
>
> Thanks for the report.
>

Hi,

by experiment I reduced the query to this

--- 0 ---
select distinct
        subq_0.c1 as c0,
        case when (true = pg_catalog.pg_rotate_logfile_old()) then
                ref_0.t else ref_0.t
        end
             as c4
        from
          public.ref_0,
          lateral (select

                ref_0.i as c1
              from
                generate_series(1, 100) as ref_1) as subq_0
--- 0 ---

the only custom table already needed can be created with this commands:

--- 0 ---
create table ref_0 as select repeat('abcde', (random() * 10)::integer)
t, random() * 1000 i from generate_series(1, 500000);
create index on ref_0 (i);
analyze ref_0 ;
--- 0 ---


> Is there by an chance an index on ref_0.radi_text_temp?
>

there is an index involved but not on that field, commands above
create the index in the right column... after that, ANALYZE the table

> And if you set enable_hashagg = off what plan do you get (or error)?
>

same error

--
Jaime Casanova                      www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: enable_incremental_sort changes query behavior

James Coleman
On Thu, Oct 1, 2020 at 3:09 AM Jaime Casanova
<[hidden email]> wrote:

>
> On Wed, 30 Sep 2020 at 21:21, James Coleman <[hidden email]> wrote:
> >
> > On Sat, Sep 26, 2020 at 2:49 PM Jaime Casanova
> > <[hidden email]> wrote:
> > >
> > > Hi,
> > >
> > > With sqlsmith I found a query that gives this error:
> > > ERROR:  ORDER/GROUP BY expression not found in targetlist
> > >
> [...]
> > >
> > > But if I set enable_incremental_sort to off the query gets executed
> > > without problems (attached the explain produced for that case)
> >
> > Thanks for the report.
> >
>
> Hi,
>
> by experiment I reduced the query to this
>
> --- 0 ---
> select distinct
>         subq_0.c1 as c0,
>         case when (true = pg_catalog.pg_rotate_logfile_old()) then
>                 ref_0.t else ref_0.t
>         end
>              as c4
>         from
>           public.ref_0,
>           lateral (select
>
>                 ref_0.i as c1
>               from
>                 generate_series(1, 100) as ref_1) as subq_0
> --- 0 ---
>
> the only custom table already needed can be created with this commands:
>
> --- 0 ---
> create table ref_0 as select repeat('abcde', (random() * 10)::integer)
> t, random() * 1000 i from generate_series(1, 500000);
> create index on ref_0 (i);
> analyze ref_0 ;
> --- 0 ---
>
>
> > Is there by an chance an index on ref_0.radi_text_temp?
> >
>
> there is an index involved but not on that field, commands above
> create the index in the right column... after that, ANALYZE the table
>
> > And if you set enable_hashagg = off what plan do you get (or error)?
> >
>
> same error

I was able to reproduce the error without incremental sort enabled
(i.e., it happens with a full sort also). The function call in the
SELECT doesn't have to be in a case expression; for example I was able
to reproduce changing that to `random()::text || ref_0.t`.

It looks like the issue happens when:
1. The sort happens within a parallel node.
2. One of the sort keys is an expression containing a volatile
function call and a column from the lateral join.

Here are the settings I used with your above repro case to show it
with regular sort:

 enable_hashagg=off
 enable_incremental_sort=off
 enable_seqscan=off
 parallel_setup_cost=10
 parallel_tuple_cost=0

The plan (obtained by replacing the volatile function with a stable one):

 Unique
   ->  Nested Loop
         ->  Gather Merge
               Workers Planned: 2
               ->  Sort
                     Sort Key: ref_0.i, (md5(ref_0.t))
                     ->  Parallel Index Scan using ref_0_i_idx on ref_0
         ->  Function Scan on generate_series ref_1

Changing `md5(ref_0.t)` to `random()::text || ref_0.t` causes the error.

I haven't been able to dig further than that yet, but my intuition is
to poke around in the parallel query machinery?

James


Reply | Threaded
Open this post in threaded view
|

Re: enable_incremental_sort changes query behavior

Tomas Vondra-4
On Thu, Oct 01, 2020 at 09:02:57AM -0400, James Coleman wrote:

>On Thu, Oct 1, 2020 at 3:09 AM Jaime Casanova
><[hidden email]> wrote:
>>
>> On Wed, 30 Sep 2020 at 21:21, James Coleman <[hidden email]> wrote:
>> >
>> > On Sat, Sep 26, 2020 at 2:49 PM Jaime Casanova
>> > <[hidden email]> wrote:
>> > >
>> > > Hi,
>> > >
>> > > With sqlsmith I found a query that gives this error:
>> > > ERROR:  ORDER/GROUP BY expression not found in targetlist
>> > >
>> [...]
>> > >
>> > > But if I set enable_incremental_sort to off the query gets executed
>> > > without problems (attached the explain produced for that case)
>> >
>> > Thanks for the report.
>> >
>>
>> Hi,
>>
>> by experiment I reduced the query to this
>>
>> --- 0 ---
>> select distinct
>>         subq_0.c1 as c0,
>>         case when (true = pg_catalog.pg_rotate_logfile_old()) then
>>                 ref_0.t else ref_0.t
>>         end
>>              as c4
>>         from
>>           public.ref_0,
>>           lateral (select
>>
>>                 ref_0.i as c1
>>               from
>>                 generate_series(1, 100) as ref_1) as subq_0
>> --- 0 ---
>>
>> the only custom table already needed can be created with this commands:
>>
>> --- 0 ---
>> create table ref_0 as select repeat('abcde', (random() * 10)::integer)
>> t, random() * 1000 i from generate_series(1, 500000);
>> create index on ref_0 (i);
>> analyze ref_0 ;
>> --- 0 ---
>>
>>
>> > Is there by an chance an index on ref_0.radi_text_temp?
>> >
>>
>> there is an index involved but not on that field, commands above
>> create the index in the right column... after that, ANALYZE the table
>>
>> > And if you set enable_hashagg = off what plan do you get (or error)?
>> >
>>
>> same error
>
>I was able to reproduce the error without incremental sort enabled
>(i.e., it happens with a full sort also). The function call in the
>SELECT doesn't have to be in a case expression; for example I was able
>to reproduce changing that to `random()::text || ref_0.t`.
>
>It looks like the issue happens when:
>1. The sort happens within a parallel node.
>2. One of the sort keys is an expression containing a volatile
>function call and a column from the lateral join.
>
>Here are the settings I used with your above repro case to show it
>with regular sort:
>
> enable_hashagg=off
> enable_incremental_sort=off
> enable_seqscan=off
> parallel_setup_cost=10
> parallel_tuple_cost=0
>
>The plan (obtained by replacing the volatile function with a stable one):
>
> Unique
>   ->  Nested Loop
>         ->  Gather Merge
>               Workers Planned: 2
>               ->  Sort
>                     Sort Key: ref_0.i, (md5(ref_0.t))
>                     ->  Parallel Index Scan using ref_0_i_idx on ref_0
>         ->  Function Scan on generate_series ref_1
>
>Changing `md5(ref_0.t)` to `random()::text || ref_0.t` causes the error.
>
>I haven't been able to dig further than that yet, but my intuition is
>to poke around in the parallel query machinery?
>

Nope. Bisect says this was introduced by this commit:

     ba3e76cc57 Consider Incremental Sort paths at additional places

Looking at the diff, I suspect generate_useful_gather_paths (or maybe
get_useful_pathkeys_for_relation) fails to do something with the
pathkeys.

Of course, that'd explain why it only happens for parallel plans, as
this builds gather nodes.


regards

--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: enable_incremental_sort changes query behavior

James Coleman
On Thu, Oct 1, 2020 at 6:08 PM Tomas Vondra
<[hidden email]> wrote:

>
> On Thu, Oct 01, 2020 at 09:02:57AM -0400, James Coleman wrote:
> >On Thu, Oct 1, 2020 at 3:09 AM Jaime Casanova
> ><[hidden email]> wrote:
> >>
> >> On Wed, 30 Sep 2020 at 21:21, James Coleman <[hidden email]> wrote:
> >> >
> >> > On Sat, Sep 26, 2020 at 2:49 PM Jaime Casanova
> >> > <[hidden email]> wrote:
> >> > >
> >> > > Hi,
> >> > >
> >> > > With sqlsmith I found a query that gives this error:
> >> > > ERROR:  ORDER/GROUP BY expression not found in targetlist
> >> > >
> >> [...]
> >> > >
> >> > > But if I set enable_incremental_sort to off the query gets executed
> >> > > without problems (attached the explain produced for that case)
> >> >
> >> > Thanks for the report.
> >> >
> >>
> >> Hi,
> >>
> >> by experiment I reduced the query to this
> >>
> >> --- 0 ---
> >> select distinct
> >>         subq_0.c1 as c0,
> >>         case when (true = pg_catalog.pg_rotate_logfile_old()) then
> >>                 ref_0.t else ref_0.t
> >>         end
> >>              as c4
> >>         from
> >>           public.ref_0,
> >>           lateral (select
> >>
> >>                 ref_0.i as c1
> >>               from
> >>                 generate_series(1, 100) as ref_1) as subq_0
> >> --- 0 ---
> >>
> >> the only custom table already needed can be created with this commands:
> >>
> >> --- 0 ---
> >> create table ref_0 as select repeat('abcde', (random() * 10)::integer)
> >> t, random() * 1000 i from generate_series(1, 500000);
> >> create index on ref_0 (i);
> >> analyze ref_0 ;
> >> --- 0 ---
> >>
> >>
> >> > Is there by an chance an index on ref_0.radi_text_temp?
> >> >
> >>
> >> there is an index involved but not on that field, commands above
> >> create the index in the right column... after that, ANALYZE the table
> >>
> >> > And if you set enable_hashagg = off what plan do you get (or error)?
> >> >
> >>
> >> same error
> >
> >I was able to reproduce the error without incremental sort enabled
> >(i.e., it happens with a full sort also). The function call in the
> >SELECT doesn't have to be in a case expression; for example I was able
> >to reproduce changing that to `random()::text || ref_0.t`.
> >
> >It looks like the issue happens when:
> >1. The sort happens within a parallel node.
> >2. One of the sort keys is an expression containing a volatile
> >function call and a column from the lateral join.
> >
> >Here are the settings I used with your above repro case to show it
> >with regular sort:
> >
> > enable_hashagg=off
> > enable_incremental_sort=off
> > enable_seqscan=off
> > parallel_setup_cost=10
> > parallel_tuple_cost=0
> >
> >The plan (obtained by replacing the volatile function with a stable one):
> >
> > Unique
> >   ->  Nested Loop
> >         ->  Gather Merge
> >               Workers Planned: 2
> >               ->  Sort
> >                     Sort Key: ref_0.i, (md5(ref_0.t))
> >                     ->  Parallel Index Scan using ref_0_i_idx on ref_0
> >         ->  Function Scan on generate_series ref_1
> >
> >Changing `md5(ref_0.t)` to `random()::text || ref_0.t` causes the error.
> >
> >I haven't been able to dig further than that yet, but my intuition is
> >to poke around in the parallel query machinery?
> >
>
> Nope. Bisect says this was introduced by this commit:
>
>      ba3e76cc57 Consider Incremental Sort paths at additional places
>
> Looking at the diff, I suspect generate_useful_gather_paths (or maybe
> get_useful_pathkeys_for_relation) fails to do something with the
> pathkeys.
>
> Of course, that'd explain why it only happens for parallel plans, as
> this builds gather nodes.

I should have known I'd regret making a wild guess. I was in the
middle of bisecting too, but had to set it aside for a bit and hadn't
finished.

I'll try to look into that commit (and I agree those functions are the
likely places to look) as I get a chance here.

James


Reply | Threaded
Open this post in threaded view
|

Re: enable_incremental_sort changes query behavior

James Coleman
On Thu, Oct 1, 2020 at 9:10 PM James Coleman <[hidden email]> wrote:

>
> On Thu, Oct 1, 2020 at 6:08 PM Tomas Vondra
> <[hidden email]> wrote:
> >
> > On Thu, Oct 01, 2020 at 09:02:57AM -0400, James Coleman wrote:
> > >On Thu, Oct 1, 2020 at 3:09 AM Jaime Casanova
> > ><[hidden email]> wrote:
> > >>
> > >> On Wed, 30 Sep 2020 at 21:21, James Coleman <[hidden email]> wrote:
> > >> >
> > >> > On Sat, Sep 26, 2020 at 2:49 PM Jaime Casanova
> > >> > <[hidden email]> wrote:
> > >> > >
> > >> > > Hi,
> > >> > >
> > >> > > With sqlsmith I found a query that gives this error:
> > >> > > ERROR:  ORDER/GROUP BY expression not found in targetlist
> > >> > >
> > >> [...]
> > >> > >
> > >> > > But if I set enable_incremental_sort to off the query gets executed
> > >> > > without problems (attached the explain produced for that case)
> > >> >
> > >> > Thanks for the report.
> > >> >
> > >>
> > >> Hi,
> > >>
> > >> by experiment I reduced the query to this
> > >>
> > >> --- 0 ---
> > >> select distinct
> > >>         subq_0.c1 as c0,
> > >>         case when (true = pg_catalog.pg_rotate_logfile_old()) then
> > >>                 ref_0.t else ref_0.t
> > >>         end
> > >>              as c4
> > >>         from
> > >>           public.ref_0,
> > >>           lateral (select
> > >>
> > >>                 ref_0.i as c1
> > >>               from
> > >>                 generate_series(1, 100) as ref_1) as subq_0
> > >> --- 0 ---
> > >>
> > >> the only custom table already needed can be created with this commands:
> > >>
> > >> --- 0 ---
> > >> create table ref_0 as select repeat('abcde', (random() * 10)::integer)
> > >> t, random() * 1000 i from generate_series(1, 500000);
> > >> create index on ref_0 (i);
> > >> analyze ref_0 ;
> > >> --- 0 ---
> > >>
> > >>
> > >> > Is there by an chance an index on ref_0.radi_text_temp?
> > >> >
> > >>
> > >> there is an index involved but not on that field, commands above
> > >> create the index in the right column... after that, ANALYZE the table
> > >>
> > >> > And if you set enable_hashagg = off what plan do you get (or error)?
> > >> >
> > >>
> > >> same error
> > >
> > >I was able to reproduce the error without incremental sort enabled
> > >(i.e., it happens with a full sort also). The function call in the
> > >SELECT doesn't have to be in a case expression; for example I was able
> > >to reproduce changing that to `random()::text || ref_0.t`.
> > >
> > >It looks like the issue happens when:
> > >1. The sort happens within a parallel node.
> > >2. One of the sort keys is an expression containing a volatile
> > >function call and a column from the lateral join.
> > >
> > >Here are the settings I used with your above repro case to show it
> > >with regular sort:
> > >
> > > enable_hashagg=off
> > > enable_incremental_sort=off
> > > enable_seqscan=off
> > > parallel_setup_cost=10
> > > parallel_tuple_cost=0
> > >
> > >The plan (obtained by replacing the volatile function with a stable one):
> > >
> > > Unique
> > >   ->  Nested Loop
> > >         ->  Gather Merge
> > >               Workers Planned: 2
> > >               ->  Sort
> > >                     Sort Key: ref_0.i, (md5(ref_0.t))
> > >                     ->  Parallel Index Scan using ref_0_i_idx on ref_0
> > >         ->  Function Scan on generate_series ref_1
> > >
> > >Changing `md5(ref_0.t)` to `random()::text || ref_0.t` causes the error.
> > >
> > >I haven't been able to dig further than that yet, but my intuition is
> > >to poke around in the parallel query machinery?
> > >
> >
> > Nope. Bisect says this was introduced by this commit:
> >
> >      ba3e76cc57 Consider Incremental Sort paths at additional places
> >
> > Looking at the diff, I suspect generate_useful_gather_paths (or maybe
> > get_useful_pathkeys_for_relation) fails to do something with the
> > pathkeys.
> >
> > Of course, that'd explain why it only happens for parallel plans, as
> > this builds gather nodes.
>
> I should have known I'd regret making a wild guess. I was in the
> middle of bisecting too, but had to set it aside for a bit and hadn't
> finished.
>
> I'll try to look into that commit (and I agree those functions are the
> likely places to look) as I get a chance here.

I've been able to confirm that the problem goes away if we stop adding
the gather merge paths in generate_useful_gather_paths().

I'm not sure yet what conclusion that leads us to. It seems to be that
the biggest clue remains that all of this works correctly unless one
of the selected columns (which happens to be a pathkey at this point
because it's a DISTINCT query) contains a volatile expression.

James


Reply | Threaded
Open this post in threaded view
|

Re: enable_incremental_sort changes query behavior

Tomas Vondra-4
On Fri, Oct 02, 2020 at 09:19:44AM -0400, James Coleman wrote:

>
> ...
>
>I've been able to confirm that the problem goes away if we stop adding
>the gather merge paths in generate_useful_gather_paths().
>
>I'm not sure yet what conclusion that leads us to. It seems to be that
>the biggest clue remains that all of this works correctly unless one
>of the selected columns (which happens to be a pathkey at this point
>because it's a DISTINCT query) contains a volatile expression.
>
Yeah. It seems to me this is a bug in get_useful_pathkeys_for_relation,
which is calling find_em_expr_for_rel and is happy with anything it
returns. But this was copied from postgres_fdw, which however does a bit
more here:

     if (pathkey_ec->ec_has_volatile ||
         !(em_expr = find_em_expr_for_rel(pathkey_ec, rel)) ||
         !is_foreign_expr(root, rel, em_expr))

So not only does it explicitly check volatility of the pathkey, it also
calls is_foreign_expr which checks the expression for mutable functions.

The attached patch seems to fix this, but it only adds the check for
mutable functions. Maybe it should check ec_has_volatile too ...


regards

--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

incremental-sort-fix.patch (947 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: enable_incremental_sort changes query behavior

James Coleman
On Fri, Oct 2, 2020 at 10:32 AM Tomas Vondra
<[hidden email]> wrote:

>
> On Fri, Oct 02, 2020 at 09:19:44AM -0400, James Coleman wrote:
> >
> > ...
> >
> >I've been able to confirm that the problem goes away if we stop adding
> >the gather merge paths in generate_useful_gather_paths().
> >
> >I'm not sure yet what conclusion that leads us to. It seems to be that
> >the biggest clue remains that all of this works correctly unless one
> >of the selected columns (which happens to be a pathkey at this point
> >because it's a DISTINCT query) contains a volatile expression.
> >
>
> Yeah. It seems to me this is a bug in get_useful_pathkeys_for_relation,
> which is calling find_em_expr_for_rel and is happy with anything it
> returns. But this was copied from postgres_fdw, which however does a bit
> more here:
>
>      if (pathkey_ec->ec_has_volatile ||
>          !(em_expr = find_em_expr_for_rel(pathkey_ec, rel)) ||
>          !is_foreign_expr(root, rel, em_expr))
>
> So not only does it explicitly check volatility of the pathkey, it also
> calls is_foreign_expr which checks the expression for mutable functions.
>
> The attached patch seems to fix this, but it only adds the check for
> mutable functions. Maybe it should check ec_has_volatile too ...

We actually discussed the volatility check in that function back on
the original thread [1], and we'd concluded that was specifically
necessary for the fdw code because the function would execute on two
different servers (and thus produce different results), but that in a
local server only scenario it should be fine.

My understanding (correct me if I'm wrong) is that the volatile
function should only be executed once (at the scan level?) to build
the tuple and from then on the datum isn't going to change, so I'm not
sure why the volatility would matter here.

James

1: https://www.postgresql.org/message-id/20200328025830.6v6ogkseohakc32q%40development


Reply | Threaded
Open this post in threaded view
|

Re: enable_incremental_sort changes query behavior

James Coleman
On Fri, Oct 2, 2020 at 10:53 AM James Coleman <[hidden email]> wrote:

>
> On Fri, Oct 2, 2020 at 10:32 AM Tomas Vondra
> <[hidden email]> wrote:
> >
> > On Fri, Oct 02, 2020 at 09:19:44AM -0400, James Coleman wrote:
> > >
> > > ...
> > >
> > >I've been able to confirm that the problem goes away if we stop adding
> > >the gather merge paths in generate_useful_gather_paths().
> > >
> > >I'm not sure yet what conclusion that leads us to. It seems to be that
> > >the biggest clue remains that all of this works correctly unless one
> > >of the selected columns (which happens to be a pathkey at this point
> > >because it's a DISTINCT query) contains a volatile expression.
> > >
> >
> > Yeah. It seems to me this is a bug in get_useful_pathkeys_for_relation,
> > which is calling find_em_expr_for_rel and is happy with anything it
> > returns. But this was copied from postgres_fdw, which however does a bit
> > more here:
> >
> >      if (pathkey_ec->ec_has_volatile ||
> >          !(em_expr = find_em_expr_for_rel(pathkey_ec, rel)) ||
> >          !is_foreign_expr(root, rel, em_expr))
> >
> > So not only does it explicitly check volatility of the pathkey, it also
> > calls is_foreign_expr which checks the expression for mutable functions.
> >
> > The attached patch seems to fix this, but it only adds the check for
> > mutable functions. Maybe it should check ec_has_volatile too ...
>
> We actually discussed the volatility check in that function back on
> the original thread [1], and we'd concluded that was specifically
> necessary for the fdw code because the function would execute on two
> different servers (and thus produce different results), but that in a
> local server only scenario it should be fine.
>
> My understanding (correct me if I'm wrong) is that the volatile
> function should only be executed once (at the scan level?) to build
> the tuple and from then on the datum isn't going to change, so I'm not
> sure why the volatility would matter here.
>
> James
>
> 1: https://www.postgresql.org/message-id/20200328025830.6v6ogkseohakc32q%40development

Oh, hmm, could what I said all be true, but there still be some rule
that you shouldn't compare datums generated from volatile expressions
in different backends (i.e., parallel query)?

James


Reply | Threaded
Open this post in threaded view
|

Re: enable_incremental_sort changes query behavior

Tomas Vondra-4
On Fri, Oct 02, 2020 at 10:55:14AM -0400, James Coleman wrote:

>On Fri, Oct 2, 2020 at 10:53 AM James Coleman <[hidden email]> wrote:
>>
>> On Fri, Oct 2, 2020 at 10:32 AM Tomas Vondra
>> <[hidden email]> wrote:
>> >
>> > On Fri, Oct 02, 2020 at 09:19:44AM -0400, James Coleman wrote:
>> > >
>> > > ...
>> > >
>> > >I've been able to confirm that the problem goes away if we stop adding
>> > >the gather merge paths in generate_useful_gather_paths().
>> > >
>> > >I'm not sure yet what conclusion that leads us to. It seems to be that
>> > >the biggest clue remains that all of this works correctly unless one
>> > >of the selected columns (which happens to be a pathkey at this point
>> > >because it's a DISTINCT query) contains a volatile expression.
>> > >
>> >
>> > Yeah. It seems to me this is a bug in get_useful_pathkeys_for_relation,
>> > which is calling find_em_expr_for_rel and is happy with anything it
>> > returns. But this was copied from postgres_fdw, which however does a bit
>> > more here:
>> >
>> >      if (pathkey_ec->ec_has_volatile ||
>> >          !(em_expr = find_em_expr_for_rel(pathkey_ec, rel)) ||
>> >          !is_foreign_expr(root, rel, em_expr))
>> >
>> > So not only does it explicitly check volatility of the pathkey, it also
>> > calls is_foreign_expr which checks the expression for mutable functions.
>> >
>> > The attached patch seems to fix this, but it only adds the check for
>> > mutable functions. Maybe it should check ec_has_volatile too ...
>>
>> We actually discussed the volatility check in that function back on
>> the original thread [1], and we'd concluded that was specifically
>> necessary for the fdw code because the function would execute on two
>> different servers (and thus produce different results), but that in a
>> local server only scenario it should be fine.
>>
>> My understanding (correct me if I'm wrong) is that the volatile
>> function should only be executed once (at the scan level?) to build
>> the tuple and from then on the datum isn't going to change, so I'm not
>> sure why the volatility would matter here.
>>
>> James
>>
>> 1: https://www.postgresql.org/message-id/20200328025830.6v6ogkseohakc32q%40development
>
>Oh, hmm, could what I said all be true, but there still be some rule
>that you shouldn't compare datums generated from volatile expressions
>in different backends (i.e., parallel query)?
>

I'm not sure it's all that related to parallel query - it's more likely
that when constructing the paths below the gather merge, this new code
fails to do something important.

I'm not 100% sure how the grouping and volatile functions work, so let
me think aloud here ...

The backtrace looks like this:

     #0  get_sortgroupref_tle
     #1  0x0000000000808ab9 in prepare_sort_from_pathkeys
     #2  0x000000000080926c in make_sort_from_pathkeys
     #3  0x0000000000801032 in create_sort_plan
     #4  0x00000000007fe7e0 in create_plan_recurse
     #5  0x0000000000800b2c in create_gather_merge_plan
     #6  0x00000000007fe94d in create_plan_recurse
     #7  0x0000000000805328 in create_nestloop_plan
     #8  0x00000000007ff3c5 in create_join_plan
     #9  0x00000000007fe5f8 in create_plan_recurse
     #10 0x0000000000800d68 in create_projection_plan
     #11 0x00000000007fe662 in create_plan_recurse
     #12 0x0000000000801252 in create_upper_unique_plan
     #13 0x00000000007fe760 in create_plan_recurse
     #14 0x00000000007fe4f2 in create_plan
     #15 0x000000000081082f in standard_planner

and the create_sort_plan works with lefttree that is IndexScan, so the
query we're constructing looks like this:

    Distinct
     -> Nestloop
         -> Gather Merge
            -> Sort
                -> Index Scan

and it's the sort that expects to find the expression in the Index Scan
target list. Which seems rather bogus, because clearly the index scan
does not include the expression. (I wonder if it's somehow related that
indexes can't be built on volatile expressions ...)

Anyway, the index scan clearly does not include the expression the sort
references, hence the failure. And the index can can't compute it,
because we probably need to compute it on top of the join I think
(otherwise we might get duplicate values for volatile functions etc.)


Looking at this from a slightly different angle, the root cause here
seems to be that generate_useful_gather_paths uses the pathkeys it gets
from get_useful_pathkeys_for_relation, which means root->query_pathkeys.
But all other create_gather_merge_calls use root->sort_pathkeys, so
maybe this is the actual problem and get_useful_pathkeys_for_relation
should use root->sort_pathkeys instead. That does fix the issue for me
too (and it passes all regression tests).


regards

--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: enable_incremental_sort changes query behavior

James Coleman
On Fri, Oct 2, 2020 at 2:25 PM Tomas Vondra
<[hidden email]> wrote:

>
> On Fri, Oct 02, 2020 at 10:55:14AM -0400, James Coleman wrote:
> >On Fri, Oct 2, 2020 at 10:53 AM James Coleman <[hidden email]> wrote:
> >>
> >> On Fri, Oct 2, 2020 at 10:32 AM Tomas Vondra
> >> <[hidden email]> wrote:
> >> >
> >> > On Fri, Oct 02, 2020 at 09:19:44AM -0400, James Coleman wrote:
> >> > >
> >> > > ...
> >> > >
> >> > >I've been able to confirm that the problem goes away if we stop adding
> >> > >the gather merge paths in generate_useful_gather_paths().
> >> > >
> >> > >I'm not sure yet what conclusion that leads us to. It seems to be that
> >> > >the biggest clue remains that all of this works correctly unless one
> >> > >of the selected columns (which happens to be a pathkey at this point
> >> > >because it's a DISTINCT query) contains a volatile expression.
> >> > >
> >> >
> >> > Yeah. It seems to me this is a bug in get_useful_pathkeys_for_relation,
> >> > which is calling find_em_expr_for_rel and is happy with anything it
> >> > returns. But this was copied from postgres_fdw, which however does a bit
> >> > more here:
> >> >
> >> >      if (pathkey_ec->ec_has_volatile ||
> >> >          !(em_expr = find_em_expr_for_rel(pathkey_ec, rel)) ||
> >> >          !is_foreign_expr(root, rel, em_expr))
> >> >
> >> > So not only does it explicitly check volatility of the pathkey, it also
> >> > calls is_foreign_expr which checks the expression for mutable functions.
> >> >
> >> > The attached patch seems to fix this, but it only adds the check for
> >> > mutable functions. Maybe it should check ec_has_volatile too ...
> >>
> >> We actually discussed the volatility check in that function back on
> >> the original thread [1], and we'd concluded that was specifically
> >> necessary for the fdw code because the function would execute on two
> >> different servers (and thus produce different results), but that in a
> >> local server only scenario it should be fine.
> >>
> >> My understanding (correct me if I'm wrong) is that the volatile
> >> function should only be executed once (at the scan level?) to build
> >> the tuple and from then on the datum isn't going to change, so I'm not
> >> sure why the volatility would matter here.
> >>
> >> James
> >>
> >> 1: https://www.postgresql.org/message-id/20200328025830.6v6ogkseohakc32q%40development
> >
> >Oh, hmm, could what I said all be true, but there still be some rule
> >that you shouldn't compare datums generated from volatile expressions
> >in different backends (i.e., parallel query)?
> >
>
> I'm not sure it's all that related to parallel query - it's more likely
> that when constructing the paths below the gather merge, this new code
> fails to do something important.
>
> I'm not 100% sure how the grouping and volatile functions work, so let
> me think aloud here ...
>
> The backtrace looks like this:
>
>      #0  get_sortgroupref_tle
>      #1  0x0000000000808ab9 in prepare_sort_from_pathkeys
>      #2  0x000000000080926c in make_sort_from_pathkeys
>      #3  0x0000000000801032 in create_sort_plan
>      #4  0x00000000007fe7e0 in create_plan_recurse
>      #5  0x0000000000800b2c in create_gather_merge_plan
>      #6  0x00000000007fe94d in create_plan_recurse
>      #7  0x0000000000805328 in create_nestloop_plan
>      #8  0x00000000007ff3c5 in create_join_plan
>      #9  0x00000000007fe5f8 in create_plan_recurse
>      #10 0x0000000000800d68 in create_projection_plan
>      #11 0x00000000007fe662 in create_plan_recurse
>      #12 0x0000000000801252 in create_upper_unique_plan
>      #13 0x00000000007fe760 in create_plan_recurse
>      #14 0x00000000007fe4f2 in create_plan
>      #15 0x000000000081082f in standard_planner
>
> and the create_sort_plan works with lefttree that is IndexScan, so the
> query we're constructing looks like this:
>
>     Distinct
>      -> Nestloop
>          -> Gather Merge
>             -> Sort
>                 -> Index Scan
>
> and it's the sort that expects to find the expression in the Index Scan
> target list. Which seems rather bogus, because clearly the index scan
> does not include the expression. (I wonder if it's somehow related that
> indexes can't be built on volatile expressions ...)

The index scan does include values not found in the index though,
right? It returns the whole tuple -- though I'm not sure if it
includes calculated values (BTW: is this what is meant by a
"projection" being added? Or is that something else?)

> Anyway, the index scan clearly does not include the expression the sort
> references, hence the failure. And the index can can't compute it,
> because we probably need to compute it on top of the join I think
> (otherwise we might get duplicate values for volatile functions etc.)

In this particular query there's no reason why we'd have to wait to
compute it until after the join, right?

For example, if we take away the parallelism but still force a Unique
plan, we get this:

 Unique
   ->  Sort
         Sort Key: ref_0.i, (CASE WHEN pg_rotate_logfile_old() THEN
ref_0.t ELSE ref_0.t END)
         ->  Nested Loop
               ->  Seq Scan on ref_0
               ->  Function Scan on generate_series ref_1

And I don't see any reason why the CASE statement couldn't in theory
(I don't know the internals enough to know when it actually happens)
be done as part of the base relation scan (in this case, the seq
scan). It's not dependent on any information from the join.

Furthermore the same plan works correctly for a non-volatile
expression, so that means (I think) that the relation scan is capable
of producing that expression in the tuple it generates (either that or
the sort is generating it?).

> Looking at this from a slightly different angle, the root cause here
> seems to be that generate_useful_gather_paths uses the pathkeys it gets
> from get_useful_pathkeys_for_relation, which means root->query_pathkeys.
> But all other create_gather_merge_calls use root->sort_pathkeys, so
> maybe this is the actual problem and get_useful_pathkeys_for_relation
> should use root->sort_pathkeys instead. That does fix the issue for me
> too (and it passes all regression tests).

So I'm not convinced this is the correct way forward. It seems we want
to be able to apply this as broadly as possible, and using
sort_pathkeys here means we wouldn't use it for the DISTINCT case at
all, right?

So far I think excluding volatile expressions (or mutable functions?)
is the best way we've discussed so far, though I'm clear yet on why it
is that that's a necessary restriction. If the answer is "there are
cases where it would be unsafe for parallel query even though it isn't
here...", I'm fine with that, but if so, we should make sure that's in
the code comments/readmes somewhere.

I also verified that the functions I've been playing with
(pg_rotate_logfile_old and md5) are marked parallel safe in pg_proc.

James


Reply | Threaded
Open this post in threaded view
|

Re: enable_incremental_sort changes query behavior

Tomas Vondra-4
On Fri, Oct 02, 2020 at 04:12:11PM -0400, James Coleman wrote:

>On Fri, Oct 2, 2020 at 2:25 PM Tomas Vondra
><[hidden email]> wrote:
>>
>> On Fri, Oct 02, 2020 at 10:55:14AM -0400, James Coleman wrote:
>> >On Fri, Oct 2, 2020 at 10:53 AM James Coleman <[hidden email]> wrote:
>> >>
>> >> On Fri, Oct 2, 2020 at 10:32 AM Tomas Vondra
>> >> <[hidden email]> wrote:
>> >> >
>> >> > On Fri, Oct 02, 2020 at 09:19:44AM -0400, James Coleman wrote:
>> >> > >
>> >> > > ...
>> >> > >
>> >> > >I've been able to confirm that the problem goes away if we stop adding
>> >> > >the gather merge paths in generate_useful_gather_paths().
>> >> > >
>> >> > >I'm not sure yet what conclusion that leads us to. It seems to be that
>> >> > >the biggest clue remains that all of this works correctly unless one
>> >> > >of the selected columns (which happens to be a pathkey at this point
>> >> > >because it's a DISTINCT query) contains a volatile expression.
>> >> > >
>> >> >
>> >> > Yeah. It seems to me this is a bug in get_useful_pathkeys_for_relation,
>> >> > which is calling find_em_expr_for_rel and is happy with anything it
>> >> > returns. But this was copied from postgres_fdw, which however does a bit
>> >> > more here:
>> >> >
>> >> >      if (pathkey_ec->ec_has_volatile ||
>> >> >          !(em_expr = find_em_expr_for_rel(pathkey_ec, rel)) ||
>> >> >          !is_foreign_expr(root, rel, em_expr))
>> >> >
>> >> > So not only does it explicitly check volatility of the pathkey, it also
>> >> > calls is_foreign_expr which checks the expression for mutable functions.
>> >> >
>> >> > The attached patch seems to fix this, but it only adds the check for
>> >> > mutable functions. Maybe it should check ec_has_volatile too ...
>> >>
>> >> We actually discussed the volatility check in that function back on
>> >> the original thread [1], and we'd concluded that was specifically
>> >> necessary for the fdw code because the function would execute on two
>> >> different servers (and thus produce different results), but that in a
>> >> local server only scenario it should be fine.
>> >>
>> >> My understanding (correct me if I'm wrong) is that the volatile
>> >> function should only be executed once (at the scan level?) to build
>> >> the tuple and from then on the datum isn't going to change, so I'm not
>> >> sure why the volatility would matter here.
>> >>
>> >> James
>> >>
>> >> 1: https://www.postgresql.org/message-id/20200328025830.6v6ogkseohakc32q%40development
>> >
>> >Oh, hmm, could what I said all be true, but there still be some rule
>> >that you shouldn't compare datums generated from volatile expressions
>> >in different backends (i.e., parallel query)?
>> >
>>
>> I'm not sure it's all that related to parallel query - it's more likely
>> that when constructing the paths below the gather merge, this new code
>> fails to do something important.
>>
>> I'm not 100% sure how the grouping and volatile functions work, so let
>> me think aloud here ...
>>
>> The backtrace looks like this:
>>
>>      #0  get_sortgroupref_tle
>>      #1  0x0000000000808ab9 in prepare_sort_from_pathkeys
>>      #2  0x000000000080926c in make_sort_from_pathkeys
>>      #3  0x0000000000801032 in create_sort_plan
>>      #4  0x00000000007fe7e0 in create_plan_recurse
>>      #5  0x0000000000800b2c in create_gather_merge_plan
>>      #6  0x00000000007fe94d in create_plan_recurse
>>      #7  0x0000000000805328 in create_nestloop_plan
>>      #8  0x00000000007ff3c5 in create_join_plan
>>      #9  0x00000000007fe5f8 in create_plan_recurse
>>      #10 0x0000000000800d68 in create_projection_plan
>>      #11 0x00000000007fe662 in create_plan_recurse
>>      #12 0x0000000000801252 in create_upper_unique_plan
>>      #13 0x00000000007fe760 in create_plan_recurse
>>      #14 0x00000000007fe4f2 in create_plan
>>      #15 0x000000000081082f in standard_planner
>>
>> and the create_sort_plan works with lefttree that is IndexScan, so the
>> query we're constructing looks like this:
>>
>>     Distinct
>>      -> Nestloop
>>          -> Gather Merge
>>             -> Sort
>>                 -> Index Scan
>>
>> and it's the sort that expects to find the expression in the Index Scan
>> target list. Which seems rather bogus, because clearly the index scan
>> does not include the expression. (I wonder if it's somehow related that
>> indexes can't be built on volatile expressions ...)
>
>The index scan does include values not found in the index though,
>right? It returns the whole tuple -- though I'm not sure if it
>includes calculated values (BTW: is this what is meant by a
>"projection" being added? Or is that something else?)
>
AFAIK index scans are projection-capable - at least it seems like that
from is_projection_capable_path. But the volatility blocks that, I think
(see below).

>> Anyway, the index scan clearly does not include the expression the sort
>> references, hence the failure. And the index can can't compute it,
>> because we probably need to compute it on top of the join I think
>> (otherwise we might get duplicate values for volatile functions etc.)
>
>In this particular query there's no reason why we'd have to wait to
>compute it until after the join, right?
>
>For example, if we take away the parallelism but still force a Unique
>plan, we get this:
>
> Unique
>   ->  Sort
>         Sort Key: ref_0.i, (CASE WHEN pg_rotate_logfile_old() THEN
>ref_0.t ELSE ref_0.t END)
>         ->  Nested Loop
>               ->  Seq Scan on ref_0
>               ->  Function Scan on generate_series ref_1
>
>And I don't see any reason why the CASE statement couldn't in theory
>(I don't know the internals enough to know when it actually happens)
>be done as part of the base relation scan (in this case, the seq
>scan). It's not dependent on any information from the join.
>
Imagine a query like this:

    select t1.id, volatile_func() from t1 join t2 using (id);

and now assume t2 contains duplicate values. If the volatile_func gets
evaluated as part of the t1 scan, then we'll get multiple occurrences
in the results, due to the t2 duplicates. I belive volatile functions
are not expected to behave like that - the docs say:

    A query using a volatile function will re-evaluate the function at
    every row where its value is needed..

And I assume this references to rows at the level where the function is
used, i.e. after the join.

>Furthermore the same plan works correctly for a non-volatile
>expression, so that means (I think) that the relation scan is capable
>of producing that expression in the tuple it generates (either that or
>the sort is generating it?).
>

I do believe the reason is exactly that non-volatile expressions can be
pushed down, so that we actually find them in the target list of the
node below the sort.

See explains for the second set of queries, where I used a simple
non-volatile expression.

>> Looking at this from a slightly different angle, the root cause here
>> seems to be that generate_useful_gather_paths uses the pathkeys it gets
>> from get_useful_pathkeys_for_relation, which means root->query_pathkeys.
>> But all other create_gather_merge_calls use root->sort_pathkeys, so
>> maybe this is the actual problem and get_useful_pathkeys_for_relation
>> should use root->sort_pathkeys instead. That does fix the issue for me
>> too (and it passes all regression tests).
>
>So I'm not convinced this is the correct way forward. It seems we want
>to be able to apply this as broadly as possible, and using
>sort_pathkeys here means we wouldn't use it for the DISTINCT case at
>all, right?
>
>So far I think excluding volatile expressions (or mutable functions?)
>is the best way we've discussed so far, though I'm clear yet on why it
>is that that's a necessary restriction. If the answer is "there are
>cases where it would be unsafe for parallel query even though it isn't
>here...", I'm fine with that, but if so, we should make sure that's in
>the code comments/readmes somewhere.
>
>I also verified that the functions I've been playing with
>(pg_rotate_logfile_old and md5) are marked parallel safe in pg_proc.
>
More importanly, it does not actually fix the issue - it does fix that
particular query, but just replacing the DISTINCT with either ORDER BY
or GROUP BY make it fail again :-(

Attached is a simple script I used, demonstrating these issues for the
three cases that expect to have ressortgroupref != 0 (per the comment
before TargetEntry in plannodes.h).


regards

--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

failure.sql (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: enable_incremental_sort changes query behavior

James Coleman
On Fri, Oct 2, 2020 at 4:56 PM Tomas Vondra
<[hidden email]> wrote:

>
> On Fri, Oct 02, 2020 at 04:12:11PM -0400, James Coleman wrote:
> >On Fri, Oct 2, 2020 at 2:25 PM Tomas Vondra
> ><[hidden email]> wrote:
> >>
> >> On Fri, Oct 02, 2020 at 10:55:14AM -0400, James Coleman wrote:
> >> >On Fri, Oct 2, 2020 at 10:53 AM James Coleman <[hidden email]> wrote:
> >> >>
> >> >> On Fri, Oct 2, 2020 at 10:32 AM Tomas Vondra
> >> >> <[hidden email]> wrote:
> >> >> >
> >> >> > On Fri, Oct 02, 2020 at 09:19:44AM -0400, James Coleman wrote:
> >> >> > >
> >> >> > > ...
> >> >> > >
> >> >> > >I've been able to confirm that the problem goes away if we stop adding
> >> >> > >the gather merge paths in generate_useful_gather_paths().
> >> >> > >
> >> >> > >I'm not sure yet what conclusion that leads us to. It seems to be that
> >> >> > >the biggest clue remains that all of this works correctly unless one
> >> >> > >of the selected columns (which happens to be a pathkey at this point
> >> >> > >because it's a DISTINCT query) contains a volatile expression.
> >> >> > >
> >> >> >
> >> >> > Yeah. It seems to me this is a bug in get_useful_pathkeys_for_relation,
> >> >> > which is calling find_em_expr_for_rel and is happy with anything it
> >> >> > returns. But this was copied from postgres_fdw, which however does a bit
> >> >> > more here:
> >> >> >
> >> >> >      if (pathkey_ec->ec_has_volatile ||
> >> >> >          !(em_expr = find_em_expr_for_rel(pathkey_ec, rel)) ||
> >> >> >          !is_foreign_expr(root, rel, em_expr))
> >> >> >
> >> >> > So not only does it explicitly check volatility of the pathkey, it also
> >> >> > calls is_foreign_expr which checks the expression for mutable functions.
> >> >> >
> >> >> > The attached patch seems to fix this, but it only adds the check for
> >> >> > mutable functions. Maybe it should check ec_has_volatile too ...
> >> >>
> >> >> We actually discussed the volatility check in that function back on
> >> >> the original thread [1], and we'd concluded that was specifically
> >> >> necessary for the fdw code because the function would execute on two
> >> >> different servers (and thus produce different results), but that in a
> >> >> local server only scenario it should be fine.
> >> >>
> >> >> My understanding (correct me if I'm wrong) is that the volatile
> >> >> function should only be executed once (at the scan level?) to build
> >> >> the tuple and from then on the datum isn't going to change, so I'm not
> >> >> sure why the volatility would matter here.
> >> >>
> >> >> James
> >> >>
> >> >> 1: https://www.postgresql.org/message-id/20200328025830.6v6ogkseohakc32q%40development
> >> >
> >> >Oh, hmm, could what I said all be true, but there still be some rule
> >> >that you shouldn't compare datums generated from volatile expressions
> >> >in different backends (i.e., parallel query)?
> >> >
> >>
> >> I'm not sure it's all that related to parallel query - it's more likely
> >> that when constructing the paths below the gather merge, this new code
> >> fails to do something important.
> >>
> >> I'm not 100% sure how the grouping and volatile functions work, so let
> >> me think aloud here ...
> >>
> >> The backtrace looks like this:
> >>
> >>      #0  get_sortgroupref_tle
> >>      #1  0x0000000000808ab9 in prepare_sort_from_pathkeys
> >>      #2  0x000000000080926c in make_sort_from_pathkeys
> >>      #3  0x0000000000801032 in create_sort_plan
> >>      #4  0x00000000007fe7e0 in create_plan_recurse
> >>      #5  0x0000000000800b2c in create_gather_merge_plan
> >>      #6  0x00000000007fe94d in create_plan_recurse
> >>      #7  0x0000000000805328 in create_nestloop_plan
> >>      #8  0x00000000007ff3c5 in create_join_plan
> >>      #9  0x00000000007fe5f8 in create_plan_recurse
> >>      #10 0x0000000000800d68 in create_projection_plan
> >>      #11 0x00000000007fe662 in create_plan_recurse
> >>      #12 0x0000000000801252 in create_upper_unique_plan
> >>      #13 0x00000000007fe760 in create_plan_recurse
> >>      #14 0x00000000007fe4f2 in create_plan
> >>      #15 0x000000000081082f in standard_planner
> >>
> >> and the create_sort_plan works with lefttree that is IndexScan, so the
> >> query we're constructing looks like this:
> >>
> >>     Distinct
> >>      -> Nestloop
> >>          -> Gather Merge
> >>             -> Sort
> >>                 -> Index Scan
> >>
> >> and it's the sort that expects to find the expression in the Index Scan
> >> target list. Which seems rather bogus, because clearly the index scan
> >> does not include the expression. (I wonder if it's somehow related that
> >> indexes can't be built on volatile expressions ...)
> >
> >The index scan does include values not found in the index though,
> >right? It returns the whole tuple -- though I'm not sure if it
> >includes calculated values (BTW: is this what is meant by a
> >"projection" being added? Or is that something else?)
> >
>
> AFAIK index scans are projection-capable - at least it seems like that
> from is_projection_capable_path. But the volatility blocks that, I think
> (see below).
>
> >> Anyway, the index scan clearly does not include the expression the sort
> >> references, hence the failure. And the index can can't compute it,
> >> because we probably need to compute it on top of the join I think
> >> (otherwise we might get duplicate values for volatile functions etc.)
> >
> >In this particular query there's no reason why we'd have to wait to
> >compute it until after the join, right?
> >
> >For example, if we take away the parallelism but still force a Unique
> >plan, we get this:
> >
> > Unique
> >   ->  Sort
> >         Sort Key: ref_0.i, (CASE WHEN pg_rotate_logfile_old() THEN
> >ref_0.t ELSE ref_0.t END)
> >         ->  Nested Loop
> >               ->  Seq Scan on ref_0
> >               ->  Function Scan on generate_series ref_1
> >
> >And I don't see any reason why the CASE statement couldn't in theory
> >(I don't know the internals enough to know when it actually happens)
> >be done as part of the base relation scan (in this case, the seq
> >scan). It's not dependent on any information from the join.
> >
>
> Imagine a query like this:
>
>     select t1.id, volatile_func() from t1 join t2 using (id);
>
> and now assume t2 contains duplicate values. If the volatile_func gets
> evaluated as part of the t1 scan, then we'll get multiple occurrences
> in the results, due to the t2 duplicates. I belive volatile functions
> are not expected to behave like that - the docs say:
>
>     A query using a volatile function will re-evaluate the function at
>     every row where its value is needed..
>
> And I assume this references to rows at the level where the function is
> used, i.e. after the join.
>
> >Furthermore the same plan works correctly for a non-volatile
> >expression, so that means (I think) that the relation scan is capable
> >of producing that expression in the tuple it generates (either that or
> >the sort is generating it?).
> >
>
> I do believe the reason is exactly that non-volatile expressions can be
> pushed down, so that we actually find them in the target list of the
> node below the sort.
>
> See explains for the second set of queries, where I used a simple
> non-volatile expression.
>
> >> Looking at this from a slightly different angle, the root cause here
> >> seems to be that generate_useful_gather_paths uses the pathkeys it gets
> >> from get_useful_pathkeys_for_relation, which means root->query_pathkeys.
> >> But all other create_gather_merge_calls use root->sort_pathkeys, so
> >> maybe this is the actual problem and get_useful_pathkeys_for_relation
> >> should use root->sort_pathkeys instead. That does fix the issue for me
> >> too (and it passes all regression tests).
> >
> >So I'm not convinced this is the correct way forward. It seems we want
> >to be able to apply this as broadly as possible, and using
> >sort_pathkeys here means we wouldn't use it for the DISTINCT case at
> >all, right?
> >
> >So far I think excluding volatile expressions (or mutable functions?)
> >is the best way we've discussed so far, though I'm clear yet on why it
> >is that that's a necessary restriction. If the answer is "there are
> >cases where it would be unsafe for parallel query even though it isn't
> >here...", I'm fine with that, but if so, we should make sure that's in
> >the code comments/readmes somewhere.
> >
> >I also verified that the functions I've been playing with
> >(pg_rotate_logfile_old and md5) are marked parallel safe in pg_proc.
> >
>
> More importanly, it does not actually fix the issue - it does fix that
> particular query, but just replacing the DISTINCT with either ORDER BY
> or GROUP BY make it fail again :-(
>
> Attached is a simple script I used, demonstrating these issues for the
> three cases that expect to have ressortgroupref != 0 (per the comment
> before TargetEntry in plannodes.h).

So does checking for volatile expressions (if you happened to test
that) solve all the cases? If you haven't tested that yet, I can try
to do that this evening.

James


Reply | Threaded
Open this post in threaded view
|

Re: enable_incremental_sort changes query behavior

James Coleman
In reply to this post by Tomas Vondra-4
On Fri, Oct 2, 2020 at 4:56 PM Tomas Vondra
<[hidden email]> wrote:

> >And I don't see any reason why the CASE statement couldn't in theory
> >(I don't know the internals enough to know when it actually happens)
> >be done as part of the base relation scan (in this case, the seq
> >scan). It's not dependent on any information from the join.
> >
>
> Imagine a query like this:
>
>     select t1.id, volatile_func() from t1 join t2 using (id);
>
> and now assume t2 contains duplicate values. If the volatile_func gets
> evaluated as part of the t1 scan, then we'll get multiple occurrences
> in the results, due to the t2 duplicates. I belive volatile functions
> are not expected to behave like that - the docs say:
>
>     A query using a volatile function will re-evaluate the function at
>     every row where its value is needed..
>
> And I assume this references to rows at the level where the function is
> used, i.e. after the join.

Ah, this makes sense. I was thinking exactly the opposite: that you'd
want not to execute volatile functions multiple times for the same
base rel row, but now that you describe it it makes sense why they
shouldn't be.

James


Reply | Threaded
Open this post in threaded view
|

Re: enable_incremental_sort changes query behavior

Tomas Vondra-4
In reply to this post by James Coleman
On Fri, Oct 02, 2020 at 05:45:52PM -0400, James Coleman wrote:

>On Fri, Oct 2, 2020 at 4:56 PM Tomas Vondra
><[hidden email]> wrote:
>>
>> ...
>>
>> More importanly, it does not actually fix the issue - it does fix that
>> particular query, but just replacing the DISTINCT with either ORDER BY
>> or GROUP BY make it fail again :-(
>>
>> Attached is a simple script I used, demonstrating these issues for the
>> three cases that expect to have ressortgroupref != 0 (per the comment
>> before TargetEntry in plannodes.h).
>
>So does checking for volatile expressions (if you happened to test
>that) solve all the cases? If you haven't tested that yet, I can try
>to do that this evening.
>

Yes, it does fix all the three queries in the SQL script.

The question however is whether this is the root issue, and whether it's
the right way to fix it. For example - volatility is not the only reason
that may block qual pushdown. If you look at qual_is_pushdown_safe, it
also blocks pushdown of leaky functions in security_barrier views. So I
wonder if that could cause failures too, somehow. But I haven't managed
to create such example.

regards

--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: enable_incremental_sort changes query behavior

James Coleman
On Fri, Oct 2, 2020 at 6:28 PM Tomas Vondra
<[hidden email]> wrote:

>
> On Fri, Oct 02, 2020 at 05:45:52PM -0400, James Coleman wrote:
> >On Fri, Oct 2, 2020 at 4:56 PM Tomas Vondra
> ><[hidden email]> wrote:
> >>
> >> ...
> >>
> >> More importanly, it does not actually fix the issue - it does fix that
> >> particular query, but just replacing the DISTINCT with either ORDER BY
> >> or GROUP BY make it fail again :-(
> >>
> >> Attached is a simple script I used, demonstrating these issues for the
> >> three cases that expect to have ressortgroupref != 0 (per the comment
> >> before TargetEntry in plannodes.h).
> >
> >So does checking for volatile expressions (if you happened to test
> >that) solve all the cases? If you haven't tested that yet, I can try
> >to do that this evening.
> >
>
> Yes, it does fix all the three queries in the SQL script.
>
> The question however is whether this is the root issue, and whether it's
> the right way to fix it. For example - volatility is not the only reason
> that may block qual pushdown. If you look at qual_is_pushdown_safe, it
> also blocks pushdown of leaky functions in security_barrier views. So I
> wonder if that could cause failures too, somehow. But I haven't managed
> to create such example.

I was about to say that the issue here is slightly different from qual
etc. pushdown, since we're not concerned about quals here, and so I
wonder where we determine what target list entries to put in a given
scan path, but then I realized that implies (maybe!) a simpler
solution. Instead of duplicating checks on target list entries would
be safe, why not check directly in get_useful_pathkeys_for_relation()
whether or not the pathkey has a target list entry?

I haven't been able to try that out yet, and so maybe I'm missing
something, but I need to step out for a bit, so I'll have to look at
it later.

James


Reply | Threaded
Open this post in threaded view
|

Re: enable_incremental_sort changes query behavior

James Coleman
On Fri, Oct 2, 2020 at 7:07 PM James Coleman <[hidden email]> wrote:

>
> On Fri, Oct 2, 2020 at 6:28 PM Tomas Vondra
> <[hidden email]> wrote:
> >
> > On Fri, Oct 02, 2020 at 05:45:52PM -0400, James Coleman wrote:
> > >On Fri, Oct 2, 2020 at 4:56 PM Tomas Vondra
> > ><[hidden email]> wrote:
> > >>
> > >> ...
> > >>
> > >> More importanly, it does not actually fix the issue - it does fix that
> > >> particular query, but just replacing the DISTINCT with either ORDER BY
> > >> or GROUP BY make it fail again :-(
> > >>
> > >> Attached is a simple script I used, demonstrating these issues for the
> > >> three cases that expect to have ressortgroupref != 0 (per the comment
> > >> before TargetEntry in plannodes.h).
> > >
> > >So does checking for volatile expressions (if you happened to test
> > >that) solve all the cases? If you haven't tested that yet, I can try
> > >to do that this evening.
> > >
> >
> > Yes, it does fix all the three queries in the SQL script.
> >
> > The question however is whether this is the root issue, and whether it's
> > the right way to fix it. For example - volatility is not the only reason
> > that may block qual pushdown. If you look at qual_is_pushdown_safe, it
> > also blocks pushdown of leaky functions in security_barrier views. So I
> > wonder if that could cause failures too, somehow. But I haven't managed
> > to create such example.
>
> I was about to say that the issue here is slightly different from qual
> etc. pushdown, since we're not concerned about quals here, and so I
> wonder where we determine what target list entries to put in a given
> scan path, but then I realized that implies (maybe!) a simpler
> solution. Instead of duplicating checks on target list entries would
> be safe, why not check directly in get_useful_pathkeys_for_relation()
> whether or not the pathkey has a target list entry?
>
> I haven't been able to try that out yet, and so maybe I'm missing
> something, but I need to step out for a bit, so I'll have to look at
> it later.
I've started poking at this, but haven't yet found a workable
solution. See the attached patch which "solves" the problem by
breaking putting the sort under the gather merge, but it at least
demonstrates conceptually what I think we're interested in doing.

The issue currently is that the comparison of expressions fails -- in
our query, the first column selected shows up as a Var (with the same
contents) but is a different pointer in the em_expr and the reltarget
exprs list. I don't yet see a good way to get the equivalence class
for a PathTarget entry.

James

incremental_sort_fix_expr_lookup_idea.patch (1022 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: enable_incremental_sort changes query behavior

James Coleman
In reply to this post by Tomas Vondra-4
On Fri, Oct 2, 2020 at 2:25 PM Tomas Vondra
<[hidden email]> wrote:

>
> The backtrace looks like this:
>
>      #0  get_sortgroupref_tle
>      #1  0x0000000000808ab9 in prepare_sort_from_pathkeys
>      #2  0x000000000080926c in make_sort_from_pathkeys
>      #3  0x0000000000801032 in create_sort_plan
>      #4  0x00000000007fe7e0 in create_plan_recurse
>      #5  0x0000000000800b2c in create_gather_merge_plan
>      #6  0x00000000007fe94d in create_plan_recurse
>      #7  0x0000000000805328 in create_nestloop_plan
>      #8  0x00000000007ff3c5 in create_join_plan
>      #9  0x00000000007fe5f8 in create_plan_recurse
>      #10 0x0000000000800d68 in create_projection_plan
>      #11 0x00000000007fe662 in create_plan_recurse
>      #12 0x0000000000801252 in create_upper_unique_plan
>      #13 0x00000000007fe760 in create_plan_recurse
>      #14 0x00000000007fe4f2 in create_plan
>      #15 0x000000000081082f in standard_planner
>
> and the create_sort_plan works with lefttree that is IndexScan, so the
> query we're constructing looks like this:
>
>     Distinct
>      -> Nestloop
>          -> Gather Merge
>             -> Sort
>                 -> Index Scan
>
> and it's the sort that expects to find the expression in the Index Scan
> target list. Which seems rather bogus, because clearly the index scan
> does not include the expression. (I wonder if it's somehow related that
> indexes can't be built on volatile expressions ...)
>
> Anyway, the index scan clearly does not include the expression the sort
> references, hence the failure. And the index can can't compute it,
> because we probably need to compute it on top of the join I think
> (otherwise we might get duplicate values for volatile functions etc.)
>
>
> Looking at this from a slightly different angle, the root cause here
> seems to be that generate_useful_gather_paths uses the pathkeys it gets
> from get_useful_pathkeys_for_relation, which means root->query_pathkeys.
> But all other create_gather_merge_calls use root->sort_pathkeys, so
> maybe this is the actual problem and get_useful_pathkeys_for_relation
> should use root->sort_pathkeys instead. That does fix the issue for me
> too (and it passes all regression tests).

So I've been a bit confused how our error could come from working with
root->query_pathkeys when that's what's supposedly being set from the
make_pathkeys_for_sortclauses() call in the backtrace Jaime reported,
but I just realized that the trace I get when reproducing the error is
different -- and matches the one you shared above.

Jaime: was the backtrace in the original report by any chance record
from breakpointing in the first call to get_sortgroupref_tle() (and
one that successfully returned a sort group ref) rather than a call
that hit the elog error on line 379?

James


Reply | Threaded
Open this post in threaded view
|

Re: enable_incremental_sort changes query behavior

James Coleman
In reply to this post by James Coleman
On Fri, Oct 2, 2020 at 11:16 PM James Coleman <[hidden email]> wrote:

>
> On Fri, Oct 2, 2020 at 7:07 PM James Coleman <[hidden email]> wrote:
> >
> > On Fri, Oct 2, 2020 at 6:28 PM Tomas Vondra
> > <[hidden email]> wrote:
> > >
> > > On Fri, Oct 02, 2020 at 05:45:52PM -0400, James Coleman wrote:
> > > >On Fri, Oct 2, 2020 at 4:56 PM Tomas Vondra
> > > ><[hidden email]> wrote:
> > > >>
> > > >> ...
> > > >>
> > > >> More importanly, it does not actually fix the issue - it does fix that
> > > >> particular query, but just replacing the DISTINCT with either ORDER BY
> > > >> or GROUP BY make it fail again :-(
> > > >>
> > > >> Attached is a simple script I used, demonstrating these issues for the
> > > >> three cases that expect to have ressortgroupref != 0 (per the comment
> > > >> before TargetEntry in plannodes.h).
> > > >
> > > >So does checking for volatile expressions (if you happened to test
> > > >that) solve all the cases? If you haven't tested that yet, I can try
> > > >to do that this evening.
> > > >
> > >
> > > Yes, it does fix all the three queries in the SQL script.
> > >
> > > The question however is whether this is the root issue, and whether it's
> > > the right way to fix it. For example - volatility is not the only reason
> > > that may block qual pushdown. If you look at qual_is_pushdown_safe, it
> > > also blocks pushdown of leaky functions in security_barrier views. So I
> > > wonder if that could cause failures too, somehow. But I haven't managed
> > > to create such example.
> >
> > I was about to say that the issue here is slightly different from qual
> > etc. pushdown, since we're not concerned about quals here, and so I
> > wonder where we determine what target list entries to put in a given
> > scan path, but then I realized that implies (maybe!) a simpler
> > solution. Instead of duplicating checks on target list entries would
> > be safe, why not check directly in get_useful_pathkeys_for_relation()
> > whether or not the pathkey has a target list entry?
> >
> > I haven't been able to try that out yet, and so maybe I'm missing
> > something, but I need to step out for a bit, so I'll have to look at
> > it later.
>
> I've started poking at this, but haven't yet found a workable
> solution. See the attached patch which "solves" the problem by
> breaking putting the sort under the gather merge, but it at least
> demonstrates conceptually what I think we're interested in doing.
>
> The issue currently is that the comparison of expressions fails -- in
> our query, the first column selected shows up as a Var (with the same
> contents) but is a different pointer in the em_expr and the reltarget
> exprs list. I don't yet see a good way to get the equivalence class
> for a PathTarget entry.
Hmm, I think I was looking to do is the attached patch. I didn't
realize until I did a lot of reading through source that we have an
equal() function that can compare exprs. That (plus the realization in
[1] the originally reported backtrace wasn't where the error actually
came from) convinced me that what we need is to confirm not only that
the all of the vars in the ec member come from the relids in the rel
but also that the expr is actually represented in the target of the
rel.

With the gucs changed as I mentioned earlier both of the plans (with
and without a volatile call in the 2nd select entry) now look like:

 Unique
   ->  Gather Merge
         Workers Planned: 2
         ->  Sort
               Sort Key: ref_0.i, (md5(ref_0.t))
               ->  Nested Loop
                     ->  Parallel Index Scan using ref_0_i_idx on ref_0
                     ->  Function Scan on generate_series ref_1

Without the gucs changed the minimal repro case now doesn't error, but
results in this plan:

 HashAggregate
   Group Key: ref_0.i, CASE WHEN pg_rotate_logfile_old() THEN ref_0.t
ELSE ref_0.t END
   ->  Nested Loop
         ->  Seq Scan on ref_0
         ->  Function Scan on generate_series ref_1

Similarly in your six queries I now only see parallel query showing up
in the last one.

I created an entirely new function because adding the target expr
lookup to the existing find_em_expr_for_rel() function broke a bunch
of postgres_fdw tests. That maybe raises questions about whether that
code also could have problems in theory/in the future, but I didn't
investigate further. In any case we already know it excludes
volatile...so maybe it's fine because in practice that's actually a
broader exclusion than what we're doing here.

This seems to fix the issue, but I'd like feedback on whether it's too
strict. We could of course just check em_has_volatile, but I'm
wondering if that's simultaneously too strict (by not allowing the
volatile expression to be computed in the gather merge supposing
there's no join) and too loose (maybe there are other cases we should
care about?). It also just strikes me as re-encoding rules that should
have already been applied (and thus we should be able to look up in
the data we have if it's safe to use the expr/pathkey). Those are
mostly intuitions though.

James

1: https://www.postgresql.org/message-id/CAAaqYe8zqDAv0Sfak5Riu%2BDKsm-i3ARPursn5v6qTwiCXmkXKQ%40mail.gmail.com

v1-0001-Fix-get_useful_pathkeys_for_relation-for-volatile.patch (6K) Download Attachment
123