Обсуждение: Problem, partition pruning for prepared statement with IS NULL clause.

Hello postgres hackers,

I noticed that combination of prepared statement with generic plan and
'IS NULL' clause could lead partition pruning to crash.

Affected versions start from 12 it seems.

'How to repeat' below and an attempt to fix it is in attachment.


Data set:
------
create function part_hashint4_noop(value int4, seed int8)
     returns int8 as $$
     select value + seed;
     $$ language sql strict immutable parallel safe;

create operator class part_test_int4_ops for type int4 using hash as
     operator 1 =,
     function 2 part_hashint4_noop(int4, int8);

create function part_hashtext_length(value text, seed int8)
     returns int8 as $$
     select length(coalesce(value, ''))::int8
     $$ language sql strict immutable parallel safe;

create operator class part_test_text_ops for type text using hash as
     operator 1 =,
     function 2 part_hashtext_length(text, int8);


create table hp (a int, b text, c int)
   partition by hash (a part_test_int4_ops, b part_test_text_ops);
create table hp0 partition of hp for values with (modulus 4, remainder 0);
create table hp3 partition of hp for values with (modulus 4, remainder 3);
create table hp1 partition of hp for values with (modulus 4, remainder 1);
create table hp2 partition of hp for values with (modulus 4, remainder 2);

insert into hp values (null, null, 0);
insert into hp values (1, null, 1);
insert into hp values (1, 'xxx', 2);
insert into hp values (null, 'xxx', 3);
insert into hp values (2, 'xxx', 4);
insert into hp values (1, 'abcde', 5);
------

Test case:
------
set plan_cache_mode to force_generic_plan;
prepare stmt AS select * from hp where a is null and b = $1;
explain execute stmt('xxx');
------


Regargs,
Gluh

On Fri, Oct  6, 2023 at 06:09:45PM +0400, Sergei Glukhov wrote:
> Test case:
> ------
> set plan_cache_mode to force_generic_plan;
> prepare stmt AS select * from hp where a is null and b = $1;
> explain execute stmt('xxx');
> ------

I can confirm the crash in git master.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

On Fri, Oct  6, 2023 at 05:00:54PM -0400, Bruce Momjian wrote:
> On Fri, Oct  6, 2023 at 06:09:45PM +0400, Sergei Glukhov wrote:
> > Test case:
> > ------
> > set plan_cache_mode to force_generic_plan;
> > prepare stmt AS select * from hp where a is null and b = $1;
> > explain execute stmt('xxx');
> > ------
> 
> I can confirm the crash in git master.

There were some UTF8 non-space whitespace characters in the email so
attached is a clean reproducable SQL crash file.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

On Sat, 7 Oct 2023 at 03:11, Sergei Glukhov <s.glukhov@postgrespro.ru> wrote:
> I noticed that combination of prepared statement with generic plan and
> 'IS NULL' clause could lead partition pruning to crash.

> Test case:
> ------
> set plan_cache_mode to force_generic_plan;
> prepare stmt AS select * from hp where a is null and b = $1;
> explain execute stmt('xxx');

Thanks for the detailed report and proposed patch.

I think your proposed fix isn't quite correct.  I think the problem
lies in InitPartitionPruneContext() where we assume that the list
positions of step->exprs are in sync with the keyno.  If you look at
perform_pruning_base_step() the code there makes a special effort to
skip over any keyno when a bit is set in opstep->nullkeys.

It seems that your patch is adjusting the keyno that's given to the
PruneCxtStateIdx() and it looks like (for your test case) it'll end up
passing keyno==0 when it should be passing keyno==1.  keyno is the
index of the partition key, so you can't pass 0 when it's for key
index 1.

I wonder if it's worth expanding the tests further to cover more of
the pruning cases to cover run-time pruning too.

David

Hi David,


