Message-ID: <2f09ce72-315e-2a33-589a-8519ada8df61@postgrespro.ru>
Date: Tue, 10 Oct 2023 12:31:32 +0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.0
Subject: Re: Problem, partition pruning for prepared statement with IS NULL
 clause.
Content-Language: en-US
To: David Rowley <dgrowleyml@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
References: <d05b26fa-af54-27e1-f693-6c31590802fa@postgrespro.ru>
 <CAApHDvrc8BjkN8U3NUVzvat3EtC7bzLwFdUw_OFf3Sv-QCp-Vg@mail.gmail.com>
From: Sergei Glukhov <s.glukhov@postgrespro.ru>
In-Reply-To: 
 <CAApHDvrc8BjkN8U3NUVzvat3EtC7bzLwFdUw_OFf3Sv-QCp-Vg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/2f09ce72-315e-2a33-589a-8519ada8df61%40postgrespro.ru>
Precedence: bulk

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