On 10/9/23 03:26, David Rowley wrote:
> On Sat, 7 Oct 2023 at 03:11, Sergei Glukhov <s.glukhov@postgrespro.ru> wrote:
>> I noticed that combination of prepared statement with generic plan and
>> 'IS NULL' clause could lead partition pruning to crash.
>> Test case:
>> ------
>> set plan_cache_mode to force_generic_plan;
>> prepare stmt AS select * from hp where a is null and b = $1;
>> explain execute stmt('xxx');
> Thanks for the detailed report and proposed patch.
>
> I think your proposed fix isn't quite correct.  I think the problem
> lies in InitPartitionPruneContext() where we assume that the list
> positions of step->exprs are in sync with the keyno.  If you look at
> perform_pruning_base_step() the code there makes a special effort to
> skip over any keyno when a bit is set in opstep->nullkeys.
>
> It seems that your patch is adjusting the keyno that's given to the
> PruneCxtStateIdx() and it looks like (for your test case) it'll end up
> passing keyno==0 when it should be passing keyno==1.  keyno is the
> index of the partition key, so you can't pass 0 when it's for key
> index 1.
>
> I wonder if it's worth expanding the tests further to cover more of
> the pruning cases to cover run-time pruning too.

Thanks for the explanation. I thought by some reason that 'exprstates ' 
array doesn't
contain elements related to 'IS NULL' clause. Now I see that they are 
there and
just empty and untouched.

I verified the patch and it fixes the problem.

Regarding test case,
besides the current test case and the test for dynamic partition 
pruning, say,

select a, (select b from hp where a is null and b = a.b) AS b from hp a 
where a = 1 and b = 'xxx';

I would like to suggest to slightly refactor 'Test Partition pruning for 
HASH partitioning' test
from 'partition_prune.sql' and add one more key field. The reason is 
that two-element
key is not enough for thorough testing since it tests mostly corner 
cases. Let me know
if it's worth doing.

Example:
------
create table hp (a int, b text, c int, d int)
   partition by hash (a part_test_int4_ops, b part_test_text_ops, c 
part_test_int4_ops);
create table hp0 partition of hp for values with (modulus 4, remainder 0);
create table hp3 partition of hp for values with (modulus 4, remainder 3);
create table hp1 partition of hp for values with (modulus 4, remainder 1);
create table hp2 partition of hp for values with (modulus 4, remainder 2);

insert into hp values (null, null, null, 0);
insert into hp values (1, null, 1, 1);
insert into hp values (1, 'xxx', 1, 2);
insert into hp values (null, 'xxx', null, 3);
insert into hp values (2, 'xxx', 2, 4);
insert into hp values (1, 'abcde', 1, 5);
------

Another crash in the different place even with the fix:
------
explain select * from hp where a = 1 and b is null and c = 1;
------


Regards,
Gluh

On Tue, 10 Oct 2023 at 21:31, Sergei Glukhov <s.glukhov@postgrespro.ru> wrote:
> create table hp (a int, b text, c int, d int)
>    partition by hash (a part_test_int4_ops, b part_test_text_ops, c
> part_test_int4_ops);
> create table hp0 partition of hp for values with (modulus 4, remainder 0);
> create table hp3 partition of hp for values with (modulus 4, remainder 3);
> create table hp1 partition of hp for values with (modulus 4, remainder 1);
> create table hp2 partition of hp for values with (modulus 4, remainder 2);
>
>
> Another crash in the different place even with the fix:
> explain select * from hp where a = 1 and b is null and c = 1;

Ouch.  It looks like 13838740f tried to fix things in this area before
and even added a regression test for it. Namely:

-- Test that get_steps_using_prefix() handles non-NULL step_nullkeys
explain (costs off) select * from hp_prefix_test where a = 1 and b is
null and c = 1 and d = 1;

I guess that one does not crash because of the "d = 1" clause is in
the "start" ListCell in get_steps_using_prefix_recurse(), whereas,
with your case start is NULL which is an issue for cur_keyno =
((PartClauseInfo *) lfirst(start))->keyno;.

It might have been better if PartClauseInfo could also describe IS
NULL quals, but I feel if we do that now then it would require lots of
careful surgery in partprune.c to account for that.  Probably the fix
should be localised to get_steps_using_prefix_recurse() to have it do
something like pass the keyno to try and work on rather than trying to
get that from the "prefix" list. That way if there's no item in that
list for that keyno, we can check in step_nullkeys for the keyno.

I'll continue looking.

David

On Wed, 11 Oct 2023 at 15:49, David Rowley <dgrowleyml@gmail.com> wrote:
> It might have been better if PartClauseInfo could also describe IS
> NULL quals, but I feel if we do that now then it would require lots of
> careful surgery in partprune.c to account for that.  Probably the fix
> should be localised to get_steps_using_prefix_recurse() to have it do
> something like pass the keyno to try and work on rather than trying to
> get that from the "prefix" list. That way if there's no item in that
> list for that keyno, we can check in step_nullkeys for the keyno.
>
> I'll continue looking.

The fix seems to amount to the attached.  The following condition
assumes that by not recursively processing step_lastkeyno - 1 that
there will be at least one more PartClauseInfo in the prefix List to
process.  However, that didn't work when that partition key clause was
covered by an IS NULL clause.

If we adjust the following condition:

if (cur_keyno < step_lastkeyno - 1)

to become:

final_keyno = ((PartClauseInfo *) llast(prefix))->keyno;
if (cur_keyno < final_keyno)

then that ensures that the else clause can pick up any clauses for the
final column mentioned in the 'prefix' list, plus any nullkeys if
there happens to be any of those too.

For testing, given that 13838740f (from 2020) had a go at fixing this
already, I'm kinda thinking that it's not overkill to test all
possible 16 combinations of IS NULL and equality equals on the 4
partition key column partitioned table that commit added in
partition_prune.sql.

I added some tests there using \gexec to prevent having to write out
each of the 16 queries by hand. I tested that pruning worked (i.e 1
matching partition in EXPLAIN), and that we get the correct results
(i.e we pruned the correct partition) by running the query and we get
the expected 1 row after having inserted 16 rows, one for each
combination of quals to test.

I wanted to come up with some tests that test for multiple quals
matching the same partition key.  This is tricky due to the
equivalence class code being smart and removing any duplicates or
marking the rel as dummy when it finds conflicting quals.  With hash
partitioning, we're limited to just equality quals, so maybe something
could be done with range-partitioned tables instead.  I see there are
some tests just above the ones I modified which try to cover this.

I also tried to outsmart the planner by using Params and prepared
queries. Namely:

set plan_cache_mode = 'force_generic_plan';
prepare q1 (int, int, int, int, int, int, int, int) as select
tableoid::regclass,* from hp_prefix_test where a = $1 and b = $2 and c
= $3 and d = $4 and a = $5 and b = $6 and c = $7 and d = $8;
explain (costs off) execute q1 (1,2,3,4,1,2,3,4);

But I was outsmarted again with a gating qual which checked the pairs
match before doing the scan :-(

 Append
   Subplans Removed: 15
   ->  Result
         One-Time Filter: (($1 = $5) AND ($2 = $6) AND ($3 = $7) AND ($4 = $8))
         ->  Seq Scan on hp_prefix_test_p14 hp_prefix_test_1
               Filter: ((a = $5) AND (b = $6) AND (c = $7) AND (d = $8))

I'm aiming to commit these as two separate fixes, so I'm going to go
look again at the first one and wait to see if anyone wants to comment
on this patch in the meantime.

David

Hi,

Thanks for fixing this!

I verified that issues are fixed.

On 10/11/23 11:50, David Rowley wrote:

> I'm aiming to commit these as two separate fixes, so I'm going to go
> look again at the first one and wait to see if anyone wants to comment
> on this patch in the meantime.

Regarding test case for the first patch,
the line 'set plan_cache_mode = 'force_generic_plan';' is not necessary
since cache mode is set at the top of the test. On the other hand test
scenario can silently  be loosed if someone set another cache mode
somewhere upper. As you mentioned earlier it's worth maybe adding
the test for run-time partition pruning.

Regards,
Gluh

On Wed, 11 Oct 2023 at 22:09, Sergei Glukhov <s.glukhov@postgrespro.ru> wrote:
> Thanks for fixing this!
>
> I verified that issues are fixed.

Thanks for having a look.

Unfortunately, I'd not long sent the last email and realised that the
step_lastkeyno parameter is now unused and can just be removed from
both get_steps_using_prefix() and get_steps_using_prefix_recurse().
This requires some comment rewriting so I've attempted to do that too
in the attached updated version.

David

On 10/11/23 13:19, David Rowley wrote:
>
> Thanks for having a look.
>
> Unfortunately, I'd not long sent the last email and realised that the
> step_lastkeyno parameter is now unused and can just be removed from
> both get_steps_using_prefix() and get_steps_using_prefix_recurse().
> This requires some comment rewriting so I've attempted to do that too
> in the attached updated version.

Thanks, verified again and everything is fine!

Regards,
Gluh

On Wed, 11 Oct 2023 at 22:59, Sergei Glukhov <s.glukhov@postgrespro.ru> wrote:
> > Unfortunately, I'd not long sent the last email and realised that the
> > step_lastkeyno parameter is now unused and can just be removed from
> > both get_steps_using_prefix() and get_steps_using_prefix_recurse().
> > This requires some comment rewriting so I've attempted to do that too
> > in the attached updated version.
>
> Thanks, verified again and everything is fine!

Thanks for looking.  I spent quite a bit more time on this again today
and fiddled lots more with the comments and tests.

I also did more testing after finding a way to easily duplicate the
quals to cause multiple quals per partition key.  The equivalence
class code will only make ECs for mergejoin-able clauses, so if we
just find a type that's not mergejoin-able but hashable, we can
duplicate the quals with a hash partitioned table

-- find a suitable non-mergejoin-able type.
select oprleft::regtype from pg_operator where oprcanmerge=false and
oprcanhash=true;
 oprleft
---------
 xid
 cid
 aclitem

create table hash_xid(a xid, b xid, c xid) partition by hash(a,b,c);
create table hash_xid1 partition of hash_xid for values with (modulus
2, remainder 0);
create table hash_xid2 partition of hash_xid for values with (modulus
2, remainder 1);

I tried out various combinations of the following query.  Each
equality clause is duplicated 6 times.  When I enable all 6 for each
of the 3 columns, I see 216 pruning steps.  That's 6*6*6, just what I
expected.

The IS NULL quals are not duplicated since we can only set a bit once
in the nullkeys.

explain select * from hash_xid where
a = '123'::xid and a = '123'::xid and a = '123'::xid and a =
'123'::xid and a = '123'::xid and a = '123'::xid and
--a is null and a is null and a is null and a is null and a is null
and a is null and
b = '123'::xid and b = '123'::xid and b = '123'::xid and b =
'123'::xid and b = '123'::xid and b = '123'::xid and
--b is null and b is null and b is null and b is null and b is null
and b is null and
c = '123'::xid and c = '123'::xid and c = '123'::xid and c =
'123'::xid and c = '123'::xid and c = '123'::xid;
--c is null and c is null and c is null and c is null and c is null
and c is null;

putting a breakpoint at the final line of
gen_prune_steps_from_opexps() yields 216 steps.

I didn't include anything of the above as part of the additional
tests. Perhaps something like that is worthwhile in a reduced form.
However, someone might make xid mergejoinable some time, which would
break the test.

Thanks for reviewing the previous version of this patch.

Onto the other run-time one now...

David

On Mon, 9 Oct 2023 at 12:26, David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Sat, 7 Oct 2023 at 03:11, Sergei Glukhov <s.glukhov@postgrespro.ru> wrote:
> > I noticed that combination of prepared statement with generic plan and
> > 'IS NULL' clause could lead partition pruning to crash.
>
> > Test case:
> > ------
> > set plan_cache_mode to force_generic_plan;
> > prepare stmt AS select * from hp where a is null and b = $1;
> > explain execute stmt('xxx');
>
> Thanks for the detailed report and proposed patch.
>
> I think your proposed fix isn't quite correct.  I think the problem
> lies in InitPartitionPruneContext() where we assume that the list
> positions of step->exprs are in sync with the keyno.  If you look at
> perform_pruning_base_step() the code there makes a special effort to
> skip over any keyno when a bit is set in opstep->nullkeys.

I've now also pushed the fix for the incorrect logic for nullkeys in
ExecInitPruningContext().

I didn't quite find a test to make this work for v11. I tried calling
execute 5 times as we used to have to before the plan_cache_mode GUC
was added in v12, but the test case kept picking the custom plan. So I
ended up pushing v11 without any test.  This goes out of support in ~1
month, so I'm not too concerned about the lack of test.  I did do a
manual test to ensure it works with:

create table hp (a int, b text, c int) partition by hash (a, b);
create table hp0 partition of hp for values with (modulus 4, remainder 0);
create table hp3 partition of hp for values with (modulus 4, remainder 3);
create table hp1 partition of hp for values with (modulus 4, remainder 1);
create table hp2 partition of hp for values with (modulus 4, remainder 2);

prepare hp_q1 (text) as select * from hp where a is null and b = $1;

(set breakpoint in choose_custom_plan() and have it return false when
we hit it.)

explain (costs off) execute hp_q1('xxx');

David

On 10/12/23 16:27, David Rowley wrote:
>
> I've now also pushed the fix for the incorrect logic for nullkeys in
> ExecInitPruningContext().
>

Thanks!

Regards,
Gluh