Thread: Problem with default partition pruning
Hi,
I found the bug of default partition pruning when executing a range query.
-----
postgres=# create table test1(id int, val text) partition by range (id);
postgres=# create table test1_1 partition of test1 for values from (0) to (100);
postgres=# create table test1_2 partition of test1 for values from (150) to (200);
postgres=# create table test1_def partition of test1 default;
postgres=# explain select * from test1 where id > 0 and id < 30;
QUERY PLAN
----------------------------------------------------------------
Append (cost=0.00..11.83 rows=59 width=11)
-> Seq Scan on test1_1 (cost=0.00..5.00 rows=58 width=11)
Filter: ((id > 0) AND (id < 30))
-> Seq Scan on test1_def (cost=0.00..6.53 rows=1 width=12)
Filter: ((id > 0) AND (id < 30))
(5 rows)
There is no need to scan the default partition, but it's scanned.
-----
In the current implement, whether the default partition is scanned
or not is determined according to each condition of given WHERE
clause at get_matching_range_bounds(). In this example, scan_default
is set true according to id > 0 because id >= 200 matches the default
partition. Similarly, according to id < 30, scan_default is set true.
Then, these results are combined according to AND/OR at perform_pruning_combine_step().
In this case, final result's scan_default is set true.
The modifications I made are as follows:
- get_matching_range_bounds() determines only offsets of range bounds
according to each condition
- These results are combined at perform_pruning_combine_step()
- Whether the default partition is scanned or not is determined at
get_matching_partitions()
Attached the patch. Any feedback is greatly appreciated.
Best regards,
---
Yuzuko Hosoya
NTT Open Source Software Center
Attachment
Hosoya-san, On 2019/02/22 17:14, Yuzuko Hosoya wrote: > Hi, > > I found the bug of default partition pruning when executing a range query. > > ----- > postgres=# create table test1(id int, val text) partition by range (id); > postgres=# create table test1_1 partition of test1 for values from (0) to (100); > postgres=# create table test1_2 partition of test1 for values from (150) to (200); > postgres=# create table test1_def partition of test1 default; > > postgres=# explain select * from test1 where id > 0 and id < 30; > QUERY PLAN > ---------------------------------------------------------------- > Append (cost=0.00..11.83 rows=59 width=11) > -> Seq Scan on test1_1 (cost=0.00..5.00 rows=58 width=11) > Filter: ((id > 0) AND (id < 30)) > -> Seq Scan on test1_def (cost=0.00..6.53 rows=1 width=12) > Filter: ((id > 0) AND (id < 30)) > (5 rows) > > There is no need to scan the default partition, but it's scanned. > ----- > > In the current implement, whether the default partition is scanned > or not is determined according to each condition of given WHERE > clause at get_matching_range_bounds(). In this example, scan_default > is set true according to id > 0 because id >= 200 matches the default > partition. Similarly, according to id < 30, scan_default is set true. > Then, these results are combined according to AND/OR at perform_pruning_combine_step(). > In this case, final result's scan_default is set true. > > The modifications I made are as follows: > - get_matching_range_bounds() determines only offsets of range bounds > according to each condition > - These results are combined at perform_pruning_combine_step() > - Whether the default partition is scanned or not is determined at > get_matching_partitions() > > Attached the patch. Any feedback is greatly appreciated. Thank you for reporting. Can you please add this to March CF in Bugs category so as not to lose track of this? I will try to send review comments soon. Regards, Amit
Amit-san, > From: Amit Langote [mailto:Langote_Amit_f8@lab.ntt.co.jp] > Sent: Wednesday, February 27, 2019 11:22 AM > > Hosoya-san, > > On 2019/02/22 17:14, Yuzuko Hosoya wrote: > > Hi, > > > > I found the bug of default partition pruning when executing a range query. > > > > ----- > > postgres=# create table test1(id int, val text) partition by range > > (id); postgres=# create table test1_1 partition of test1 for values > > from (0) to (100); postgres=# create table test1_2 partition of test1 > > for values from (150) to (200); postgres=# create table test1_def > > partition of test1 default; > > > > postgres=# explain select * from test1 where id > 0 and id < 30; > > QUERY PLAN > > ---------------------------------------------------------------- > > Append (cost=0.00..11.83 rows=59 width=11) > > -> Seq Scan on test1_1 (cost=0.00..5.00 rows=58 width=11) > > Filter: ((id > 0) AND (id < 30)) > > -> Seq Scan on test1_def (cost=0.00..6.53 rows=1 width=12) > > Filter: ((id > 0) AND (id < 30)) > > (5 rows) > > > > There is no need to scan the default partition, but it's scanned. > > ----- > > > > In the current implement, whether the default partition is scanned or > > not is determined according to each condition of given WHERE clause at > > get_matching_range_bounds(). In this example, scan_default is set > > true according to id > 0 because id >= 200 matches the default > > partition. Similarly, according to id < 30, scan_default is set true. > > Then, these results are combined according to AND/OR at perform_pruning_combine_step(). > > In this case, final result's scan_default is set true. > > > > The modifications I made are as follows: > > - get_matching_range_bounds() determines only offsets of range bounds > > according to each condition > > - These results are combined at perform_pruning_combine_step() > > - Whether the default partition is scanned or not is determined at > > get_matching_partitions() > > > > Attached the patch. Any feedback is greatly appreciated. > > Thank you for reporting. Can you please add this to March CF in Bugs category so as not to lose track > of this? > > I will try to send review comments soon. > Thank you for your reply. I added this to March CF. Regards, Yuzuko Hosoya NTT Open Source Software Center
Hosoya-san
On Wed, Feb 27, 2019 at 6:51 AM, Yuzuko Hosoya wrote:
> > From: Amit Langote [mailto:Langote_Amit_f8@lab.ntt.co.jp]
> > Sent: Wednesday, February 27, 2019 11:22 AM
> >
> > Hosoya-san,
> >
> > On 2019/02/22 17:14, Yuzuko Hosoya wrote:
> > > Hi,
> > >
> > > I found the bug of default partition pruning when executing a range
> query.
> > >
> > > -----
> > > postgres=# create table test1(id int, val text) partition by range
> > > (id); postgres=# create table test1_1 partition of test1 for values
> > > from (0) to (100); postgres=# create table test1_2 partition of
> > > test1 for values from (150) to (200); postgres=# create table
> > > test1_def partition of test1 default;
> > >
> > > postgres=# explain select * from test1 where id > 0 and id < 30;
> > > QUERY PLAN
> > > ----------------------------------------------------------------
> > > Append (cost=0.00..11.83 rows=59 width=11)
> > > -> Seq Scan on test1_1 (cost=0.00..5.00 rows=58 width=11)
> > > Filter: ((id > 0) AND (id < 30))
> > > -> Seq Scan on test1_def (cost=0.00..6.53 rows=1 width=12)
> > > Filter: ((id > 0) AND (id < 30))
> > > (5 rows)
> > >
> > > There is no need to scan the default partition, but it's scanned.
> > > -----
> > >
> > > In the current implement, whether the default partition is scanned
> > > or not is determined according to each condition of given WHERE
> > > clause at get_matching_range_bounds(). In this example,
> > > scan_default is set true according to id > 0 because id >= 200
> > > matches the default partition. Similarly, according to id < 30,
> scan_default is set true.
> > > Then, these results are combined according to AND/OR at
> perform_pruning_combine_step().
> > > In this case, final result's scan_default is set true.
> > >
> > > The modifications I made are as follows:
> > > - get_matching_range_bounds() determines only offsets of range bounds
> > > according to each condition
> > > - These results are combined at perform_pruning_combine_step()
> > > - Whether the default partition is scanned or not is determined at
> > > get_matching_partitions()
> > >
> > > Attached the patch. Any feedback is greatly appreciated.
> >
> > Thank you for reporting. Can you please add this to March CF in Bugs
> > category so as not to lose
> track
> > of this?
> >
> > I will try to send review comments soon.
> >
> Thank you for your reply. I added this to March CF.
I tested with simple use case and I confirmed it works correctly like below.
In case using between clause:
postgres=# create table test1(id int, val text) partition by range (id);
postgres=# create table test1_1 partition of test1 for values from (0) to (100);
postgres=# create table test1_2 partition of test1 for values from (150) to (200);
postgres=# create table test1_def partition of test1 default;
[HEAD]
postgres=# explain analyze select * from test1 where id between 0 and 50;
QUERY PLAN
-----------------------------------------------------------------------------------------------------------
Append (cost=0.00..58.16 rows=12 width=36) (actual time=0.008..0.008 rows=0 loops=1)
-> Seq Scan on test1_1 (cost=0.00..29.05 rows=6 width=36) (actual time=0.005..0.005 rows=0 loops=1)
Filter: ((id >= 0) AND (id <= 50))
-> Seq Scan on test1_def (cost=0.00..29.05 rows=6 width=36) (actual time=0.002..0.002 rows=0 loops=1)
Filter: ((id >= 0) AND (id <= 50))
[patched]
postgres=# explain analyze select * from test1 where id between 0 and 50;
QUERY PLAN
---------------------------------------------------------------------------------------------------------
Append (cost=0.00..29.08 rows=6 width=36) (actual time=0.006..0.006 rows=0 loops=1)
-> Seq Scan on test1_1 (cost=0.00..29.05 rows=6 width=36) (actual time=0.004..0.005 rows=0 loops=1)
Filter: ((id >= 0) AND (id <= 50))
I considered about another use case. If default partition contains rows whose id = 300 and then we add another
partitionwhich have constraints like id >= 300 and id < 400, I thought we won't scan the rows anymore. But I noticed we
simplycan't add such a partition.
postgres=# insert into test1 values (300);
INSERT 0 1
postgres=# create table test1_3 partition of test1 for values from (300) to (400);
ERROR: updated partition constraint for default partition "test1_def" would be violated by some row
So I haven't come up with bad cases so far :)
--
Yoshikazu Imai
Hi
Patch work fine to me, but I have one test case where default partition still scanned.
postgres=# explain select * from test1 where (id < 10) and true;
QUERY PLAN
-------------------------------------------------------------------
Append (cost=0.00..55.98 rows=846 width=36)
-> Seq Scan on test1_1 (cost=0.00..25.88 rows=423 width=36)
Filter: (id < 10)
-> Seq Scan on test1_def (cost=0.00..25.88 rows=423 width=36)
Filter: (id < 10)
(5 rows)
Hi Yuzuko Hosoya,
Ignore my last message, I think this is also a legitimate scan on default partition.
On Mon, Mar 4, 2019 at 10:29 PM Ibrar Ahmed <ibrar.ahmad@gmail.com> wrote:
Hi
Patch work fine to me, but I have one test case where default partition still scanned.
postgres=# explain select * from test1 where (id < 10) and true;
QUERY PLAN
-------------------------------------------------------------------
Append (cost=0.00..55.98 rows=846 width=36)
-> Seq Scan on test1_1 (cost=0.00..25.88 rows=423 width=36)
Filter: (id < 10)
-> Seq Scan on test1_def (cost=0.00..25.88 rows=423 width=36)
Filter: (id < 10)
(5 rows)
Ibrar Ahmed
Imai-san, Thanks for sharing your tests! On Thu, Feb 28, 2019 at 5:27 PM Imai, Yoshikazu <imai.yoshikazu@jp.fujitsu.com> wrote: > > Hosoya-san > > On Wed, Feb 27, 2019 at 6:51 AM, Yuzuko Hosoya wrote: > > > From: Amit Langote [mailto:Langote_Amit_f8@lab.ntt.co.jp] > > > Sent: Wednesday, February 27, 2019 11:22 AM > > > > > > Hosoya-san, > > > > > > On 2019/02/22 17:14, Yuzuko Hosoya wrote: > > > > Hi, > > > > > > > > I found the bug of default partition pruning when executing a range > > query. > > > > > > > > ----- > > > > postgres=# create table test1(id int, val text) partition by range > > > > (id); postgres=# create table test1_1 partition of test1 for values > > > > from (0) to (100); postgres=# create table test1_2 partition of > > > > test1 for values from (150) to (200); postgres=# create table > > > > test1_def partition of test1 default; > > > > > > > > postgres=# explain select * from test1 where id > 0 and id < 30; > > > > QUERY PLAN > > > > ---------------------------------------------------------------- > > > > Append (cost=0.00..11.83 rows=59 width=11) > > > > -> Seq Scan on test1_1 (cost=0.00..5.00 rows=58 width=11) > > > > Filter: ((id > 0) AND (id < 30)) > > > > -> Seq Scan on test1_def (cost=0.00..6.53 rows=1 width=12) > > > > Filter: ((id > 0) AND (id < 30)) > > > > (5 rows) > > > > > > > > There is no need to scan the default partition, but it's scanned. > > > > ----- > > > > > > > > In the current implement, whether the default partition is scanned > > > > or not is determined according to each condition of given WHERE > > > > clause at get_matching_range_bounds(). In this example, > > > > scan_default is set true according to id > 0 because id >= 200 > > > > matches the default partition. Similarly, according to id < 30, > > scan_default is set true. > > > > Then, these results are combined according to AND/OR at > > perform_pruning_combine_step(). > > > > In this case, final result's scan_default is set true. > > > > > > > > The modifications I made are as follows: > > > > - get_matching_range_bounds() determines only offsets of range bounds > > > > according to each condition > > > > - These results are combined at perform_pruning_combine_step() > > > > - Whether the default partition is scanned or not is determined at > > > > get_matching_partitions() > > > > > > > > Attached the patch. Any feedback is greatly appreciated. > > > > > > Thank you for reporting. Can you please add this to March CF in Bugs > > > category so as not to lose > > track > > > of this? > > > > > > I will try to send review comments soon. > > > > > Thank you for your reply. I added this to March CF. > > I tested with simple use case and I confirmed it works correctly like below. > > In case using between clause: > postgres=# create table test1(id int, val text) partition by range (id); > postgres=# create table test1_1 partition of test1 for values from (0) to (100); > postgres=# create table test1_2 partition of test1 for values from (150) to (200); > postgres=# create table test1_def partition of test1 default; > > [HEAD] > postgres=# explain analyze select * from test1 where id between 0 and 50; > QUERY PLAN > ----------------------------------------------------------------------------------------------------------- > Append (cost=0.00..58.16 rows=12 width=36) (actual time=0.008..0.008 rows=0 loops=1) > -> Seq Scan on test1_1 (cost=0.00..29.05 rows=6 width=36) (actual time=0.005..0.005 rows=0 loops=1) > Filter: ((id >= 0) AND (id <= 50)) > -> Seq Scan on test1_def (cost=0.00..29.05 rows=6 width=36) (actual time=0.002..0.002 rows=0 loops=1) > Filter: ((id >= 0) AND (id <= 50)) > > > [patched] > postgres=# explain analyze select * from test1 where id between 0 and 50; > QUERY PLAN > --------------------------------------------------------------------------------------------------------- > Append (cost=0.00..29.08 rows=6 width=36) (actual time=0.006..0.006 rows=0 loops=1) > -> Seq Scan on test1_1 (cost=0.00..29.05 rows=6 width=36) (actual time=0.004..0.005 rows=0 loops=1) > Filter: ((id >= 0) AND (id <= 50)) > > > > I considered about another use case. If default partition contains rows whose id = 300 and then we add another partitionwhich have constraints like id >= 300 and id < 400, I thought we won't scan the rows anymore. But I noticed we simplycan't add such a partition. > > postgres=# insert into test1 values (300); > INSERT 0 1 > postgres=# create table test1_3 partition of test1 for values from (300) to (400); > ERROR: updated partition constraint for default partition "test1_def" would be violated by some row > > > So I haven't come up with bad cases so far :) I didn't test cases you mentioned. Thanks to you, I could check correctness of the patch! -- Best regards, Yuzuko Hosoya NTT Open Source Software Center
Hi Ibrar, On Tue, Mar 5, 2019 at 2:37 AM Ibrar Ahmed <ibrar.ahmad@gmail.com> wrote: > > Hi Yuzuko Hosoya, > > Ignore my last message, I think this is also a legitimate scan on default partition. > Oh, I got it. Thanks a lot. > > On Mon, Mar 4, 2019 at 10:29 PM Ibrar Ahmed <ibrar.ahmad@gmail.com> wrote: >> >> Hi >> >> Patch work fine to me, but I have one test case where default partition still scanned. >> >> postgres=# explain select * from test1 where (id < 10) and true; >> QUERY PLAN >> ------------------------------------------------------------------- >> Append (cost=0.00..55.98 rows=846 width=36) >> -> Seq Scan on test1_1 (cost=0.00..25.88 rows=423 width=36) >> Filter: (id < 10) >> -> Seq Scan on test1_def (cost=0.00..25.88 rows=423 width=36) >> Filter: (id < 10) >> (5 rows) > > > > -- > Ibrar Ahmed -- Best regards, Yuzuko Hosoya NTT Open Source Software Center
Le 28/02/2019 à 09:26, Imai, Yoshikazu a écrit : > Hosoya-san > > On Wed, Feb 27, 2019 at 6:51 AM, Yuzuko Hosoya wrote: >>> From: Amit Langote [mailto:Langote_Amit_f8@lab.ntt.co.jp] >>> Sent: Wednesday, February 27, 2019 11:22 AM >>> >>> Hosoya-san, >>> >>> On 2019/02/22 17:14, Yuzuko Hosoya wrote: >>>> Hi, >>>> >>>> I found the bug of default partition pruning when executing a range >> query. >>>> ----- >>>> postgres=# create table test1(id int, val text) partition by range >>>> (id); postgres=# create table test1_1 partition of test1 for values >>>> from (0) to (100); postgres=# create table test1_2 partition of >>>> test1 for values from (150) to (200); postgres=# create table >>>> test1_def partition of test1 default; >>>> >>>> postgres=# explain select * from test1 where id > 0 and id < 30; >>>> QUERY PLAN >>>> ---------------------------------------------------------------- >>>> Append (cost=0.00..11.83 rows=59 width=11) >>>> -> Seq Scan on test1_1 (cost=0.00..5.00 rows=58 width=11) >>>> Filter: ((id > 0) AND (id < 30)) >>>> -> Seq Scan on test1_def (cost=0.00..6.53 rows=1 width=12) >>>> Filter: ((id > 0) AND (id < 30)) >>>> (5 rows) >>>> >>>> There is no need to scan the default partition, but it's scanned. >>>> ----- >>>> >>>> In the current implement, whether the default partition is scanned >>>> or not is determined according to each condition of given WHERE >>>> clause at get_matching_range_bounds(). In this example, >>>> scan_default is set true according to id > 0 because id >= 200 >>>> matches the default partition. Similarly, according to id < 30, >> scan_default is set true. >>>> Then, these results are combined according to AND/OR at >> perform_pruning_combine_step(). >>>> In this case, final result's scan_default is set true. >>>> >>>> The modifications I made are as follows: >>>> - get_matching_range_bounds() determines only offsets of range bounds >>>> according to each condition >>>> - These results are combined at perform_pruning_combine_step() >>>> - Whether the default partition is scanned or not is determined at >>>> get_matching_partitions() >>>> >>>> Attached the patch. Any feedback is greatly appreciated. >>> Thank you for reporting. Can you please add this to March CF in Bugs >>> category so as not to lose >> track >>> of this? >>> >>> I will try to send review comments soon. >>> >> Thank you for your reply. I added this to March CF. > I tested with simple use case and I confirmed it works correctly like below. > > In case using between clause: > postgres=# create table test1(id int, val text) partition by range (id); > postgres=# create table test1_1 partition of test1 for values from (0) to (100); > postgres=# create table test1_2 partition of test1 for values from (150) to (200); > postgres=# create table test1_def partition of test1 default; > > [HEAD] > postgres=# explain analyze select * from test1 where id between 0 and 50; > QUERY PLAN > ----------------------------------------------------------------------------------------------------------- > Append (cost=0.00..58.16 rows=12 width=36) (actual time=0.008..0.008 rows=0 loops=1) > -> Seq Scan on test1_1 (cost=0.00..29.05 rows=6 width=36) (actual time=0.005..0.005 rows=0 loops=1) > Filter: ((id >= 0) AND (id <= 50)) > -> Seq Scan on test1_def (cost=0.00..29.05 rows=6 width=36) (actual time=0.002..0.002 rows=0 loops=1) > Filter: ((id >= 0) AND (id <= 50)) > > > [patched] > postgres=# explain analyze select * from test1 where id between 0 and 50; > QUERY PLAN > --------------------------------------------------------------------------------------------------------- > Append (cost=0.00..29.08 rows=6 width=36) (actual time=0.006..0.006 rows=0 loops=1) > -> Seq Scan on test1_1 (cost=0.00..29.05 rows=6 width=36) (actual time=0.004..0.005 rows=0 loops=1) > Filter: ((id >= 0) AND (id <= 50)) > > > > I considered about another use case. If default partition contains rows whose id = 300 and then we add another partitionwhich have constraints like id >= 300 and id < 400, I thought we won't scan the rows anymore. But I noticed we simplycan't add such a partition. > > postgres=# insert into test1 values (300); > INSERT 0 1 > postgres=# create table test1_3 partition of test1 for values from (300) to (400); > ERROR: updated partition constraint for default partition "test1_def" would be violated by some row > > > So I haven't come up with bad cases so far :) > > -- > Yoshikazu Imai Hello Yoshikazu-San, I tested your patch using some sub-partitions and found a possible problem. I create a new partitioned partition test1_3 with 2 sub-partitions : ------------------------- create table test1_3 partition of test1 for values from (200) to (400) partition by range (id); create table test1_3_1 partition of test1_3 for values from (200) to (250); create table test1_3_2 partition of test1_3 for values from (250) to (350); # explain select * from test1 where (id > 0 and id < 30); QUERY PLAN --------------------------------------------------------------- Append (cost=0.00..29.08 rows=6 width=36) -> Seq Scan on test1_1 (cost=0.00..29.05 rows=6 width=36) Filter: ((id > 0) AND (id < 30)) (3 rows) # explain select * from test1 where (id > 220 and id < 230); QUERY PLAN ----------------------------------------------------------------- Append (cost=0.00..29.08 rows=6 width=36) -> Seq Scan on test1_3_1 (cost=0.00..29.05 rows=6 width=36) Filter: ((id > 220) AND (id < 230)) (3 rows) # explain select * from test1 where (id > 0 and id < 30) or (id > 220 and id < 230); QUERY PLAN --------------------------------------------------------------------------- Append (cost=0.00..106.40 rows=39 width=36) -> Seq Scan on test1_1 (cost=0.00..35.40 rows=13 width=36) Filter: (((id > 0) AND (id < 30)) OR ((id > 220) AND (id < 230))) -> Seq Scan on test1_3_1 (cost=0.00..35.40 rows=13 width=36) Filter: (((id > 0) AND (id < 30)) OR ((id > 220) AND (id < 230))) -> Seq Scan on test1_3_2 (cost=0.00..35.40 rows=13 width=36) Filter: (((id > 0) AND (id < 30)) OR ((id > 220) AND (id < 230))) (7 rows) ----------------- Partition pruning is functioning when only the sub-partition is required. When both the partition and the sub-partition is required, there is no pruning on the sub-partition. Cordialement, -- Thibaut Madelaine Dalibo
Hi Thibaut,
Thanks a lot for your test and comments.
>
> Le 28/02/2019 à 09:26, Imai, Yoshikazu a écrit :
> > Hosoya-san
> >
> > On Wed, Feb 27, 2019 at 6:51 AM, Yuzuko Hosoya wrote:
> >>> From: Amit Langote [mailto:Langote_Amit_f8@lab.ntt.co.jp]
> >>> Sent: Wednesday, February 27, 2019 11:22 AM
> >>>
> >>> Hosoya-san,
> >>>
> >>> On 2019/02/22 17:14, Yuzuko Hosoya wrote:
> >>>> Hi,
> >>>>
> >>>> I found the bug of default partition pruning when executing a range
> >> query.
> >>>> -----
> >>>> postgres=# create table test1(id int, val text) partition by range
> >>>> (id); postgres=# create table test1_1 partition of test1 for values
> >>>> from (0) to (100); postgres=# create table test1_2 partition of
> >>>> test1 for values from (150) to (200); postgres=# create table
> >>>> test1_def partition of test1 default;
> >>>>
> >>>> postgres=# explain select * from test1 where id > 0 and id < 30;
> >>>> QUERY PLAN
> >>>> ----------------------------------------------------------------
> >>>> Append (cost=0.00..11.83 rows=59 width=11)
> >>>> -> Seq Scan on test1_1 (cost=0.00..5.00 rows=58 width=11)
> >>>> Filter: ((id > 0) AND (id < 30))
> >>>> -> Seq Scan on test1_def (cost=0.00..6.53 rows=1 width=12)
> >>>> Filter: ((id > 0) AND (id < 30))
> >>>> (5 rows)
> >>>>
> >>>> There is no need to scan the default partition, but it's scanned.
> >>>> -----
> >>>>
> >>>> In the current implement, whether the default partition is scanned
> >>>> or not is determined according to each condition of given WHERE
> >>>> clause at get_matching_range_bounds(). In this example,
> >>>> scan_default is set true according to id > 0 because id >= 200
> >>>> matches the default partition. Similarly, according to id < 30,
> >> scan_default is set true.
> >>>> Then, these results are combined according to AND/OR at
> >> perform_pruning_combine_step().
> >>>> In this case, final result's scan_default is set true.
> >>>>
> >>>> The modifications I made are as follows:
> >>>> - get_matching_range_bounds() determines only offsets of range bounds
> >>>> according to each condition
> >>>> - These results are combined at perform_pruning_combine_step()
> >>>> - Whether the default partition is scanned or not is determined at
> >>>> get_matching_partitions()
> >>>>
> >>>> Attached the patch. Any feedback is greatly appreciated.
> >>> Thank you for reporting. Can you please add this to March CF in
> >>> Bugs category so as not to lose
> >> track
> >>> of this?
> >>>
> >>> I will try to send review comments soon.
> >>>
> >> Thank you for your reply. I added this to March CF.
> > I tested with simple use case and I confirmed it works correctly like below.
> >
> > In case using between clause:
> > postgres=# create table test1(id int, val text) partition by range
> > (id); postgres=# create table test1_1 partition of test1 for values
> > from (0) to (100); postgres=# create table test1_2 partition of test1
> > for values from (150) to (200); postgres=# create table test1_def
> > partition of test1 default;
> >
> > [HEAD]
> > postgres=# explain analyze select * from test1 where id between 0 and 50;
> > QUERY PLAN
> > ----------------------------------------------------------------------
> > -------------------------------------
> > Append (cost=0.00..58.16 rows=12 width=36) (actual time=0.008..0.008 rows=0 loops=1)
> > -> Seq Scan on test1_1 (cost=0.00..29.05 rows=6 width=36) (actual time=0.005..0.005
> rows=0 loops=1)
> > Filter: ((id >= 0) AND (id <= 50))
> > -> Seq Scan on test1_def (cost=0.00..29.05 rows=6 width=36) (actual
> time=0.002..0.002 rows=0 loops=1)
> > Filter: ((id >= 0) AND (id <= 50))
> >
> >
> > [patched]
> > postgres=# explain analyze select * from test1 where id between 0 and 50;
> > QUERY PLAN
> > ----------------------------------------------------------------------
> > -----------------------------------
> > Append (cost=0.00..29.08 rows=6 width=36) (actual time=0.006..0.006 rows=0 loops=1)
> > -> Seq Scan on test1_1 (cost=0.00..29.05 rows=6 width=36) (actual time=0.004..0.005
> rows=0 loops=1)
> > Filter: ((id >= 0) AND (id <= 50))
> >
> >
> >
> > I considered about another use case. If default partition contains rows whose id = 300
> and then we add another partition which have constraints like id >= 300 and id < 400, I thought
> we won't scan the rows anymore. But I noticed we simply can't add such a partition.
> >
> > postgres=# insert into test1 values (300); INSERT 0 1 postgres=#
> > create table test1_3 partition of test1 for values from (300) to
> > (400);
> > ERROR: updated partition constraint for default partition "test1_def"
> > would be violated by some row
> >
> >
> > So I haven't come up with bad cases so far :)
> >
> > --
> > Yoshikazu Imai
>
> Hello Yoshikazu-San,
>
> I tested your patch using some sub-partitions and found a possible problem.
>
> I create a new partitioned partition test1_3 with 2 sub-partitions :
>
> -------------------------
>
> create table test1_3 partition of test1 for values from (200) to (400) partition by range
> (id); create table test1_3_1 partition of test1_3 for values from (200) to (250); create
> table test1_3_2 partition of test1_3 for values from (250) to (350);
>
> # explain select * from test1 where (id > 0 and id < 30);
> QUERY PLAN
> ---------------------------------------------------------------
> Append (cost=0.00..29.08 rows=6 width=36)
> -> Seq Scan on test1_1 (cost=0.00..29.05 rows=6 width=36)
> Filter: ((id > 0) AND (id < 30))
> (3 rows)
>
> # explain select * from test1 where (id > 220 and id < 230);
> QUERY PLAN
> -----------------------------------------------------------------
> Append (cost=0.00..29.08 rows=6 width=36)
> -> Seq Scan on test1_3_1 (cost=0.00..29.05 rows=6 width=36)
> Filter: ((id > 220) AND (id < 230))
> (3 rows)
>
> # explain select * from test1
> where (id > 0 and id < 30) or (id > 220 and id < 230);
> QUERY PLAN
> ---------------------------------------------------------------------------
> Append (cost=0.00..106.40 rows=39 width=36)
> -> Seq Scan on test1_1 (cost=0.00..35.40 rows=13 width=36)
> Filter: (((id > 0) AND (id < 30)) OR ((id > 220) AND (id < 230)))
> -> Seq Scan on test1_3_1 (cost=0.00..35.40 rows=13 width=36)
> Filter: (((id > 0) AND (id < 30)) OR ((id > 220) AND (id < 230)))
> -> Seq Scan on test1_3_2 (cost=0.00..35.40 rows=13 width=36)
> Filter: (((id > 0) AND (id < 30)) OR ((id > 220) AND (id < 230)))
> (7 rows)
>
> -----------------
>
> Partition pruning is functioning when only the sub-partition is required. When both the
> partition and the sub-partition is required, there is no pruning on the sub-partition.
>
Indeed, it's problematic. I also did test and I found that
this problem was occurred when any partition didn't match
WHERE clauses. So following query didn't work correctly.
# explain select * from test1_3 where (id > 0 and id < 30);
QUERY PLAN
-----------------------------------------------------------------
Append (cost=0.00..58.16 rows=12 width=36)
-> Seq Scan on test1_3_1 (cost=0.00..29.05 rows=6 width=36)
Filter: ((id > 0) AND (id < 30))
-> Seq Scan on test1_3_2 (cost=0.00..29.05 rows=6 width=36)
Filter: ((id > 0) AND (id < 30))
(5 rows)
I created a new patch to handle this problem, and confirmed
the query you mentioned works as expected
# explain select * from test1 where (id > 0 and id < 30) or (id > 220 and id < 230);
QUERY PLAN
---------------------------------------------------------------------------
Append (cost=0.00..70.93 rows=26 width=36)
-> Seq Scan on test1_1_1 (cost=0.00..35.40 rows=13 width=36)
Filter: (((id > 0) AND (id < 30)) OR ((id > 220) AND (id < 230)))
-> Seq Scan on test1_3_1 (cost=0.00..35.40 rows=13 width=36)
Filter: (((id > 0) AND (id < 30)) OR ((id > 220) AND (id < 230)))
(5 rows)
v2 patch attached.
Could you please check it again?
--
Best regards,
Yuzuko Hosoya
Attachment
Hello. At Fri, 15 Mar 2019 15:05:41 +0900, "Yuzuko Hosoya" <hosoya.yuzuko@lab.ntt.co.jp> wrote in <001901d4daf5$1ef4f640$5cdee2c0$@lab.ntt.co.jp> > v2 patch attached. > Could you please check it again? I have some comments on the patch itself. The patch relies on the fact(?) that the lowest index is always -1 in range partition and uses it as pseudo default partition. I'm not sure it is really the fact and anyway it donsn't seem the right thing to do. Could you explain how it works, not what you did in this patch? L96: > /* There can only be zero or one matching partition. */ > - if (partindices[off + 1] >= 0) > - result->bound_offsets = bms_make_singleton(off + 1); > - else > - result->scan_default = > - partition_bound_has_default(boundinfo); > + result->bound_offsets = bms_make_singleton(off + 1); The comment had a meaning for the old code. Seems to need rewrite? L183: > + /* > + * All bounds are greater than the key, so we could only > + * expect to find the lookup key in the default partition. > + */ Long trailing spaces are attached to every line without substantial modification. L198: > - * inclusive, no need add the adjacent partition. > + * inclusive, no need add the adjacent partition. If 'off' is > + * -1 indicating that all bounds are greater, then we simply > + * end up adding the first bound's offset, that is, 0. off doesn't seem to be -1 there. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Hosoya-san, On 2019/03/15 15:05, Yuzuko Hosoya wrote: > Indeed, it's problematic. I also did test and I found that > this problem was occurred when any partition didn't match > WHERE clauses. So following query didn't work correctly. > > # explain select * from test1_3 where (id > 0 and id < 30); > QUERY PLAN > ----------------------------------------------------------------- > Append (cost=0.00..58.16 rows=12 width=36) > -> Seq Scan on test1_3_1 (cost=0.00..29.05 rows=6 width=36) > Filter: ((id > 0) AND (id < 30)) > -> Seq Scan on test1_3_2 (cost=0.00..29.05 rows=6 width=36) > Filter: ((id > 0) AND (id < 30)) > (5 rows) > > I created a new patch to handle this problem, and confirmed > the query you mentioned works as expected > > # explain select * from test1 where (id > 0 and id < 30) or (id > 220 and id < 230); > QUERY PLAN > --------------------------------------------------------------------------- > Append (cost=0.00..70.93 rows=26 width=36) > -> Seq Scan on test1_1_1 (cost=0.00..35.40 rows=13 width=36) > Filter: (((id > 0) AND (id < 30)) OR ((id > 220) AND (id < 230))) > -> Seq Scan on test1_3_1 (cost=0.00..35.40 rows=13 width=36) > Filter: (((id > 0) AND (id < 30)) OR ((id > 220) AND (id < 230))) > (5 rows) > > v2 patch attached. > Could you please check it again? I think the updated patch breaks the promise that get_matching_range_bounds won't set scan_default based on individual pruning value comparisons. How about the attached delta patch that applies on top of your earlier v1 patch, which fixes the issue reported by Thibaut? Thanks, Amit
Attachment
Hello. At Fri, 15 Mar 2019 17:30:07 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20190315.173007.147577546.horiguchi.kyotaro@lab.ntt.co.jp> > The patch relies on the fact(?) that the lowest index is always > -1 in range partition and uses it as pseudo default > partition. I'm not sure it is really the fact and anyway it > donsn't seem the right thing to do. Could you explain how it > works, not what you did in this patch? I understood how it works but still uneasy that only list partitioning requires scan_default. Anyway please ignore this. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Hi.
At Mon, 18 Mar 2019 18:44:07 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in
<9bed6b79-f264-6976-b880-e2a5d23e9d85@lab.ntt.co.jp>
> > v2 patch attached.
> > Could you please check it again?
>
> I think the updated patch breaks the promise that
> get_matching_range_bounds won't set scan_default based on individual
> pruning value comparisons. How about the attached delta patch that
> applies on top of your earlier v1 patch, which fixes the issue reported by
> Thibaut?
I read through the patch and understood how it works. And Amit's
proposal looks fine.
But that makes me think of scan_default as a wart.
The attached patch is a refactoring that removes scan_default
from PruneStepResult and the defaut partition is represented as
the same way as non-default partitions, without changing in
behavior. This improves the modularity of partprune code a bit.
The fix would be put on top of this easily.
Thoughts?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index 5b897d50ee..828240119d 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -92,7 +92,6 @@ static int partition_range_bsearch(int partnatts, FmgrInfo *partsupfunc,
Oid *partcollation,
PartitionBoundInfo boundinfo,
PartitionRangeBound *probe, bool *is_equal);
-static int get_partition_bound_num_indexes(PartitionBoundInfo b);
static Expr *make_partition_op_expr(PartitionKey key, int keynum,
uint16 strategy, Expr *arg1, Expr *arg2);
static Oid get_partition_operator(PartitionKey key, int col,
@@ -266,6 +265,7 @@ create_hash_bounds(PartitionBoundSpec **boundspecs, int nparts,
boundinfo->ndatums = ndatums;
boundinfo->datums = (Datum **) palloc0(ndatums * sizeof(Datum *));
+ boundinfo->nindexes = greatest_modulus;
boundinfo->indexes = (int *) palloc(greatest_modulus * sizeof(int));
for (i = 0; i < greatest_modulus; i++)
boundinfo->indexes[i] = -1;
@@ -399,7 +399,10 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
boundinfo->ndatums = ndatums;
boundinfo->datums = (Datum **) palloc0(ndatums * sizeof(Datum *));
- boundinfo->indexes = (int *) palloc(ndatums * sizeof(int));
+
+ /* the last element is reserved for the default partition */
+ boundinfo->nindexes = ndatums + 1;
+ boundinfo->indexes = (int *) palloc(boundinfo->nindexes * sizeof(int));
/*
* Copy values. Canonical indexes are values ranging from 0 to (nparts -
@@ -423,6 +426,9 @@ create_list_bounds(PartitionBoundSpec **boundspecs, int nparts,
boundinfo->indexes[i] = (*mapping)[orig_index];
}
+ /* set default partition index (-1) */
+ boundinfo->indexes[ndatums] = -1;
+
/*
* Set the canonical value for null_index, if any.
*
@@ -596,7 +602,8 @@ create_range_bounds(PartitionBoundSpec **boundspecs, int nparts,
* For range partitioning, an additional value of -1 is stored as the last
* element.
*/
- boundinfo->indexes = (int *) palloc((ndatums + 1) * sizeof(int));
+ boundinfo->nindexes = ndatums + 1;
+ boundinfo->indexes = (int *) palloc(boundinfo->nindexes * sizeof(int));
for (i = 0; i < ndatums; i++)
{
@@ -676,6 +683,9 @@ partition_bounds_equal(int partnatts, int16 *parttyplen, bool *parttypbyval,
if (b1->ndatums != b2->ndatums)
return false;
+ if (b1->nindexes != b2->nindexes)
+ return false;
+
if (b1->null_index != b2->null_index)
return false;
@@ -704,7 +714,7 @@ partition_bounds_equal(int partnatts, int16 *parttyplen, bool *parttypbyval,
* their indexes array will be same. So, it suffices to compare
* indexes array.
*/
- for (i = 0; i < greatest_modulus; i++)
+ for (i = 0; i < b1->nindexes; i++)
if (b1->indexes[i] != b2->indexes[i])
return false;
@@ -765,9 +775,9 @@ partition_bounds_equal(int partnatts, int16 *parttyplen, bool *parttypbyval,
return false;
}
- /* There are ndatums+1 indexes in case of range partitions */
- if (b1->strategy == PARTITION_STRATEGY_RANGE &&
- b1->indexes[i] != b2->indexes[i])
+ /* There may be ndatums+1 indexes in some cases */
+ Assert (i == b1->nindexes || i == b1->nindexes - 1);
+ if (i < b1->nindexes && b1->indexes[i] != b2->indexes[i])
return false;
}
return true;
@@ -793,7 +803,7 @@ partition_bounds_copy(PartitionBoundInfo src,
ndatums = dest->ndatums = src->ndatums;
partnatts = key->partnatts;
- num_indexes = get_partition_bound_num_indexes(src);
+ num_indexes = dest->nindexes = src->nindexes;
/* List partitioned tables have only a single partition key. */
Assert(key->strategy != PARTITION_STRATEGY_LIST || partnatts == 1);
@@ -1710,46 +1720,6 @@ qsort_partition_rbound_cmp(const void *a, const void *b, void *arg)
b1->lower, b2);
}
-/*
- * get_partition_bound_num_indexes
- *
- * Returns the number of the entries in the partition bound indexes array.
- */
-static int
-get_partition_bound_num_indexes(PartitionBoundInfo bound)
-{
- int num_indexes;
-
- Assert(bound);
-
- switch (bound->strategy)
- {
- case PARTITION_STRATEGY_HASH:
-
- /*
- * The number of the entries in the indexes array is same as the
- * greatest modulus.
- */
- num_indexes = get_hash_partition_greatest_modulus(bound);
- break;
-
- case PARTITION_STRATEGY_LIST:
- num_indexes = bound->ndatums;
- break;
-
- case PARTITION_STRATEGY_RANGE:
- /* Range partitioned table has an extra index. */
- num_indexes = bound->ndatums + 1;
- break;
-
- default:
- elog(ERROR, "unexpected partition strategy: %d",
- (int) bound->strategy);
- }
-
- return num_indexes;
-}
-
/*
* get_partition_operator
*
diff --git a/src/backend/partitioning/partprune.c b/src/backend/partitioning/partprune.c
index 95a60183a1..3cc9c9f5b8 100644
--- a/src/backend/partitioning/partprune.c
+++ b/src/backend/partitioning/partprune.c
@@ -105,7 +105,6 @@ typedef struct PruneStepResult
*/
Bitmapset *bound_offsets;
- bool scan_default; /* Scan the default partition? */
bool scan_null; /* Scan the partition for NULL values? */
} PruneStepResult;
@@ -671,23 +670,20 @@ get_matching_partitions(PartitionPruneContext *context, List *pruning_steps)
Assert(final_result != NULL);
i = -1;
result = NULL;
- scan_default = final_result->scan_default;
+ scan_default = false;
while ((i = bms_next_member(final_result->bound_offsets, i)) >= 0)
{
int partindex = context->boundinfo->indexes[i];
/*
- * In range and hash partitioning cases, some index values may be -1,
- * indicating that no partition has been defined to accept a given
- * range of values (that is, the bound at given offset is the upper
- * bound of this unassigned range of values) or for a given remainder,
- * respectively. As it's still part of the queried range of values,
- * add the default partition, if any.
+ * Some index slot may contain -1, indicating that no partition has
+ * been defined to accept a given range of values. As it's still part
+ * of the queried range of values, add the default partition, if any.
*/
if (partindex >= 0)
result = bms_add_member(result, partindex);
- else
- scan_default |= partition_bound_has_default(context->boundinfo);
+ else if (partition_bound_has_default(context->boundinfo))
+ scan_default = true;
}
/* Add the null and/or default partition if needed and if present. */
@@ -2202,7 +2198,7 @@ get_matching_hash_bounds(PartitionPruneContext *context,
* There is neither a special hash null partition or the default hash
* partition.
*/
- result->scan_null = result->scan_default = false;
+ result->scan_null = false;
return result;
}
@@ -2212,10 +2208,6 @@ get_matching_hash_bounds(PartitionPruneContext *context,
* Determine the offsets of list bounds matching the specified value,
* according to the semantics of the given operator strategy
*
- * scan_default will be set in the returned struct, if the default partition
- * needs to be scanned, provided one exists at all. scan_null will be set if
- * the special null-accepting partition needs to be scanned.
- *
* 'opstrategy' if non-zero must be a btree strategy number.
*
* 'value' contains the value to use for pruning.
@@ -2244,7 +2236,7 @@ get_matching_list_bounds(PartitionPruneContext *context,
Assert(context->strategy == PARTITION_STRATEGY_LIST);
Assert(context->partnatts == 1);
- result->scan_null = result->scan_default = false;
+ result->scan_null = false;
if (!bms_is_empty(nullkeys))
{
@@ -2256,7 +2248,8 @@ get_matching_list_bounds(PartitionPruneContext *context,
if (partition_bound_accepts_nulls(boundinfo))
result->scan_null = true;
else
- result->scan_default = partition_bound_has_default(boundinfo);
+ /* scan the default partition, if any */
+ result->bound_offsets = bms_make_singleton(boundinfo->ndatums);
return result;
}
@@ -2266,7 +2259,7 @@ get_matching_list_bounds(PartitionPruneContext *context,
*/
if (boundinfo->ndatums == 0)
{
- result->scan_default = partition_bound_has_default(boundinfo);
+ result->bound_offsets = bms_make_singleton(boundinfo->ndatums);
return result;
}
@@ -2280,10 +2273,9 @@ get_matching_list_bounds(PartitionPruneContext *context,
*/
if (nvalues == 0)
{
- Assert(boundinfo->ndatums > 0);
- result->bound_offsets = bms_add_range(NULL, 0,
- boundinfo->ndatums - 1);
- result->scan_default = partition_bound_has_default(boundinfo);
+ Assert(boundinfo->nindexes > 0);
+ result->bound_offsets = bms_add_range(result->bound_offsets,
+ 0, boundinfo->nindexes - 1);
return result;
}
@@ -2294,8 +2286,8 @@ get_matching_list_bounds(PartitionPruneContext *context,
* First match to all bounds. We'll remove any matching datums below.
*/
Assert(boundinfo->ndatums > 0);
- result->bound_offsets = bms_add_range(NULL, 0,
- boundinfo->ndatums - 1);
+ result->bound_offsets = bms_add_range(result->bound_offsets,
+ 0, boundinfo->ndatums);
off = partition_list_bsearch(partsupfunc, partcollation, boundinfo,
value, &is_equal);
@@ -2308,23 +2300,9 @@ get_matching_list_bounds(PartitionPruneContext *context,
off);
}
- /* Always include the default partition if any. */
- result->scan_default = partition_bound_has_default(boundinfo);
-
return result;
}
- /*
- * With range queries, always include the default list partition, because
- * list partitions divide the key space in a discontinuous manner, not all
- * values in the given range will have a partition assigned. This may not
- * technically be true for some data types (e.g. integer types), however,
- * we currently lack any sort of infrastructure to provide us with proofs
- * that would allow us to do anything smarter here.
- */
- if (opstrategy != BTEqualStrategyNumber)
- result->scan_default = partition_bound_has_default(boundinfo);
-
switch (opstrategy)
{
case BTEqualStrategyNumber:
@@ -2338,7 +2316,9 @@ get_matching_list_bounds(PartitionPruneContext *context,
result->bound_offsets = bms_make_singleton(off);
}
else
- result->scan_default = partition_bound_has_default(boundinfo);
+ /* scan the default partition, if any */
+ result->bound_offsets = bms_make_singleton(boundinfo->ndatums);
+
return result;
case BTGreaterEqualStrategyNumber:
@@ -2367,11 +2347,14 @@ get_matching_list_bounds(PartitionPruneContext *context,
/*
* off is greater than the numbers of datums we have partitions
* for. The only possible partition that could contain a match is
- * the default partition, but we must've set context->scan_default
- * above anyway if one exists.
+ * the default partition. Scan the default partition if one
+ * exists.
*/
if (off > boundinfo->ndatums - 1)
+ {
+ result->bound_offsets = bms_make_singleton(boundinfo->ndatums);
return result;
+ }
minoff = off;
break;
@@ -2390,11 +2373,14 @@ get_matching_list_bounds(PartitionPruneContext *context,
/*
* off is smaller than the datums of all non-default partitions.
* The only possible partition that could contain a match is the
- * default partition, but we must've set context->scan_default
- * above anyway if one exists.
+ * default partition, but we scan the default partition if one
+ * exists.
*/
if (off < 0)
+ {
+ result->bound_offsets = bms_make_singleton(boundinfo->ndatums);
return result;
+ }
maxoff = off;
break;
@@ -2404,8 +2390,21 @@ get_matching_list_bounds(PartitionPruneContext *context,
break;
}
+ /*
+ * With range queries, always include the default list partition, because
+ * list partitions divide the key space in a discontinuous manner, not all
+ * values in the given range will have a partition assigned. This may not
+ * technically be true for some data types (e.g. integer types), however,
+ * we currently lack any sort of infrastructure to provide us with proofs
+ * that would allow us to do anything smarter here.
+ */
+ Assert (opstrategy != BTEqualStrategyNumber);
+ result->bound_offsets = bms_add_member(result->bound_offsets,
+ boundinfo->ndatums);
+
Assert(minoff >= 0 && maxoff >= 0);
- result->bound_offsets = bms_add_range(NULL, minoff, maxoff);
+ result->bound_offsets = bms_add_range(result->bound_offsets,
+ minoff, maxoff);
return result;
}
@@ -2418,9 +2417,8 @@ get_matching_list_bounds(PartitionPruneContext *context,
* Each datum whose offset is in result is to be treated as the upper bound of
* the partition that will contain the desired values.
*
- * scan_default will be set in the returned struct, if the default partition
- * needs to be scanned, provided one exists at all. Although note that we
- * intentionally don't set scan_default in this function if only because the
+ * bound_offsets may contain a bit for the indexes element that contains -1,
+ * which means the default partition if any. That happens only if the
* matching set of values, found by comparing the input values using the
* specified opstrategy, contains unassigned portions of key space, which
* we normally assume to belong to the default partition. We don't infer
@@ -2461,7 +2459,7 @@ get_matching_range_bounds(PartitionPruneContext *context,
Assert(context->strategy == PARTITION_STRATEGY_RANGE);
Assert(nvalues <= partnatts);
- result->scan_null = result->scan_default = false;
+ result->scan_null = false;
/*
* If there are no datums to compare keys with, or if we got an IS NULL
@@ -2469,7 +2467,7 @@ get_matching_range_bounds(PartitionPruneContext *context,
*/
if (boundinfo->ndatums == 0 || !bms_is_empty(nullkeys))
{
- result->scan_default = partition_bound_has_default(boundinfo);
+ result->bound_offsets = bms_make_singleton(boundinfo->ndatums);
return result;
}
@@ -2489,9 +2487,12 @@ get_matching_range_bounds(PartitionPruneContext *context,
if (partindices[maxoff] < 0)
maxoff--;
- result->scan_default = partition_bound_has_default(boundinfo);
+ /* offset 0 is always corresnponds to invalid partition */
+ result->bound_offsets = bms_make_singleton(0);
+
Assert(minoff >= 0 && maxoff >= 0);
- result->bound_offsets = bms_add_range(NULL, minoff, maxoff);
+ result->bound_offsets = bms_add_range(result->bound_offsets,
+ minoff, maxoff);
return result;
}
@@ -2501,7 +2502,7 @@ get_matching_range_bounds(PartitionPruneContext *context,
* default partition, if any.
*/
if (nvalues < partnatts)
- result->scan_default = partition_bound_has_default(boundinfo);
+ result->bound_offsets = bms_make_singleton(0);
switch (opstrategy)
{
@@ -2518,7 +2519,8 @@ get_matching_range_bounds(PartitionPruneContext *context,
if (nvalues == partnatts)
{
/* There can only be zero or one matching partition. */
- result->bound_offsets = bms_make_singleton(off + 1);
+ result->bound_offsets =
+ bms_add_member(result->bound_offsets, off + 1);
return result;
}
else
@@ -2607,7 +2609,8 @@ get_matching_range_bounds(PartitionPruneContext *context,
}
Assert(minoff >= 0 && maxoff >= 0);
- result->bound_offsets = bms_add_range(NULL, minoff, maxoff);
+ result->bound_offsets = bms_add_range(result->bound_offsets,
+ minoff, maxoff);
}
else
{
@@ -2620,7 +2623,8 @@ get_matching_range_bounds(PartitionPruneContext *context,
* -1 indicating that all bounds are greater, then we simply
* end up adding the first bound's offset, that is, 0.
*/
- result->bound_offsets = bms_make_singleton(off + 1);
+ result->bound_offsets =
+ bms_add_member(result->bound_offsets, off + 1);
}
return result;
@@ -2821,8 +2825,8 @@ get_matching_range_bounds(PartitionPruneContext *context,
Assert(minoff >= 0 && maxoff >= 0);
if (minoff <= maxoff)
- result->bound_offsets = bms_add_range(NULL, minoff, maxoff);
-
+ result->bound_offsets = bms_add_range(result->bound_offsets,
+ minoff, maxoff);
return result;
}
@@ -3001,7 +3005,6 @@ perform_pruning_base_step(PartitionPruneContext *context,
result = (PruneStepResult *) palloc(sizeof(PruneStepResult));
result->bound_offsets = NULL;
- result->scan_default = false;
result->scan_null = false;
return result;
@@ -3102,17 +3105,9 @@ perform_pruning_combine_step(PartitionPruneContext *context,
{
PartitionBoundInfo boundinfo = context->boundinfo;
- /*
- * Add all valid offsets into the boundinfo->indexes array. For range
- * partitioning, boundinfo->indexes contains (boundinfo->ndatums + 1)
- * valid entries.
- */
- if (context->strategy == PARTITION_STRATEGY_RANGE)
- result->bound_offsets = bms_add_range(NULL, 0, boundinfo->ndatums);
- else
- result->bound_offsets = bms_add_range(NULL, 0,
- boundinfo->ndatums - 1);
- result->scan_default = partition_bound_has_default(boundinfo);
+ /* Add all valid offsets into the boundinfo->indexes array. */
+ result->bound_offsets = bms_add_range(NULL, 0, boundinfo->nindexes - 1);
+
result->scan_null = partition_bound_accepts_nulls(boundinfo);
return result;
}
@@ -3143,8 +3138,6 @@ perform_pruning_combine_step(PartitionPruneContext *context,
/* Update whether to scan null and default partitions. */
if (!result->scan_null)
result->scan_null = step_result->scan_null;
- if (!result->scan_default)
- result->scan_default = step_result->scan_default;
}
break;
@@ -3166,7 +3159,6 @@ perform_pruning_combine_step(PartitionPruneContext *context,
result->bound_offsets =
bms_copy(step_result->bound_offsets);
result->scan_null = step_result->scan_null;
- result->scan_default = step_result->scan_default;
firststep = false;
}
else
@@ -3179,8 +3171,6 @@ perform_pruning_combine_step(PartitionPruneContext *context,
/* Update whether to scan null and default partitions. */
if (result->scan_null)
result->scan_null = step_result->scan_null;
- if (result->scan_default)
- result->scan_default = step_result->scan_default;
}
}
break;
diff --git a/src/include/partitioning/partbounds.h b/src/include/partitioning/partbounds.h
index b1ae39ad63..18ac8cf0bb 100644
--- a/src/include/partitioning/partbounds.h
+++ b/src/include/partitioning/partbounds.h
@@ -65,6 +65,7 @@ typedef struct PartitionBoundInfoData
PartitionRangeDatumKind **kind; /* The kind of each range bound datum;
* NULL for hash and list partitioned
* tables */
+ int nindexes; /* Length of the indexes following array */
int *indexes; /* Partition indexes */
int null_index; /* Index of the null-accepting partition; -1
* if there isn't one */
Hi Amit-san, From: Amit Langote [mailto:Langote_Amit_f8@lab.ntt.co.jp] Sent: Monday, March 18, 2019 6:44 PM > Hosoya-san, > > On 2019/03/15 15:05, Yuzuko Hosoya wrote: > > Indeed, it's problematic. I also did test and I found that this > > problem was occurred when any partition didn't match WHERE clauses. > > So following query didn't work correctly. > > > > # explain select * from test1_3 where (id > 0 and id < 30); > > QUERY PLAN > > ----------------------------------------------------------------- > > Append (cost=0.00..58.16 rows=12 width=36) > > -> Seq Scan on test1_3_1 (cost=0.00..29.05 rows=6 width=36) > > Filter: ((id > 0) AND (id < 30)) > > -> Seq Scan on test1_3_2 (cost=0.00..29.05 rows=6 width=36) > > Filter: ((id > 0) AND (id < 30)) > > (5 rows) > > > > I created a new patch to handle this problem, and confirmed the query > > you mentioned works as expected > > > > # explain select * from test1 where (id > 0 and id < 30) or (id > 220 and id < 230); > > QUERY PLAN > > ---------------------------------------------------------------------- > > ----- Append (cost=0.00..70.93 rows=26 width=36) > > -> Seq Scan on test1_1_1 (cost=0.00..35.40 rows=13 width=36) > > Filter: (((id > 0) AND (id < 30)) OR ((id > 220) AND (id < 230))) > > -> Seq Scan on test1_3_1 (cost=0.00..35.40 rows=13 width=36) > > Filter: (((id > 0) AND (id < 30)) OR ((id > 220) AND (id < > > 230))) > > (5 rows) > > > > v2 patch attached. > > Could you please check it again? > > I think the updated patch breaks the promise that get_matching_range_bounds won't set scan_default > based on individual pruning value comparisons. How about the attached delta patch that applies on > top of your earlier v1 patch, which fixes the issue reported by Thibaut? > Indeed. I agreed with your proposal. Also, I confirmed your patch works correctly. Best regards, Yuzuko Hosoya
Le 19/03/2019 à 08:01, Yuzuko Hosoya a écrit : > Hi Amit-san, > > From: Amit Langote [mailto:Langote_Amit_f8@lab.ntt.co.jp] > Sent: Monday, March 18, 2019 6:44 PM > >> Hosoya-san, >> >> On 2019/03/15 15:05, Yuzuko Hosoya wrote: >>> Indeed, it's problematic. I also did test and I found that this >>> problem was occurred when any partition didn't match WHERE clauses. >>> So following query didn't work correctly. >>> >>> # explain select * from test1_3 where (id > 0 and id < 30); >>> QUERY PLAN >>> ----------------------------------------------------------------- >>> Append (cost=0.00..58.16 rows=12 width=36) >>> -> Seq Scan on test1_3_1 (cost=0.00..29.05 rows=6 width=36) >>> Filter: ((id > 0) AND (id < 30)) >>> -> Seq Scan on test1_3_2 (cost=0.00..29.05 rows=6 width=36) >>> Filter: ((id > 0) AND (id < 30)) >>> (5 rows) >>> >>> I created a new patch to handle this problem, and confirmed the query >>> you mentioned works as expected >>> >>> # explain select * from test1 where (id > 0 and id < 30) or (id > 220 and id < 230); >>> QUERY PLAN >>> ---------------------------------------------------------------------- >>> ----- Append (cost=0.00..70.93 rows=26 width=36) >>> -> Seq Scan on test1_1_1 (cost=0.00..35.40 rows=13 width=36) >>> Filter: (((id > 0) AND (id < 30)) OR ((id > 220) AND (id < 230))) >>> -> Seq Scan on test1_3_1 (cost=0.00..35.40 rows=13 width=36) >>> Filter: (((id > 0) AND (id < 30)) OR ((id > 220) AND (id < >>> 230))) >>> (5 rows) >>> >>> v2 patch attached. >>> Could you please check it again? >> I think the updated patch breaks the promise that get_matching_range_bounds won't set scan_default >> based on individual pruning value comparisons. How about the attached delta patch that applies on >> top of your earlier v1 patch, which fixes the issue reported by Thibaut? >> > Indeed. I agreed with your proposal. > Also, I confirmed your patch works correctly. > > Best regards, > Yuzuko Hosoya I kept on testing with sub-partitioning. I found a case, using 2 default partitions, where a default partition is not pruned: -------------- create table test2(id int, val text) partition by range (id); create table test2_20_plus_def partition of test2 default; create table test2_0_20 partition of test2 for values from (0) to (20) partition by range (id); create table test2_0_10 partition of test2_0_20 for values from (0) to (10); create table test2_10_20_def partition of test2_0_20 default; # explain (costs off) select * from test2 where id=5 or id=25; QUERY PLAN ----------------------------------------- Append -> Seq Scan on test2_0_10 Filter: ((id = 5) OR (id = 25)) -> Seq Scan on test2_10_20_def Filter: ((id = 5) OR (id = 25)) -> Seq Scan on test2_20_plus_def Filter: ((id = 5) OR (id = 25)) (7 rows) -------------- I have the same output using Amit's v1-delta.patch or Hosoya's v2_default_partition_pruning.patch.
Hi Thibaut, On 2019/03/19 23:58, Thibaut Madelaine wrote: > I kept on testing with sub-partitioning. Thanks. > I found a case, using 2 default partitions, where a default partition is > not pruned: > > -------------- > > create table test2(id int, val text) partition by range (id); > create table test2_20_plus_def partition of test2 default; > create table test2_0_20 partition of test2 for values from (0) to (20) > partition by range (id); > create table test2_0_10 partition of test2_0_20 for values from (0) to (10); > create table test2_10_20_def partition of test2_0_20 default; > > # explain (costs off) select * from test2 where id=5 or id=25; > QUERY PLAN > ----------------------------------------- > Append > -> Seq Scan on test2_0_10 > Filter: ((id = 5) OR (id = 25)) > -> Seq Scan on test2_10_20_def > Filter: ((id = 5) OR (id = 25)) > -> Seq Scan on test2_20_plus_def > Filter: ((id = 5) OR (id = 25)) > (7 rows) > > -------------- > > I have the same output using Amit's v1-delta.patch or Hosoya's > v2_default_partition_pruning.patch. I think I've figured what may be wrong. Partition pruning step generation code should ignore any arguments of an OR clause that won't be true for a sub-partitioned partition, given its partition constraint. In this case, id = 25 contradicts test2_0_20's partition constraint (which is, a IS NOT NULL AND a >= 0 AND a < 20), so the OR clause should really be simplified to id = 5, ignoring the id = 25 argument. Note that we remove id = 25 only for the considerations of pruning and not from the actual clause that's passed to the final plan, although it wouldn't be a bad idea to try to do that. Attached revised delta patch, which includes the fix described above. Thanks, Amit
Attachment
Le 20/03/2019 à 10:06, Amit Langote a écrit : > Hi Thibaut, > > On 2019/03/19 23:58, Thibaut Madelaine wrote: >> I kept on testing with sub-partitioning. > Thanks. > >> I found a case, using 2 default partitions, where a default partition is >> not pruned: >> >> -------------- >> >> create table test2(id int, val text) partition by range (id); >> create table test2_20_plus_def partition of test2 default; >> create table test2_0_20 partition of test2 for values from (0) to (20) >> partition by range (id); >> create table test2_0_10 partition of test2_0_20 for values from (0) to (10); >> create table test2_10_20_def partition of test2_0_20 default; >> >> # explain (costs off) select * from test2 where id=5 or id=25; >> QUERY PLAN >> ----------------------------------------- >> Append >> -> Seq Scan on test2_0_10 >> Filter: ((id = 5) OR (id = 25)) >> -> Seq Scan on test2_10_20_def >> Filter: ((id = 5) OR (id = 25)) >> -> Seq Scan on test2_20_plus_def >> Filter: ((id = 5) OR (id = 25)) >> (7 rows) >> >> -------------- >> >> I have the same output using Amit's v1-delta.patch or Hosoya's >> v2_default_partition_pruning.patch. > I think I've figured what may be wrong. > > Partition pruning step generation code should ignore any arguments of an > OR clause that won't be true for a sub-partitioned partition, given its > partition constraint. > > In this case, id = 25 contradicts test2_0_20's partition constraint (which > is, a IS NOT NULL AND a >= 0 AND a < 20), so the OR clause should really > be simplified to id = 5, ignoring the id = 25 argument. Note that we > remove id = 25 only for the considerations of pruning and not from the > actual clause that's passed to the final plan, although it wouldn't be a > bad idea to try to do that. > > Attached revised delta patch, which includes the fix described above. > > Thanks, > Amit Amit, I tested many cases with nested range sub-partitions... and I did not find any problem with your last patch :-) I tried mixing with hash partitions with no problems. From the patch, there seems to be less checks than before. I cannot think of a case that can have performance impacts. Hosoya-san, if you agree with Amit's proposal, do you think you can send a patch unifying your default_partition_pruning.patch and Amit's second v1-delta.patch? Cordialement, Thibaut
Hi,
Thanks a lot for additional tests and the new patch.
> Le 20/03/2019 à 10:06, Amit Langote a écrit :
> > Hi Thibaut,
> >
> > On 2019/03/19 23:58, Thibaut Madelaine wrote:
> >> I kept on testing with sub-partitioning.
> > Thanks.
> >
> >> I found a case, using 2 default partitions, where a default partition
> >> is not pruned:
> >>
> >> --------------
> >>
> >> create table test2(id int, val text) partition by range (id); create
> >> table test2_20_plus_def partition of test2 default; create table
> >> test2_0_20 partition of test2 for values from (0) to (20)
> >> partition by range (id);
> >> create table test2_0_10 partition of test2_0_20 for values from (0)
> >> to (10); create table test2_10_20_def partition of test2_0_20
> >> default;
> >>
> >> # explain (costs off) select * from test2 where id=5 or id=25;
> >> QUERY PLAN
> >> -----------------------------------------
> >> Append
> >> -> Seq Scan on test2_0_10
> >> Filter: ((id = 5) OR (id = 25))
> >> -> Seq Scan on test2_10_20_def
> >> Filter: ((id = 5) OR (id = 25))
> >> -> Seq Scan on test2_20_plus_def
> >> Filter: ((id = 5) OR (id = 25))
> >> (7 rows)
> >>
> >> --------------
> >>
> >> I have the same output using Amit's v1-delta.patch or Hosoya's
> >> v2_default_partition_pruning.patch.
> > I think I've figured what may be wrong.
> >
> > Partition pruning step generation code should ignore any arguments of
> > an OR clause that won't be true for a sub-partitioned partition, given
> > its partition constraint.
> >
> > In this case, id = 25 contradicts test2_0_20's partition constraint
> > (which is, a IS NOT NULL AND a >= 0 AND a < 20), so the OR clause
> > should really be simplified to id = 5, ignoring the id = 25 argument.
> > Note that we remove id = 25 only for the considerations of pruning and
> > not from the actual clause that's passed to the final plan, although
> > it wouldn't be a bad idea to try to do that.
> >
> > Attached revised delta patch, which includes the fix described above.
> >
> > Thanks,
> > Amit
> Amit, I tested many cases with nested range sub-partitions... and I did not find any problem with your
> last patch :-)
>
> I tried mixing with hash partitions with no problems.
>
> From the patch, there seems to be less checks than before. I cannot think of a case that can have
> performance impacts.
>
> Hosoya-san, if you agree with Amit's proposal, do you think you can send a patch unifying your
> default_partition_pruning.patch and Amit's second v1-delta.patch?
>
I understood Amit's proposal. But I think the issue Thibaut reported would
occur regardless of whether clauses have OR clauses or not as follows.
I tested a query which should output "One-Time Filter: false".
# explain select * from test2_0_20 where id = 25;
QUERY PLAN
-----------------------------------------------------------------------
Append (cost=0.00..25.91 rows=6 width=36)
-> Seq Scan on test2_10_20_def (cost=0.00..25.88 rows=6 width=36)
Filter: (id = 25)
As Amit described in the previous email, id = 25 contradicts test2_0_20's
partition constraint, so I think this clause should be ignored and we can
also handle this case in the similar way as Amit proposal.
I attached v1-delta-2.patch which fix the above issue.
What do you think about it?
Best regards,
Yuzuko Hosoya
Attachment
Hosoya-san, On 2019/03/22 15:02, Yuzuko Hosoya wrote: > I understood Amit's proposal. But I think the issue Thibaut reported would > occur regardless of whether clauses have OR clauses or not as follows. > I tested a query which should output "One-Time Filter: false". > > # explain select * from test2_0_20 where id = 25; > QUERY PLAN > ----------------------------------------------------------------------- > Append (cost=0.00..25.91 rows=6 width=36) > -> Seq Scan on test2_10_20_def (cost=0.00..25.88 rows=6 width=36) > Filter: (id = 25) > Good catch, thanks. > As Amit described in the previous email, id = 25 contradicts test2_0_20's > partition constraint, so I think this clause should be ignored and we can > also handle this case in the similar way as Amit proposal. > > I attached v1-delta-2.patch which fix the above issue. > > What do you think about it? It looks fine to me. You put the code block to check whether a give clause contradicts the partition constraint in its perfect place. :) Maybe we should have two patches as we seem to be improving two things: 1. Patch to fix problems with default partition pruning originally reported by Hosoya-san 2. Patch to determine if a given clause contradicts a sub-partitioned table's partition constraint, fixing problems unearthed by Thibaut's tests About the patch that Horiguchi-san proposed upthread, I think it has merit that it will make partprune.c code easier to reason about, but I think we should pursue it separately. Thanks, Amit
Le 22/03/2019 à 07:38, Amit Langote a écrit : > Hosoya-san, > > On 2019/03/22 15:02, Yuzuko Hosoya wrote: >> I understood Amit's proposal. But I think the issue Thibaut reported would >> occur regardless of whether clauses have OR clauses or not as follows. >> I tested a query which should output "One-Time Filter: false". >> >> # explain select * from test2_0_20 where id = 25; >> QUERY PLAN >> ----------------------------------------------------------------------- >> Append (cost=0.00..25.91 rows=6 width=36) >> -> Seq Scan on test2_10_20_def (cost=0.00..25.88 rows=6 width=36) >> Filter: (id = 25) >> > Good catch, thanks. > >> As Amit described in the previous email, id = 25 contradicts test2_0_20's >> partition constraint, so I think this clause should be ignored and we can >> also handle this case in the similar way as Amit proposal. >> >> I attached v1-delta-2.patch which fix the above issue. >> >> What do you think about it? > It looks fine to me. You put the code block to check whether a give > clause contradicts the partition constraint in its perfect place. :) > > Maybe we should have two patches as we seem to be improving two things: > > 1. Patch to fix problems with default partition pruning originally > reported by Hosoya-san > > 2. Patch to determine if a given clause contradicts a sub-partitioned > table's partition constraint, fixing problems unearthed by Thibaut's tests > > About the patch that Horiguchi-san proposed upthread, I think it has merit > that it will make partprune.c code easier to reason about, but I think we > should pursue it separately. > > Thanks, > Amit Hosoya-san, very good idea to run queries directly on tables partitions! I tested your last patch and if I didn't mix up patches on the end of a too long week, I get a problem when querying the sub-sub partition: test=# explain select * from test2_0_10 where id = 25; QUERY PLAN ------------------------------------------------------------ Seq Scan on test2_0_10 (cost=0.00..25.88 rows=6 width=36) Filter: (id = 25) (2 rows) Cordialement, Thibaut
Hi, On 2019/03/23 2:36, Thibaut Madelaine wrote: > I tested your last patch and if I didn't mix up patches on the end of a > too long week, I get a problem when querying the sub-sub partition: > > test=# explain select * from test2_0_10 where id = 25; > QUERY PLAN > ------------------------------------------------------------ > Seq Scan on test2_0_10 (cost=0.00..25.88 rows=6 width=36) > Filter: (id = 25) > (2 rows) The problem here is not really related to partition pruning, but another problem I recently sent an email about: https://www.postgresql.org/message-id/9813f079-f16b-61c8-9ab7-4363cab28d80%40lab.ntt.co.jp The problem in this case is that *constraint exclusion* is not working, because partition constraint is not loaded by the planner. Note that pruning is only used if a query specifies the parent table, not a partition. Thanks, Amit
Hi, > > Hi, > > On 2019/03/23 2:36, Thibaut Madelaine wrote: > > I tested your last patch and if I didn't mix up patches on the end of > > a too long week, I get a problem when querying the sub-sub partition: > > > > test=# explain select * from test2_0_10 where id = 25; > > QUERY PLAN > > ------------------------------------------------------------ > > Seq Scan on test2_0_10 (cost=0.00..25.88 rows=6 width=36) > > Filter: (id = 25) > > (2 rows) > > The problem here is not really related to partition pruning, but another problem I recently sent an > email about: > > https://www.postgresql.org/message-id/9813f079-f16b-61c8-9ab7-4363cab28d80%40lab.ntt.co.jp > > The problem in this case is that *constraint exclusion* is not working, because partition constraint > is not loaded by the planner. Note that pruning is only used if a query specifies the parent table, > not a partition. Thanks for the comments. I saw that email. Also, I checked that query Thibaut mentioned worked correctly with Amit's patch discussed in that thread. Best regards, Yuzuko Hosoya
Hi, > Maybe we should have two patches as we seem to be improving two things: > > 1. Patch to fix problems with default partition pruning originally reported by Hosoya-san > > 2. Patch to determine if a given clause contradicts a sub-partitioned table's partition constraint, > fixing problems unearthed by Thibaut's tests I attached the latest patches according to Amit comment. v3_default_partition_pruning.patch fixes default partition pruning problems and ignore_contradictory_where_clauses_at_partprune_step.patch fixes sub-partition problems Thibaut tested. Best regards, Yuzuko Hosoya
Attachment
Hosoya-san, On 2019/04/02 14:02, Yuzuko Hosoya wrote: > Hi, > >> Maybe we should have two patches as we seem to be improving two things: >> >> 1. Patch to fix problems with default partition pruning originally reported by Hosoya-san >> >> 2. Patch to determine if a given clause contradicts a sub-partitioned table's partition constraint, >> fixing problems unearthed by Thibaut's tests > > I attached the latest patches according to Amit comment. > v3_default_partition_pruning.patch fixes default partition pruning problems > and ignore_contradictory_where_clauses_at_partprune_step.patch fixes > sub-partition problems Thibaut tested. Thanks for dividing patches that way. Would it be a good idea to add some new test cases to these patches, just so it's easily apparent what we're changing? So, we could add the test case presented by Thibaut at the following link to the default_partition_pruning.patch: https://www.postgresql.org/message-id/a4968068-6401-7a9c-8bd4-6a3bc9164a86%40dalibo.com And, another reported at the following link to ignore_contradictory_where_clauses_at_partprune_step.patch: https://www.postgresql.org/message-id/bd03f475-30d4-c4d0-3d7f-d2fbde755971%40dalibo.com Actually, it might be possible/better to construct the test queries in partition_prune.sql using the existing tables in that script, that is, without defining new tables just for adding the new test cases. If not, maybe it's OK to create the new tables too. Thanks, Amit
Amit-san, Thanks for the comments. > > Thanks for dividing patches that way. > > Would it be a good idea to add some new test cases to these patches, just so it's easily apparent what > we're changing? Yes, I agree with you. > > So, we could add the test case presented by Thibaut at the following link to the > default_partition_pruning.patch: > > https://www.postgresql.org/message-id/a4968068-6401-7a9c-8bd4-6a3bc9164a86%40dalibo.com > > And, another reported at the following link to > ignore_contradictory_where_clauses_at_partprune_step.patch: > > https://www.postgresql.org/message-id/bd03f475-30d4-c4d0-3d7f-d2fbde755971%40dalibo.com > > Actually, it might be possible/better to construct the test queries in partition_prune.sql using the > existing tables in that script, that is, without defining new tables just for adding the new test cases. > If not, maybe it's OK to create the new tables too. > I see. I added some test cases to each patch according to tests discussed in this thread. However, I found another problem as follows. This query should output "One-Time Filter: false" because rlp3's constraints contradict WHERE clause. ----- postgres=# \d+ rlp3 Partitioned table "public.rlp3" Column | Type | Collation | Nullable | Default | Storage | Stats target | Description --------+-------------------+-----------+----------+---------+----------+--------------+------------- b | character varying | | | | extended | | a | integer | | | | plain | | Partition of: rlp FOR VALUES FROM (15) TO (20) Partition constraint: ((a IS NOT NULL) AND (a >= 15) AND (a < 20)) Partition key: LIST (b varchar_ops) Partitions: rlp3abcd FOR VALUES IN ('ab', 'cd'), rlp3efgh FOR VALUES IN ('ef', 'gh'), rlp3nullxy FOR VALUES IN (NULL, 'xy'), rlp3_default DEFAULT postgres=# explain select * from rlp3 where a = 2; QUERY PLAN -------------------------------------------------------------------- Append (cost=0.00..103.62 rows=24 width=36) -> Seq Scan on rlp3abcd (cost=0.00..25.88 rows=6 width=36) Filter: (a = 2) -> Seq Scan on rlp3efgh (cost=0.00..25.88 rows=6 width=36) Filter: (a = 2) -> Seq Scan on rlp3nullxy (cost=0.00..25.88 rows=6 width=36) Filter: (a = 2) -> Seq Scan on rlp3_default (cost=0.00..25.88 rows=6 width=36) Filter: (a = 2) (9 rows) ----- I think that the place of check contradiction process was wrong At ignore_contradictory_where_clauses_at_partprune_step.patch. So I fixed it. Attached the latest patches. Please check it again. Best regards, Yuzuko Hosoya
Attachment
Hosoya-san,
On 2019/04/04 13:00, Yuzuko Hosoya wrote:
> I added some test cases to each patch according to tests
> discussed in this thread.
Thanks a lot.
> However, I found another problem as follows. This query should
> output "One-Time Filter: false" because rlp3's constraints
> contradict WHERE clause.
>
> -----
> postgres=# \d+ rlp3
> Partitioned table "public.rlp3"
> Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
> --------+-------------------+-----------+----------+---------+----------+--------------+-------------
> b | character varying | | | | extended | |
> a | integer | | | | plain | |
> Partition of: rlp FOR VALUES FROM (15) TO (20)
> Partition constraint: ((a IS NOT NULL) AND (a >= 15) AND (a < 20))
> Partition key: LIST (b varchar_ops)
> Partitions: rlp3abcd FOR VALUES IN ('ab', 'cd'),
> rlp3efgh FOR VALUES IN ('ef', 'gh'),
> rlp3nullxy FOR VALUES IN (NULL, 'xy'),
> rlp3_default DEFAULT
>
> postgres=# explain select * from rlp3 where a = 2;
> QUERY PLAN
> --------------------------------------------------------------------
> Append (cost=0.00..103.62 rows=24 width=36)
> -> Seq Scan on rlp3abcd (cost=0.00..25.88 rows=6 width=36)
> Filter: (a = 2)
> -> Seq Scan on rlp3efgh (cost=0.00..25.88 rows=6 width=36)
> Filter: (a = 2)
> -> Seq Scan on rlp3nullxy (cost=0.00..25.88 rows=6 width=36)
> Filter: (a = 2)
> -> Seq Scan on rlp3_default (cost=0.00..25.88 rows=6 width=36)
> Filter: (a = 2)
> (9 rows)
> -----
This one too would be solved with the other patch I mentioned to fix
get_relation_info() to load the partition constraint so that constraint
exclusion can use it. Partition in the earlier example given by Thibaut
is a leaf partition, whereas rlp3 above is a sub-partitioned partition,
but both are partitions nonetheless.
Fixing partprune.c like we're doing with the
v2_ignore_contradictory_where_clauses_at_partprune_step.patch only works
for the latter, because only partitioned tables visit partprune.c.
OTOH, the other patch only applies to situations where
constraint_exclusion = on.
> I think that the place of check contradiction process was wrong
> At ignore_contradictory_where_clauses_at_partprune_step.patch.
> So I fixed it.
Thanks. Patch contains some whitespace noise:
$ git diff --check
src/backend/partitioning/partprune.c:790: trailing whitespace.
+ * given its partition constraint, we can ignore it,
src/backend/partitioning/partprune.c:791: trailing whitespace.
+ * that is not try to pass it to the pruning code.
src/backend/partitioning/partprune.c:792: trailing whitespace.
+ * We should do that especially to avoid pruning code
src/backend/partitioning/partprune.c:810: trailing whitespace.
+
src/test/regress/sql/partition_prune.sql:87: trailing whitespace.
+-- where clause contradicts sub-partition's constraint
Can you please fix it?
BTW, now I'm a bit puzzled between whether this case should be fixed by
hacking on partprune.c like this patch does or whether to work on getting
the other patch committed and expect users to set constraint_exclusion =
on for this to behave as expected. The original intention of setting
partition_qual in set_relation_partition_info() was for partprune.c to use
it to remove useless arguments of OR clauses which otherwise would cause
the failure to correctly prune the default partitions of sub-partitioned
tables. As shown by the examples in this thread, the original effort was
insufficient, which this patch aims to improve. But, it also expands the
scope of partprune.c's usage of partition_qual, which is to effectively
perform full-blown constraint exclusion without being controllable by
constraint_exclusion GUC, which may be seen as being good or bad. The
fact that it helps in getting partition pruning working correctly in more
obscure cases like those discussed in this thread means it's good maybe.
Thanks,
Amit
Amit-san,
> -----Original Message-----
> From: Amit Langote [mailto:Langote_Amit_f8@lab.ntt.co.jp]
> Sent: Friday, April 05, 2019 6:47 PM
> To: Yuzuko Hosoya <hosoya.yuzuko@lab.ntt.co.jp>; 'Thibaut' <thibaut.madelaine@dalibo.com>; 'Imai,
> Yoshikazu' <imai.yoshikazu@jp.fujitsu.com>
> Cc: 'PostgreSQL Hackers' <pgsql-hackers@lists.postgresql.org>
> Subject: Re: Problem with default partition pruning
>
> Hosoya-san,
>
>
> On 2019/04/04 13:00, Yuzuko Hosoya wrote:
> > I added some test cases to each patch according to tests discussed in
> > this thread.
>
> Thanks a lot.
>
> > However, I found another problem as follows. This query should output
> > "One-Time Filter: false" because rlp3's constraints contradict WHERE
> > clause.
> >
> > -----
> > postgres=# \d+ rlp3
> > Partitioned table "public.rlp3"
> > Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
> >
> --------+-------------------+-----------+----------+---------+----------+--------------+---------
> ----
> > b | character varying | | | | extended | |
> > a | integer | | | | plain | |
> > Partition of: rlp FOR VALUES FROM (15) TO (20) Partition constraint:
> > ((a IS NOT NULL) AND (a >= 15) AND (a < 20)) Partition key: LIST (b
> > varchar_ops)
> > Partitions: rlp3abcd FOR VALUES IN ('ab', 'cd'),
> > rlp3efgh FOR VALUES IN ('ef', 'gh'),
> > rlp3nullxy FOR VALUES IN (NULL, 'xy'),
> > rlp3_default DEFAULT
> >
> > postgres=# explain select * from rlp3 where a = 2;
> > QUERY PLAN
> > --------------------------------------------------------------------
> > Append (cost=0.00..103.62 rows=24 width=36)
> > -> Seq Scan on rlp3abcd (cost=0.00..25.88 rows=6 width=36)
> > Filter: (a = 2)
> > -> Seq Scan on rlp3efgh (cost=0.00..25.88 rows=6 width=36)
> > Filter: (a = 2)
> > -> Seq Scan on rlp3nullxy (cost=0.00..25.88 rows=6 width=36)
> > Filter: (a = 2)
> > -> Seq Scan on rlp3_default (cost=0.00..25.88 rows=6 width=36)
> > Filter: (a = 2)
> > (9 rows)
> > -----
>
> This one too would be solved with the other patch I mentioned to fix
> get_relation_info() to load the partition constraint so that constraint exclusion can use it.
> Partition in the earlier example given by Thibaut is a leaf partition, whereas rlp3 above is a
> sub-partitioned partition, but both are partitions nonetheless.
>
> Fixing partprune.c like we're doing with the
> v2_ignore_contradictory_where_clauses_at_partprune_step.patch only works for the latter, because only
> partitioned tables visit partprune.c.
>
> OTOH, the other patch only applies to situations where constraint_exclusion = on.
>
I see. I think that following example discussed in this thread before would
also be solved with your patch, not v2_ignore_contradictory_where_clauses_at_partprune_step.patch.
postgres=# set constraint_exclusion to on;
postgres=# explain select * from test2_0_20 where id = 25;
QUERY PLAN
------------------------------------------
Result (cost=0.00..0.00 rows=0 width=0)
One-Time Filter: false
(2 rows)
> > I think that the place of check contradiction process was wrong At
> > ignore_contradictory_where_clauses_at_partprune_step.patch.
> > So I fixed it.
>
> Thanks. Patch contains some whitespace noise:
>
> $ git diff --check
> src/backend/partitioning/partprune.c:790: trailing whitespace.
> + * given its partition constraint, we can ignore it,
> src/backend/partitioning/partprune.c:791: trailing whitespace.
> + * that is not try to pass it to the pruning code.
> src/backend/partitioning/partprune.c:792: trailing whitespace.
> + * We should do that especially to avoid pruning code
> src/backend/partitioning/partprune.c:810: trailing whitespace.
> +
> src/test/regress/sql/partition_prune.sql:87: trailing whitespace.
> +-- where clause contradicts sub-partition's constraint
>
> Can you please fix it?
>
Thanks for checking.
I'm attaching the latest patch.
>
> BTW, now I'm a bit puzzled between whether this case should be fixed by hacking on partprune.c like
> this patch does or whether to work on getting the other patch committed and expect users to set
> constraint_exclusion = on for this to behave as expected. The original intention of setting
> partition_qual in set_relation_partition_info() was for partprune.c to use it to remove useless
> arguments of OR clauses which otherwise would cause the failure to correctly prune the default partitions
> of sub-partitioned tables. As shown by the examples in this thread, the original effort was
> insufficient, which this patch aims to improve. But, it also expands the scope of partprune.c's usage
> of partition_qual, which is to effectively perform full-blown constraint exclusion without being
> controllable by constraint_exclusion GUC, which may be seen as being good or bad. The fact that it
> helps in getting partition pruning working correctly in more obscure cases like those discussed in
> this thread means it's good maybe.
>
Umm, even though this modification might be overhead, I think this problem should be solved
without setting constraint_exclusion GUC. But I'm not sure.
Best regards,
Yuzuko Hosoya
Attachment
At Mon, 8 Apr 2019 16:57:35 +0900, "Yuzuko Hosoya" <hosoya.yuzuko@lab.ntt.co.jp> wrote in <00c101d4ede0$babd4390$3037cab0$@lab.ntt.co.jp> > > BTW, now I'm a bit puzzled between whether this case should be fixed by hacking on partprune.c like > > this patch does or whether to work on getting the other patch committed and expect users to set > > constraint_exclusion = on for this to behave as expected. The original intention of setting > > partition_qual in set_relation_partition_info() was for partprune.c to use it to remove useless > > arguments of OR clauses which otherwise would cause the failure to correctly prune the default partitions > > of sub-partitioned tables. As shown by the examples in this thread, the original effort was > > insufficient, which this patch aims to improve. But, it also expands the scope of partprune.c's usage > > of partition_qual, which is to effectively perform full-blown constraint exclusion without being > > controllable by constraint_exclusion GUC, which may be seen as being good or bad. The fact that it > > helps in getting partition pruning working correctly in more obscure cases like those discussed in > > this thread means it's good maybe. > > > Umm, even though this modification might be overhead, I think this problem should be solved > without setting constraint_exclusion GUC. But I'm not sure. Partition pruning and constraint exclusion are orthogonal functions. Note that the default value of the variable is CONSTRAINT_EXCLUSION_PARTITION and the behavior is not a perfect bug. So I think we can reasonably ignore constraints when constraint_exclusion is intentionally turned off. As the result I propose to move the "if(partconstr)" block in the latest patches after the constant-false block, changing the condition as "if (partconstr && constraint_exclusion != CONSTRAINT_EXCLUSION_OFF)". This make partprune reacts to constraint_exclusion the consistent way with the old-fashioned partitioning. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
At Mon, 08 Apr 2019 20:42:51 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20190408.204251.143128146.horiguchi.kyotaro@lab.ntt.co.jp> > At Mon, 8 Apr 2019 16:57:35 +0900, "Yuzuko Hosoya" <hosoya.yuzuko@lab.ntt.co.jp> wrote in <00c101d4ede0$babd4390$3037cab0$@lab.ntt.co.jp> > > > BTW, now I'm a bit puzzled between whether this case should be fixed by hacking on partprune.c like > > > this patch does or whether to work on getting the other patch committed and expect users to set > > > constraint_exclusion = on for this to behave as expected. The original intention of setting > > > partition_qual in set_relation_partition_info() was for partprune.c to use it to remove useless > > > arguments of OR clauses which otherwise would cause the failure to correctly prune the default partitions > > > of sub-partitioned tables. As shown by the examples in this thread, the original effort was > > > insufficient, which this patch aims to improve. But, it also expands the scope of partprune.c's usage > > > of partition_qual, which is to effectively perform full-blown constraint exclusion without being > > > controllable by constraint_exclusion GUC, which may be seen as being good or bad. The fact that it > > > helps in getting partition pruning working correctly in more obscure cases like those discussed in > > > this thread means it's good maybe. > > > > > Umm, even though this modification might be overhead, I think this problem should be solved > > without setting constraint_exclusion GUC. But I'm not sure. > > Partition pruning and constraint exclusion are orthogonal > functions. Note that the default value of the variable is > CONSTRAINT_EXCLUSION_PARTITION and the behavior is not a perfect > bug. So I think we can reasonably ignore constraints when > constraint_exclusion is intentionally turned off. > As the result I propose to move the "if(partconstr)" block in the > latest patches after the constant-false block, changing the > condition as "if (partconstr && constraint_exclusion != > CONSTRAINT_EXCLUSION_OFF)". Mmm. Something is wrong. I should have been sleeping at the time. In my opinion, what we should there is: - Try partition pruning first. - If the partition was not pruned, and constraint is set, check for constant false. - if constraint_exclusion is turned on and constraint is set, examine the constraint. Sorry for the stupidity. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
At Mon, 8 Apr 2019 16:57:35 +0900, "Yuzuko Hosoya" <hosoya.yuzuko@lab.ntt.co.jp> wrote in <00c101d4ede0$babd4390$3037cab0$@lab.ntt.co.jp> > > BTW, now I'm a bit puzzled between whether this case should be fixed by hacking on partprune.c like > > this patch does or whether to work on getting the other patch committed and expect users to set > > constraint_exclusion = on for this to behave as expected. The original intention of setting > > partition_qual in set_relation_partition_info() was for partprune.c to use it to remove useless > > arguments of OR clauses which otherwise would cause the failure to correctly prune the default partitions > > of sub-partitioned tables. As shown by the examples in this thread, the original effort was > > insufficient, which this patch aims to improve. But, it also expands the scope of partprune.c's usage > > of partition_qual, which is to effectively perform full-blown constraint exclusion without being > > controllable by constraint_exclusion GUC, which may be seen as being good or bad. The fact that it > > helps in getting partition pruning working correctly in more obscure cases like those discussed in > > this thread means it's good maybe. > > > Umm, even though this modification might be overhead, I think this problem should be solved > without setting constraint_exclusion GUC. But I'm not sure. As the second thought. Partition constraint is not constraint expression so that's fair to apply partqual ignoring constraint_exclusion. The variable is set false to skip useless expression evaluation on all partitions, but partqual should be evaluated just once. Sorry for my confusion. So still it is wrong that the new code is added in gen_partprune_steps_internal. If partqual results true and the clause is long, the partqual is evaluated uselessly at every recursion. Maybe we should do that when we find that the current clause doesn't match part attributes. Specifically just after the for loop "for (i = 0 ; i < part_scheme->partnattrs; i++)". regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Sigh.. At Tue, 09 Apr 2019 10:28:48 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20190409.102848.252476604.horiguchi.kyotaro@lab.ntt.co.jp> > As the second thought. Partition constraint is not constraint > expression so that's fair to apply partqual ignoring > constraint_exclusion. The variable is set false to skip useless > expression evaluation on all partitions, but partqual should be > evaluated just once. Sorry for my confusion. > > So still it is wrong that the new code is added in > gen_partprune_steps_internal. So still it is wrong that the new code is added at the beginning of the loop on clauses in gen_partprune_steps_internal. > If partqual results true and the > clause is long, the partqual is evaluated uselessly at every > recursion. > > Maybe we should do that when we find that the current clause > doesn't match part attributes. Specifically just after the for > loop "for (i = 0 ; i < part_scheme->partnattrs; i++)". -- Kyotaro Horiguchi NTT Open Source Software Center
Horiguchi-san, Thanks for your comments. > -----Original Message----- > From: Kyotaro HORIGUCHI [mailto:horiguchi.kyotaro@lab.ntt.co.jp] > Sent: Tuesday, April 09, 2019 10:33 AM > To: hosoya.yuzuko@lab.ntt.co.jp > Cc: Langote_Amit_f8@lab.ntt.co.jp; thibaut.madelaine@dalibo.com; imai.yoshikazu@jp.fujitsu.com; > pgsql-hackers@lists.postgresql.org > Subject: Re: Problem with default partition pruning > > Sigh.. > > At Tue, 09 Apr 2019 10:28:48 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI > <horiguchi.kyotaro@lab.ntt.co.jp> wrote in > <20190409.102848.252476604.horiguchi.kyotaro@lab.ntt.co.jp> > > As the second thought. Partition constraint is not constraint > > expression so that's fair to apply partqual ignoring > > constraint_exclusion. The variable is set false to skip useless > > expression evaluation on all partitions, but partqual should be > > evaluated just once. Sorry for my confusion. > > > > So still it is wrong that the new code is added in > > gen_partprune_steps_internal. > > So still it is wrong that the new code is added at the beginning of the loop on clauses in > gen_partprune_steps_internal. > > > If partqual results true and the clause > > is long, the partqual is evaluated uselessly at every recursion. > > > > Maybe we should do that when we find that the current clause doesn't > > match part attributes. Specifically just after the for loop "for (i = > > 0 ; i < part_scheme->partnattrs; i++)". > I think we should check whether WHERE clause contradicts partition constraint even when the clause matches part attributes. So I moved "if (partqual)" block to the beginning of the loop you mentioned. I'm attaching the latest version. Could you please check it again? Best regards, Yuzuko Hosoya
Attachment
Hi. At Tue, 9 Apr 2019 16:41:47 +0900, "Yuzuko Hosoya" <hosoya.yuzuko@lab.ntt.co.jp> wrote in <00cf01d4eea7$afa43370$0eec9a50$@lab.ntt.co.jp> > > So still it is wrong that the new code is added at the beginning of the loop on clauses in > > gen_partprune_steps_internal. > > > > > If partqual results true and the clause > > > is long, the partqual is evaluated uselessly at every recursion. > > > > > > Maybe we should do that when we find that the current clause doesn't > > > match part attributes. Specifically just after the for loop "for (i = > > > 0 ; i < part_scheme->partnattrs; i++)". > > > I think we should check whether WHERE clause contradicts partition > constraint even when the clause matches part attributes. So I moved Why? If clauses contains a clause on a partition key, the clause is involved in determination of whether a partition survives or not in ordinary way. Could you show how or on what configuration (tables and query) it happens that such a matching clause needs to be checked against partqual? The "if (partconstr)" block uselessly runs for every clause in the clause tree other than BoolExpr. If we want do that, isn't just doing predicate_refuted_by(partconstr, clauses, false) sufficient before looping over clauses? > "if (partqual)" block to the beginning of the loop you mentioned. > > I'm attaching the latest version. Could you please check it again? regards. -- Kyotaro Horiguchi NTT Open Source Software Center
At Tue, 09 Apr 2019 17:37:25 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20190409.173725.31175835.horiguchi.kyotaro@lab.ntt.co.jp> > > I'm attaching the latest version. Could you please check it again? By the way, I noticed that partition constraint in a multi-level partition contains duplicate clauses. create table p (a int) partition by range (a); create table c1 partition of p for values from (0) to (10) partition by range (a); create table c11 partition of c1 for values from (0) to (2) partition by range (a); create table c12 partition of c1 for values from (2) to (4) partition by range (a); =# \d+ c12 | Partitioned table "public.c12" | Column | Type | Collation | Nullable | Default | Storage | Stats target | De | scription | --------+---------+-----------+----------+---------+---------+--------------+--- | ---------- | a | integer | | | | plain | | | Partition of: c1 FOR VALUES FROM (2) TO (4) | Partition constraint: ((a IS NOT NULL) AND (a >= 0) AND (a < 10) AND (a IS NOT N | ULL) AND (a >= 2) AND (a < 4)) | Partition key: RANGE (a) | Number of partitions: 0 The partition constraint is equivalent to "(a IS NOT NULL) AND (a >= 2) AND (a < 4)". Is it intentional (for, for example, performance reasons)? Or is it reasonable to deduplicate the quals? regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Horiguchi-san,
On 2019/04/09 17:51, Kyotaro HORIGUCHI wrote:
> At Tue, 09 Apr 2019 17:37:25 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote
in<20190409.173725.31175835.horiguchi.kyotaro@lab.ntt.co.jp>
>>> I'm attaching the latest version. Could you please check it again?
>
> By the way, I noticed that partition constraint in a multi-level
> partition contains duplicate clauses.
>
> create table p (a int) partition by range (a);
> create table c1 partition of p for values from (0) to (10) partition by range (a);
> create table c11 partition of c1 for values from (0) to (2) partition by range (a);
> create table c12 partition of c1 for values from (2) to (4) partition by range (a);
>
> =# \d+ c12
> | Partitioned table "public.c12"
> | Column | Type | Collation | Nullable | Default | Storage | Stats target | De
> | scription
> | --------+---------+-----------+----------+---------+---------+--------------+---
> | ----------
> | a | integer | | | | plain | |
> | Partition of: c1 FOR VALUES FROM (2) TO (4)
> | Partition constraint: ((a IS NOT NULL) AND (a >= 0) AND (a < 10) AND (a IS NOT N
> | ULL) AND (a >= 2) AND (a < 4))
> | Partition key: RANGE (a)
> | Number of partitions: 0
>
>
> The partition constraint is equivalent to "(a IS NOT NULL) AND (a
>> = 2) AND (a < 4)". Is it intentional (for, for example,
> performance reasons)? Or is it reasonable to deduplicate the
> quals?
Yeah, we don't try to simplify that due to lack of infrastructure, maybe.
If said infrastructure was present, maybe CHECK constraints would already
be using that, which doesn't seem to be the case.
create table foo (a int check ((a IS NOT NULL) AND (a >= 0) AND (a < 10)
AND (a IS NOT NULL) AND (a >= 2) AND (a < 4)));
\d foo
Table "public.foo"
Column │ Type │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼─────────
a │ integer │ │ │
Check constraints:
"foo_a_check" CHECK (a IS NOT NULL AND a >= 0 AND a < 10 AND a IS NOT
NULL AND a >= 2 AND a < 4)
Now it's true that users wouldn't manually write expressions like that,
but the expressions might be an automatically generated, which is also the
case with partition constraint of a deeply nested partition.
Thanks,
Amit
Hi, Amit. Thank you for the explanation.
At Tue, 9 Apr 2019 18:09:20 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in
<4c1074cc-bf60-1610-c728-9a5b12f5b234@lab.ntt.co.jp>
> > The partition constraint is equivalent to "(a IS NOT NULL) AND (a
> >> = 2) AND (a < 4)". Is it intentional (for, for example,
> > performance reasons)? Or is it reasonable to deduplicate the
> > quals?
>
> Yeah, we don't try to simplify that due to lack of infrastructure, maybe.
> If said infrastructure was present, maybe CHECK constraints would already
> be using that, which doesn't seem to be the case.
Doesn't predicate_implied_by do that?
With the attached small patch, the partqual in my example becomes.
Partition constraint: ((a IS NOT NULL) AND (a >= 2) AND (a < 4))
And for in a more complex case:
create table p2 (a int, b int) partition by range (a, b);
create table c21 partition of p2 for values from (0, 0) to (1, 50) partition by range (a, b);
create table c22 partition of p2 for values from (1, 50) to (2, 100) partition by range (a, b);
create table c211 partition of c21 for values from (0, 0) to (0, 1000);
create table c212 partition of c21 for values from (0, 1000) to (0, 2000);
\d+ c212
..
Partition constraint: ((a IS NOT NULL) AND (b IS NOT NULL) AND ((a > 0) OR ((a =
0) AND (b >= 0))) AND ((a < 1) OR ((a = 1) AND (b < 50))) AND (a IS NOT NULL) A
ND (b IS NOT NULL) AND (a = 0) AND (b >= 1000) AND (b < 2000))
is reduced to:
Partition constraint: ((a IS NOT NULL) AND (b IS NOT NULL) AND (a = 0) AND (b >=
1000) AND (b < 2000))
Of course this cannot be reducible:
create table p3 (a int, b int) partition by range (a);
create table c31 partition of p3 for values from (0) to (1) partition by range(b);
create table c311 partition of c31 for values from (0) to (1);
\d+ c311
Partition constraint: ((a IS NOT NULL) AND (a >= 0) AND (a < 1) AND (b IS NOT NU
LL) AND (b >= 0) AND (b < 1))
I think this is useful even counting possible degradation, and I
believe generate_partition_qual is not called so often.
> create table foo (a int check ((a IS NOT NULL) AND (a >= 0) AND (a < 10)
> AND (a IS NOT NULL) AND (a >= 2) AND (a < 4)));
>
> \d foo
> Table "public.foo"
> Column │ Type │ Collation │ Nullable │ Default
> ────────┼─────────┼───────────┼──────────┼─────────
> a │ integer │ │ │
> Check constraints:
> "foo_a_check" CHECK (a IS NOT NULL AND a >= 0 AND a < 10 AND a IS NOT
> NULL AND a >= 2 AND a < 4)
>
> Now it's true that users wouldn't manually write expressions like that,
> but the expressions might be an automatically generated, which is also the
> case with partition constraint of a deeply nested partition.
Differently from manually written constraint, partition
constraint is highly reducible.
Thoughts?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/utils/cache/partcache.c b/src/backend/utils/cache/partcache.c
index 8f43d682cf..c2f6d472c2 100644
--- a/src/backend/utils/cache/partcache.c
+++ b/src/backend/utils/cache/partcache.c
@@ -357,7 +357,14 @@ generate_partition_qual(Relation rel)
/* Add the parent's quals to the list (if any) */
if (parent->rd_rel->relispartition)
- result = list_concat(generate_partition_qual(parent), my_qual);
+ {
+ List *pqual = generate_partition_qual(parent);
+
+ if (predicate_implied_by(pqual, my_qual, false))
+ result = my_qual;
+ else
+ result = list_concat(pqual, my_qual);
+ }
else
result = my_qual;
Horiguchi-san, On 2019/04/09 18:49, Kyotaro HORIGUCHI wrote: > Hi, Amit. Thank you for the explanation. > > At Tue, 9 Apr 2019 18:09:20 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in <4c1074cc-bf60-1610-c728-9a5b12f5b234@lab.ntt.co.jp> >>> The partition constraint is equivalent to "(a IS NOT NULL) AND (a >>>> = 2) AND (a < 4)". Is it intentional (for, for example, >>> performance reasons)? Or is it reasonable to deduplicate the >>> quals? >> >> Yeah, we don't try to simplify that due to lack of infrastructure, maybe. >> If said infrastructure was present, maybe CHECK constraints would already >> be using that, which doesn't seem to be the case. > > Doesn't predicate_implied_by do that? > > With the attached small patch, the partqual in my example becomes. Ah, I was wrong in saying we lack the infrastructure. > Partition constraint: ((a IS NOT NULL) AND (a >= 2) AND (a < 4)) > > And for in a more complex case: > > create table p2 (a int, b int) partition by range (a, b); > create table c21 partition of p2 for values from (0, 0) to (1, 50) partition by range (a, b); > create table c22 partition of p2 for values from (1, 50) to (2, 100) partition by range (a, b); > create table c211 partition of c21 for values from (0, 0) to (0, 1000); > create table c212 partition of c21 for values from (0, 1000) to (0, 2000); > > \d+ c212 > .. > Partition constraint: ((a IS NOT NULL) AND (b IS NOT NULL) AND ((a > 0) OR ((a = > 0) AND (b >= 0))) AND ((a < 1) OR ((a = 1) AND (b < 50))) AND (a IS NOT NULL) A > ND (b IS NOT NULL) AND (a = 0) AND (b >= 1000) AND (b < 2000)) > > is reduced to: > > Partition constraint: ((a IS NOT NULL) AND (b IS NOT NULL) AND (a = 0) AND (b >= > 1000) AND (b < 2000)) > > Of course this cannot be reducible: > > create table p3 (a int, b int) partition by range (a); > create table c31 partition of p3 for values from (0) to (1) partition by range(b); > create table c311 partition of c31 for values from (0) to (1); > \d+ c311 > > Partition constraint: ((a IS NOT NULL) AND (a >= 0) AND (a < 1) AND (b IS NOT NU > LL) AND (b >= 0) AND (b < 1)) > > I think this is useful even counting possible degradation, and I > believe generate_partition_qual is not called so often. I think more commonly used forms of sub-partitioning will use different columns at different levels as in the 2nd example. So, although we don't call generate_partition_qual() as much as we used to before, even at the times we do, we'd encounter this type of sub-partitioning more often and the proposed optimization step will end up being futile in more cases than the cases in which it would help. Maybe, that was the reason not to try too hard in the first place, not the lack of infrastructure as I was saying. Thanks, Amit
On 2019/04/09 17:37, Kyotaro HORIGUCHI wrote: > At Tue, 9 Apr 2019 16:41:47 +0900, "Yuzuko Hosoya" <hosoya.yuzuko@lab.ntt.co.jp> wrote >>> So still it is wrong that the new code is added at the beginning of the loop on clauses in >>> gen_partprune_steps_internal. >>> >>>> If partqual results true and the clause >>>> is long, the partqual is evaluated uselessly at every recursion. >>>> >>>> Maybe we should do that when we find that the current clause doesn't >>>> match part attributes. Specifically just after the for loop "for (i = >>>> 0 ; i < part_scheme->partnattrs; i++)". >>> >> I think we should check whether WHERE clause contradicts partition >> constraint even when the clause matches part attributes. So I moved > > Why? If clauses contains a clause on a partition key, the clause > is involved in determination of whether a partition survives or > not in ordinary way. Could you show how or on what configuration > (tables and query) it happens that such a matching clause needs > to be checked against partqual? > > The "if (partconstr)" block uselessly runs for every clause in > the clause tree other than BoolExpr. If we want do that, isn't > just doing predicate_refuted_by(partconstr, clauses, false) > sufficient before looping over clauses? Yeah, I think we should move the "if (partconstr)" block to the "if (is_orclause(clause))" block as I originally proposed in: https://www.postgresql.org/message-id/9bb31dfe-b0d0-53f3-3ea6-e64b811424cf%40lab.ntt.co.jp It's kind of a hack to get over the limitation that get_matching_partitions() can't prune default partitions for certain OR clauses and I think we shouldn't let that hack grow into what seems like almost duplicating relation_excluded_by_constraints() but without the constraint_exclusion GUC guard. Thanks, Amit
Horiguchi-san,
> -----Original Message-----
> From: Kyotaro HORIGUCHI [mailto:horiguchi.kyotaro@lab.ntt.co.jp]
> Sent: Tuesday, April 09, 2019 5:37 PM
> To: hosoya.yuzuko@lab.ntt.co.jp
> Cc: Langote_Amit_f8@lab.ntt.co.jp; thibaut.madelaine@dalibo.com;
> imai.yoshikazu@jp.fujitsu.com; pgsql-hackers@lists.postgresql.org
> Subject: Re: Problem with default partition pruning
>
> Hi.
>
> At Tue, 9 Apr 2019 16:41:47 +0900, "Yuzuko Hosoya"
> <hosoya.yuzuko@lab.ntt.co.jp> wrote in
> <00cf01d4eea7$afa43370$0eec9a50$@lab.ntt.co.jp>
> > > So still it is wrong that the new code is added at the beginning
> > > of the loop on clauses in gen_partprune_steps_internal.
> > >
> > > > If partqual results true and the
> > > > clause is long, the partqual is evaluated uselessly at every recursion.
> > > >
> > > > Maybe we should do that when we find that the current clause
> > > > doesn't match part attributes. Specifically just after the for
> > > > loop "for (i =
> > > > 0 ; i < part_scheme->partnattrs; i++)".
> > >
> > I think we should check whether WHERE clause contradicts partition
> > constraint even when the clause matches part attributes. So I moved
>
> Why? If clauses contains a clause on a partition key, the clause is
> involved in determination of whether a partition survives or not in
> ordinary way. Could you show how or on what configuration (tables and
> query) it happens that such a matching clause needs to be checked against partqual?
>
We found that partition pruning didn't work as expect when we scanned a sub-partition using WHERE
clause which contradicts the sub-partition's constraint by Thibaut tests.
The example discussed in this thread as follows.
postgres=# \d+ test2
Partitioned table "public.test2"
Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
--------+---------+-----------+----------+---------+----------+--------------+-------------
id | integer | | | | plain | |
val | text | | | | extended | |
Partition key: RANGE (id)
Partitions: test2_0_20 FOR VALUES FROM (0) TO (20), PARTITIONED,
test2_20_plus_def DEFAULT
postgres=# \d+ test2_0_20
Partitioned table "public.test2_0_20"
Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
--------+---------+-----------+----------+---------+----------+--------------+-------------
id | integer | | | | plain | |
val | text | | | | extended | |
Partition of: test2 FOR VALUES FROM (0) TO (20) Partition constraint: ((id IS NOT NULL) AND (id >=
0) AND (id < 20)) Partition key: RANGE (id)
Partitions: test2_0_10 FOR VALUES FROM (0) TO (10),
test2_10_20_def DEFAULT
postgres=# explain (costs off) select * from test2 where id=5 or id=20;
QUERY PLAN
-----------------------------------------
Append
-> Seq Scan on test2_0_10
Filter: ((id = 5) OR (id = 20))
-> Seq Scan on test2_10_20_def
Filter: ((id = 5) OR (id = 20))
-> Seq Scan on test2_20_plus_def
Filter: ((id = 5) OR (id = 20))
(7 rows)
postgres=# explain (costs off) select * from test2_0_20 where id=25;
QUERY PLAN
-----------------------------
Seq Scan on test2_10_20_def
Filter: (id = 25)
(2 rows)
So I think we have to check if WHERE clause contradicts sub-partition's constraint regardless of
whether the clause matches part attributes or not.
> The "if (partconstr)" block uselessly runs for every clause in the clause tree other than
BoolExpr.
> If we want do that, isn't just doing predicate_refuted_by(partconstr,
> clauses, false) sufficient before looping over clauses?
Yes, I tried doing that in the original patch.
>
>
> > "if (partqual)" block to the beginning of the loop you mentioned.
> >
> > I'm attaching the latest version. Could you please check it again?
>
> regards.
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
Best regards,
Yuzuko Hosoya
Hi, Amit. At Wed, 10 Apr 2019 10:48:38 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in <4ef8d47d-b0c7-3093-5aaa-226162c5b59b@lab.ntt.co.jp> > > I think this is useful even counting possible degradation, and I > > believe generate_partition_qual is not called so often. > > I think more commonly used forms of sub-partitioning will use different > columns at different levels as in the 2nd example. So, although we don't > call generate_partition_qual() as much as we used to before, even at the > times we do, we'd encounter this type of sub-partitioning more often and > the proposed optimization step will end up being futile in more cases than > the cases in which it would help. Maybe, that was the reason not to try > too hard in the first place, not the lack of infrastructure as I was saying. Range partitioning on date could be a common example of multilevel partitioning, but I agree with you given a premise that partition qual is not scanned so frequently. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
At Wed, 10 Apr 2019 11:17:53 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in <494124a7-d305-1bc9-ef64-d5c790e13e86@lab.ntt.co.jp> > On 2019/04/09 17:37, Kyotaro HORIGUCHI wrote: > > At Tue, 9 Apr 2019 16:41:47 +0900, "Yuzuko Hosoya" <hosoya.yuzuko@lab.ntt.co.jp> wrote > >>> So still it is wrong that the new code is added at the beginning of the loop on clauses in > >>> gen_partprune_steps_internal. > >>> > >>>> If partqual results true and the clause > >>>> is long, the partqual is evaluated uselessly at every recursion. > >>>> > >>>> Maybe we should do that when we find that the current clause doesn't > >>>> match part attributes. Specifically just after the for loop "for (i = > >>>> 0 ; i < part_scheme->partnattrs; i++)". > >>> > >> I think we should check whether WHERE clause contradicts partition > >> constraint even when the clause matches part attributes. So I moved > > > > Why? If clauses contains a clause on a partition key, the clause > > is involved in determination of whether a partition survives or > > not in ordinary way. Could you show how or on what configuration > > (tables and query) it happens that such a matching clause needs > > to be checked against partqual? > > > > The "if (partconstr)" block uselessly runs for every clause in > > the clause tree other than BoolExpr. If we want do that, isn't > > just doing predicate_refuted_by(partconstr, clauses, false) > > sufficient before looping over clauses? > > Yeah, I think we should move the "if (partconstr)" block to the "if > (is_orclause(clause))" block as I originally proposed in: > > https://www.postgresql.org/message-id/9bb31dfe-b0d0-53f3-3ea6-e64b811424cf%40lab.ntt.co.jp > > It's kind of a hack to get over the limitation that > get_matching_partitions() can't prune default partitions for certain OR > clauses and I think we shouldn't let that hack grow into what seems like > almost duplicating relation_excluded_by_constraints() but without the > constraint_exclusion GUC guard. That leaves an issue of contradicting clauses that is not an arm of OR-expr. Isn't that what Hosoya-san is trying to fix? -- Kyotaro Horiguchi NTT Open Source Software Center
Hi. (The thread seems broken for Thunderbird) At Wed, 10 Apr 2019 11:24:11 +0900, "Yuzuko Hosoya" <hosoya.yuzuko@lab.ntt.co.jp> wrote in <00df01d4ef44$7bb79370$7326ba50$@lab.ntt.co.jp> > > Why? If clauses contains a clause on a partition key, the clause is > > involved in determination of whether a partition survives or not in > > ordinary way. Could you show how or on what configuration (tables and > > query) it happens that such a matching clause needs to be checked against partqual? > > > We found that partition pruning didn't work as expect when we scanned a sub-partition using WHERE > clause which contradicts the sub-partition's constraint by Thibaut tests. > The example discussed in this thread as follows. > > postgres=# \d+ test2 > Partitioned table "public.test2" > Column | Type | Collation | Nullable | Default | Storage | Stats target | Description > --------+---------+-----------+----------+---------+----------+--------------+------------- > id | integer | | | | plain | | > val | text | | | | extended | | > Partition key: RANGE (id) > Partitions: test2_0_20 FOR VALUES FROM (0) TO (20), PARTITIONED, > test2_20_plus_def DEFAULT > > postgres=# \d+ test2_0_20 > Partitioned table "public.test2_0_20" > Column | Type | Collation | Nullable | Default | Storage | Stats target | Description > --------+---------+-----------+----------+---------+----------+--------------+------------- > id | integer | | | | plain | | > val | text | | | | extended | | > Partition of: test2 FOR VALUES FROM (0) TO (20) Partition constraint: ((id IS NOT NULL) AND (id >= > 0) AND (id < 20)) Partition key: RANGE (id) > Partitions: test2_0_10 FOR VALUES FROM (0) TO (10), > test2_10_20_def DEFAULT > > postgres=# explain (costs off) select * from test2 where id=5 or id=20; > QUERY PLAN > ----------------------------------------- > Append > -> Seq Scan on test2_0_10 > Filter: ((id = 5) OR (id = 20)) > -> Seq Scan on test2_10_20_def > Filter: ((id = 5) OR (id = 20)) > -> Seq Scan on test2_20_plus_def > Filter: ((id = 5) OR (id = 20)) > (7 rows) I think this is problematic. > postgres=# explain (costs off) select * from test2_0_20 where id=25; > QUERY PLAN > ----------------------------- > Seq Scan on test2_10_20_def > Filter: (id = 25) > (2 rows) > > So I think we have to check if WHERE clause contradicts sub-partition's constraint regardless of > whether the clause matches part attributes or not. If that is the only issue here, doesn't Amit's proposal work? And that doesn't seem to justify rechecking key clauses to partquals for every leaf node in an expression tree. I thought that you are trying to resolve is the issue on non-key caluses that contradicts to partition constraints? > > The "if (partconstr)" block uselessly runs for every clause in the clause tree other than > BoolExpr. > > If we want do that, isn't just doing predicate_refuted_by(partconstr, > > clauses, false) sufficient before looping over clauses? > Yes, I tried doing that in the original patch. -- Kyotaro Horiguchi NTT Open Source Software Center
On 2019/04/10 12:53, Kyotaro HORIGUCHI wrote: > At Wed, 10 Apr 2019 11:17:53 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: >> Yeah, I think we should move the "if (partconstr)" block to the "if >> (is_orclause(clause))" block as I originally proposed in: >> >> https://www.postgresql.org/message-id/9bb31dfe-b0d0-53f3-3ea6-e64b811424cf%40lab.ntt.co.jp >> >> It's kind of a hack to get over the limitation that >> get_matching_partitions() can't prune default partitions for certain OR >> clauses and I think we shouldn't let that hack grow into what seems like >> almost duplicating relation_excluded_by_constraints() but without the >> constraint_exclusion GUC guard. > > That leaves an issue of contradicting clauses that is not an arm > of OR-expr. Isn't that what Hosoya-san is trying to fix? Yes, that's right. But as I said, maybe we should try not to duplicate the functionality of relation_excluded_by_constraints() in partprune.c. To summarize, aside from the problem described by the subject of this thread (patch for that is v4_default_partition_pruning.patch posted by Hosoya-san on 2019/04/04), we have identified couple of other issues: 1. One that Thibaut reported on 2019/03/04 > I kept on testing with sub-partitioning.Thanks. > I found a case, using 2 default partitions, where a default partition is > not pruned: > > -------------- > > create table test2(id int, val text) partition by range (id); > create table test2_20_plus_def partition of test2 default; > create table test2_0_20 partition of test2 for values from (0) to (20) > partition by range (id); > create table test2_0_10 partition of test2_0_20 for values from (0) to (10); > create table test2_10_20_def partition of test2_0_20 default; > > # explain (costs off) select * from test2 where id=5 or id=25; > QUERY PLAN > ----------------------------------------- > Append > -> Seq Scan on test2_0_10 > Filter: ((id = 5) OR (id = 25)) > -> Seq Scan on test2_10_20_def > Filter: ((id = 5) OR (id = 25)) > -> Seq Scan on test2_20_plus_def > Filter: ((id = 5) OR (id = 25)) > (7 rows) For this, we can move the "if (partconstr)" block in the same if (is_orclause(clause)) block, as proposed in the v1-delta.patch that I proposed on 2019/03/20. Note that that patch restricts the scope of applying predicate_refuted_by() only to the problem that's currently tricky to solve by partition pruning alone -- pruning default partitions for OR clauses like in the above example. 2. Hosoya-san reported on 2019/03/22 that a contradictory WHERE clause applied to a *partition* doesn't return an empty plan: > I understood Amit's proposal. But I think the issue Thibaut reported > would occur regardless of whether clauses have OR clauses or not as > follows. > > I tested a query which should output "One-Time Filter: false". > > # explain select * from test2_0_20 where id = 25; > QUERY PLAN > ----------------------------------------------------------------------- > Append (cost=0.00..25.91 rows=6 width=36) > -> Seq Scan on test2_10_20_def (cost=0.00..25.88 rows=6 width=36) > Filter: (id = 25) So, she proposed to apply predicate_refuted_by to the whole baserestrictinfo (at least in the latest patch), which is same as always performing constraint exclusion to sub-partitioned partitions. I initially thought it might be a good idea, but only later realized that now there will be two places doing the same constraint exclusion proof -- gen_partprune_steps_internal(), and set_rel_size() calling relation_excluded_by_constraints(). The latter depends on constraint_exclusion GUC whose default being 'partition' would mean we'd not get an empty plan with it. Even if you turn it to 'on', a bug of get_relation_constraints() will prevent the partition constraint from being loaded and performing constraint exclusion with it; I reported it in [1]. I think that we may be better off solving the latter problem as follows: 1. Modify relation_excluded_by_constraints() to *always* try to exclude "baserel" partitions using their partition constraint (disregarding constraint_exclusion = off/partition). 2. Modify prune_append_rel_partitions(), which runs much earlier these days compared to set_rel_size(), to call relation_excluded_by_constraint() modified as described in step 1. If it returns true, don't perform partition pruning, set the appendrel parent as dummy right away. It's not done today, but appendrel parent can also be set to dummy based on the result of pruning, that is, when get_matching_partitions() returns no matching partitions. 3. Modify set_base_rel_sizes() to ignore already-dummy rels, so that we don't perform constraint exclusion again via set_rel_size(). I have to say this other problem involving partition constraints is quite complicated (aforementioned past bug messing up the situation further), so it would be nice if a committer can review and commit the solutions for the originally reported pruning issues. Thanks, Amit [1] https://www.postgresql.org/message-id/9813f079-f16b-61c8-9ab7-4363cab28d80@lab.ntt.co.jp
On 2019/04/10 14:55, Amit Langote wrote: > 2. Hosoya-san reported on 2019/03/22 that a contradictory WHERE clause > applied to a *partition* doesn't return an empty plan: > >> I understood Amit's proposal. But I think the issue Thibaut reported >> would occur regardless of whether clauses have OR clauses or not as >> follows. >> >> I tested a query which should output "One-Time Filter: false". >> >> # explain select * from test2_0_20 where id = 25; >> QUERY PLAN >> ----------------------------------------------------------------------- >> Append (cost=0.00..25.91 rows=6 width=36) >> -> Seq Scan on test2_10_20_def (cost=0.00..25.88 rows=6 width=36) >> Filter: (id = 25) > > So, she proposed to apply predicate_refuted_by to the whole > baserestrictinfo (at least in the latest patch), which is same as always > performing constraint exclusion to sub-partitioned partitions. I > initially thought it might be a good idea, but only later realized that > now there will be two places doing the same constraint exclusion proof -- > gen_partprune_steps_internal(), and set_rel_size() calling > relation_excluded_by_constraints(). The latter depends on > constraint_exclusion GUC whose default being 'partition' would mean we'd > not get an empty plan with it. Even if you turn it to 'on', a bug of > get_relation_constraints() will prevent the partition constraint from > being loaded and performing constraint exclusion with it; I reported it in > [1]. > > I think that we may be better off solving the latter problem as follows: > > 1. Modify relation_excluded_by_constraints() to *always* try to exclude > "baserel" partitions using their partition constraint (disregarding > constraint_exclusion = off/partition). > > 2. Modify prune_append_rel_partitions(), which runs much earlier these > days compared to set_rel_size(), to call relation_excluded_by_constraint() > modified as described in step 1. If it returns true, don't perform > partition pruning, set the appendrel parent as dummy right away. It's not > done today, but appendrel parent can also be set to dummy based on the > result of pruning, that is, when get_matching_partitions() returns no > matching partitions. > > 3. Modify set_base_rel_sizes() to ignore already-dummy rels, so that we > don't perform constraint exclusion again via set_rel_size(). > > I have to say this other problem involving partition constraints is quite > complicated (aforementioned past bug messing up the situation further), so > it would be nice if a committer can review and commit the solutions for > the originally reported pruning issues. Just to be clear, I wrote this for HEAD. In PG 11, set_rel_size() and relation_excluded_by_constraints() run before prune_append_rel_partitions(), so we won't need to change the latter when back-patching. Thanks, Amit
At Wed, 10 Apr 2019 14:55:48 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in <d2c38e4e-ade4-74de-f686-af37e4a5f1c1@lab.ntt.co.jp> > On 2019/04/10 12:53, Kyotaro HORIGUCHI wrote: > > At Wed, 10 Apr 2019 11:17:53 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote: > >> Yeah, I think we should move the "if (partconstr)" block to the "if > >> (is_orclause(clause))" block as I originally proposed in: > >> > >> https://www.postgresql.org/message-id/9bb31dfe-b0d0-53f3-3ea6-e64b811424cf%40lab.ntt.co.jp > >> > >> It's kind of a hack to get over the limitation that > >> get_matching_partitions() can't prune default partitions for certain OR > >> clauses and I think we shouldn't let that hack grow into what seems like > >> almost duplicating relation_excluded_by_constraints() but without the > >> constraint_exclusion GUC guard. > > > > That leaves an issue of contradicting clauses that is not an arm > > of OR-expr. Isn't that what Hosoya-san is trying to fix? > > Yes, that's right. But as I said, maybe we should try not to duplicate > the functionality of relation_excluded_by_constraints() in partprune.c. Currently we classify partition constraint as a constraint. So it should be handled not in partition pruning, but constraint exclusion code. That's sound reasonable. > To summarize, aside from the problem described by the subject of this > thread (patch for that is v4_default_partition_pruning.patch posted by > Hosoya-san on 2019/04/04), we have identified couple of other issues: > > 1. One that Thibaut reported on 2019/03/04 > > > I kept on testing with sub-partitioning.Thanks. > > I found a case, using 2 default partitions, where a default partition is > > not pruned: > > > > -------------- > > > > create table test2(id int, val text) partition by range (id); > > create table test2_20_plus_def partition of test2 default; > > create table test2_0_20 partition of test2 for values from (0) to (20) > > partition by range (id); > > create table test2_0_10 partition of test2_0_20 for values from (0) to (10); > > create table test2_10_20_def partition of test2_0_20 default; > > > > # explain (costs off) select * from test2 where id=5 or id=25; > > QUERY PLAN > > ----------------------------------------- > > Append > > -> Seq Scan on test2_0_10 > > Filter: ((id = 5) OR (id = 25)) > > -> Seq Scan on test2_10_20_def > > Filter: ((id = 5) OR (id = 25)) > > -> Seq Scan on test2_20_plus_def > > Filter: ((id = 5) OR (id = 25)) > > (7 rows) > > For this, we can move the "if (partconstr)" block in the same if > (is_orclause(clause)) block, as proposed in the v1-delta.patch that I > proposed on 2019/03/20. Note that that patch restricts the scope of > applying predicate_refuted_by() only to the problem that's currently > tricky to solve by partition pruning alone -- pruning default partitions > for OR clauses like in the above example. This is a failure of partition pruning, which should be resolved in the partprune code. > 2. Hosoya-san reported on 2019/03/22 that a contradictory WHERE clause > applied to a *partition* doesn't return an empty plan: > > > I understood Amit's proposal. But I think the issue Thibaut reported > > would occur regardless of whether clauses have OR clauses or not as > > follows. > > > > I tested a query which should output "One-Time Filter: false". > > > > # explain select * from test2_0_20 where id = 25; > > QUERY PLAN > > ----------------------------------------------------------------------- > > Append (cost=0.00..25.91 rows=6 width=36) > > -> Seq Scan on test2_10_20_def (cost=0.00..25.88 rows=6 width=36) > > Filter: (id = 25) > > So, she proposed to apply predicate_refuted_by to the whole > baserestrictinfo (at least in the latest patch), which is same as always > performing constraint exclusion to sub-partitioned partitions. I > initially thought it might be a good idea, but only later realized that > now there will be two places doing the same constraint exclusion proof -- > gen_partprune_steps_internal(), and set_rel_size() calling > relation_excluded_by_constraints(). The latter depends on > constraint_exclusion GUC whose default being 'partition' would mean we'd > not get an empty plan with it. Even if you turn it to 'on', a bug of > get_relation_constraints() will prevent the partition constraint from > being loaded and performing constraint exclusion with it; I reported it in > [1]. Hmm. One perplexing thing here is the fact that partition constraint is not a table constraint but a partitioning classification in users' view. > I think that we may be better off solving the latter problem as follows: > > 1. Modify relation_excluded_by_constraints() to *always* try to exclude > "baserel" partitions using their partition constraint (disregarding > constraint_exclusion = off/partition). > > 2. Modify prune_append_rel_partitions(), which runs much earlier these > days compared to set_rel_size(), to call relation_excluded_by_constraint() > modified as described in step 1. If it returns true, don't perform > partition pruning, set the appendrel parent as dummy right away. It's not > done today, but appendrel parent can also be set to dummy based on the > result of pruning, that is, when get_matching_partitions() returns no > matching partitions. > > 3. Modify set_base_rel_sizes() to ignore already-dummy rels, so that we > don't perform constraint exclusion again via set_rel_size(). > > I have to say this other problem involving partition constraints is quite > complicated (aforementioned past bug messing up the situation further), so > it would be nice if a committer can review and commit the solutions for > the originally reported pruning issues. Tend to agree. Anyway the other problem needs to involve parent of the specified relation, which is not a thing that doesn't seem to be able to be done the ordinary way. > Thanks, > Amit > > [1] > https://www.postgresql.org/message-id/9813f079-f16b-61c8-9ab7-4363cab28d80@lab.ntt.co.jp regards. -- Kyotaro Horiguchi NTT Open Source Software Center
The following review has been posted through the commitfest application: make installcheck-world: tested, failed Implements feature: tested, passed Spec compliant: not tested Documentation: not tested Hi Hosoya-san, I tested different types of key values, and multi-level partitioned tables, and found no problems. Only the SQL in the file of src/test/regress/results/partition_prune.out has a space that caused the regression test to fail. The new status of this patch is: Waiting on Author
On 2019-Jun-17, Shawn Wang wrote:
> I tested different types of key values, and multi-level partitioned tables, and found no problems.
> Only the SQL in the file of src/test/regress/results/partition_prune.out has a space that caused the regression test
tofail.
It's not clear to me what patch were you reviewing. The latest patch I
see in this thread, in [1], does not apply in any branches. As another
test, I tried to apply the patch on commit 489e431ba56b (before Tom's
partprune changes in mid May); if you use "patch -p1
--ignore-whitespace" it is accepted, but the failure case proposed at
the start of the thread shows the same behavior (namely, that test1_def
is scanned when it is not needed):
55432 12devel 23506=# create table test1(id int, val text) partition by range (id);
create table test1_1 partition of test1 for values from (0) to (100);
create table test1_2 partition of test1 for values from (150) to (200);
create table test1_def partition of test1 default;
explain select * from test1 where id > 0 and id < 30;
CREATE TABLE
Duración: 5,736 ms
CREATE TABLE
Duración: 5,622 ms
CREATE TABLE
Duración: 3,585 ms
CREATE TABLE
Duración: 3,828 ms
QUERY PLAN
─────────────────────────────────────────────────────────────────
Append (cost=0.00..58.16 rows=12 width=36)
-> Seq Scan on test1_1 (cost=0.00..29.05 rows=6 width=36)
Filter: ((id > 0) AND (id < 30))
-> Seq Scan on test1_def (cost=0.00..29.05 rows=6 width=36)
Filter: ((id > 0) AND (id < 30))
(5 filas)
Duración: 2,465 ms
[1] https://postgr.es/m/00cf01d4eea7$afa43370$0eec9a50$@lab.ntt.co.jp
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi Alvaro,
Thank you for your reply.
You can see that the mail start time is February 22. So I looked at the latest version at that time. I found that v11.2 was the newest branch at the time. So I tried to merge this patch into the code, and I found that everything worked. So I tested on this branch and got the results.
You need to add the v4_default_partition_pruning.patch first, and then add the v3_ignore_contradictory_where_clauses_at_partprune_step.patch.
Otherwise, you will find some errors.
I hope this helps you.
Regards.
--
Shawn Wang
Shawn Wang
Alvaro Herrera <alvherre@2ndquadrant.com> 于2019年6月22日周六 上午4:03写道：
On 2019-Jun-17, Shawn Wang wrote:
> I tested different types of key values, and multi-level partitioned tables, and found no problems.
> Only the SQL in the file of src/test/regress/results/partition_prune.out has a space that caused the regression test to fail.
It's not clear to me what patch were you reviewing. The latest patch I
see in this thread, in [1], does not apply in any branches. As another
test, I tried to apply the patch on commit 489e431ba56b (before Tom's
partprune changes in mid May); if you use "patch -p1
--ignore-whitespace" it is accepted, but the failure case proposed at
the start of the thread shows the same behavior (namely, that test1_def
is scanned when it is not needed):
55432 12devel 23506=# create table test1(id int, val text) partition by range (id);
create table test1_1 partition of test1 for values from (0) to (100);
create table test1_2 partition of test1 for values from (150) to (200);
create table test1_def partition of test1 default;
explain select * from test1 where id > 0 and id < 30;
CREATE TABLE
Duración: 5,736 ms
CREATE TABLE
Duración: 5,622 ms
CREATE TABLE
Duración: 3,585 ms
CREATE TABLE
Duración: 3,828 ms
QUERY PLAN
─────────────────────────────────────────────────────────────────
Append (cost=0.00..58.16 rows=12 width=36)
-> Seq Scan on test1_1 (cost=0.00..29.05 rows=6 width=36)
Filter: ((id > 0) AND (id < 30))
-> Seq Scan on test1_def (cost=0.00..29.05 rows=6 width=36)
Filter: ((id > 0) AND (id < 30))
(5 filas)
Duración: 2,465 ms
[1] https://postgr.es/m/00cf01d4eea7$afa43370$0eec9a50$@lab.ntt.co.jp
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-Jun-24, shawn wang wrote: Hello, > Thank you for your reply. > You can see that the mail start time is February 22. So I looked at the > latest version at that time. I found that v11.2 was the newest branch at > the time. So I tried to merge this patch into the code, and I found that > everything worked. I see -- I only tried master, didn't occur to me to try it against REL_11_STABLE. > So I tested on this branch and got the results. > You need to add the v4_default_partition_pruning.patch > <https://www.postgresql.org/message-id/attachment/100463/v4_default_partition_pruning.patch> > first, > and then add the > v3_ignore_contradictory_where_clauses_at_partprune_step.patch > <https://www.postgresql.org/message-id/attachment/100591/v3_ignore_contradictory_where_clauses_at_partprune_step.patch> Oh, so there are two patches? It's easier to keep track if they're always posted together. Anyway, I may have some time to have a look tomorrow (Monday). -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hello Shawn, Alvaro, Thank you for testing patches and comments. Yes, there are two patches: (1) v4_default_partition_pruning.patch fixes problems with default partition pruning and (2) v3_ignore_contradictory_where_clauses_at_partprune_step.patch determines if a given clause contradicts a sub-partitioned table's partition constraint. I'll post two patches together next time. Anyway, I'll rebase two patches to apply on master and fix space. Regards, Yuzuko Hosoya On Mon, Jun 24, 2019 at 12:45 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > On 2019-Jun-24, shawn wang wrote: > > Hello, > > > Thank you for your reply. > > You can see that the mail start time is February 22. So I looked at the > > latest version at that time. I found that v11.2 was the newest branch at > > the time. So I tried to merge this patch into the code, and I found that > > everything worked. > > I see -- I only tried master, didn't occur to me to try it against > REL_11_STABLE. > > > So I tested on this branch and got the results. > > You need to add the v4_default_partition_pruning.patch > > <https://www.postgresql.org/message-id/attachment/100463/v4_default_partition_pruning.patch> > > first, > > and then add the > > v3_ignore_contradictory_where_clauses_at_partprune_step.patch > > <https://www.postgresql.org/message-id/attachment/100591/v3_ignore_contradictory_where_clauses_at_partprune_step.patch> > > Oh, so there are two patches? It's easier to keep track if they're > always posted together. Anyway, I may have some time to have a look > tomorrow (Monday). > > -- > Álvaro Herrera https://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Best regards, Yuzuko Hosoya NTT Open Source Software Center
Hello, On Tue, Jun 25, 2019 at 1:45 PM yuzuko <yuzukohosoya@gmail.com> wrote: > > Hello Shawn, Alvaro, > > Thank you for testing patches and comments. > Yes, there are two patches: > (1) v4_default_partition_pruning.patch fixes problems with default > partition pruning > and (2) v3_ignore_contradictory_where_clauses_at_partprune_step.patch determines > if a given clause contradicts a sub-partitioned table's partition constraint. > I'll post two patches together next time. > > Anyway, I'll rebase two patches to apply on master and fix space. > Attach the latest patches discussed in this thread. I rebased the second patch (v5_ignore_contradictory_where_clauses_at_partprune_step.patch) on the current master. Could you please check them again? -- Best regards, Yuzuko Hosoya NTT Open Source Software Center
Attachment
Hosoya-san, Thanks for updating the patches. I have no comment in particular about v4_default_partition_pruning.patch, but let me reiterate my position about v5_ignore_contradictory_where_clauses_at_partprune_step.patch, which I first stated in the following email a few months ago: https://www.postgresql.org/message-id/d2c38e4e-ade4-74de-f686-af37e4a5f1c1%40lab.ntt.co.jp This patch proposes to apply constraint exclusion to check whether it will be wasteful to generate pruning steps from a given clause against a given sub-partitioned table, because the clause contradicts its partition clause. Actually, the patch started out to generalize the existing usage of constraint exclusion in partprune.c that's used to skip processing useless arguments of an OR clause. The problem with steps generated from such contradictory clauses is that they fail to prune the default partition of a sub-partitioned table, because the value extracted from such a clause appears to the pruning logic to fall in the default partition, given that the pruning logic proper is unaware of the partition constraint of the partitioned table that pruning is being applied to. Here is an example similar to one that Hosoya-san shared earlier on this thread that shows the problem. create table p (a int) partition by range (a); create table p1 partition of p for values from (0) to (20) partition by range (a); create table p11 partition of p1 for values from (0) to (10); create table p1_def partition of p1 default; -- p11 correctly pruned, but p1_def not explain select * from p1 where a = 25; QUERY PLAN ────────────────────────────────────────────────────────────── Append (cost=0.00..41.94 rows=13 width=4) -> Seq Scan on p1_def (cost=0.00..41.88 rows=13 width=4) Filter: (a = 25) (3 rows) Here without the knowledge that p1's range is restricted to 0 <= a < 20 by way of its partition constraint, the pruning logic, when handed the value 25, concludes that p1_def must be scanned. With the patch, partprune.c concludes without performing pruning that scanning any of p1's partitions is unnecessary. explain select * from p1 where a = 25; QUERY PLAN ────────────────────────────────────────── Result (cost=0.00..0.00 rows=0 width=0) One-Time Filter: false (2 rows) Actually, as of 11.4, setting constraint_exclusion = on, by way of relation_excluded_by_constraints(), will give you the same result even without the patch. My argument earlier was that we shouldn't have two places that will do essentially the same processing -- partprune.c with the patch applied and relation_excluded_by_constraints(). That is, we should only keep the latter, with the trade-off that users have to live with the default partition of sub-partitioned tables not being pruned in some corner cases like this one. Note that there's still a problem with the existing usage of constraint exclusion in partprune.c, which Thibaut first reported on this thread [1]. explain select * from p1 where a = 25 or a = 5; QUERY PLAN ────────────────────────────────────────────────────────────── Append (cost=0.00..96.75 rows=50 width=4) -> Seq Scan on p11 (cost=0.00..48.25 rows=25 width=4) Filter: ((a = 25) OR (a = 5)) -> Seq Scan on p1_def (cost=0.00..48.25 rows=25 width=4) Filter: ((a = 25) OR (a = 5)) (5 rows) Here only one of the OR's arguments can be true for p1's partitions, but partprune.c's current usage of constraint exclusion fails to notice that. I had posted a patch [2] to solve this specific problem. Hosoya-san's patch is a generalization of my patch. Thanks, Amit [1] https://www.postgresql.org/message-id/bd03f475-30d4-c4d0-3d7f-d2fbde755971%40dalibo.com [2] https://www.postgresql.org/message-id/9bb31dfe-b0d0-53f3-3ea6-e64b811424cf%40lab.ntt.co.jp
Hello, I noticed that the patch is still marked as "Waiting on Author" ever since Shawn set it that way on June 17. Since Hosoya-san posted updated patches on June 27, the status should've been changed to "Needs Review". Or maybe "Ready for Committer", because the last time I looked, at least the default partition pruning issue seems to be sufficiently taken care of by the latest patch. Whether or not we should apply the other patch (more aggressive use of constraint exclusion by partprune.c on partitioned partitions), I'm not sure, but maybe a committer can decide in an instant. :) I've marked it RfC for now. Thanks, Amit
On 2019-Jul-31, Amit Langote wrote: > Hello, > > I noticed that the patch is still marked as "Waiting on Author" ever > since Shawn set it that way on June 17. Since Hosoya-san posted > updated patches on June 27, the status should've been changed to > "Needs Review". Or maybe "Ready for Committer", because the last time > I looked, at least the default partition pruning issue seems to be > sufficiently taken care of by the latest patch. Whether or not we > should apply the other patch (more aggressive use of constraint > exclusion by partprune.c on partitioned partitions), I'm not sure, but > maybe a committer can decide in an instant. :) Thanks for the status update. I intend to get this patch pushed before the next set of minors. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Jul 31, 2019 at 9:49 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > On 2019-Jul-31, Amit Langote wrote: > > I noticed that the patch is still marked as "Waiting on Author" ever > > since Shawn set it that way on June 17. Since Hosoya-san posted > > updated patches on June 27, the status should've been changed to > > "Needs Review". Or maybe "Ready for Committer", because the last time > > I looked, at least the default partition pruning issue seems to be > > sufficiently taken care of by the latest patch. Whether or not we > > should apply the other patch (more aggressive use of constraint > > exclusion by partprune.c on partitioned partitions), I'm not sure, but > > maybe a committer can decide in an instant. :) > > Thanks for the status update. I intend to get this patch pushed before > the next set of minors. Thank you Alvaro. Regards, Amit
On 2019-Jul-03, Amit Langote wrote:
> Hosoya-san,
>
> Thanks for updating the patches.
>
> I have no comment in particular about
> v4_default_partition_pruning.patch,
Cool, thanks. I spent some time reviewing this patch (the first one)
and I propose the attached cosmetic changes. Mostly they consist of a
few comment rewordings.
There is one Assert() that changed in a pretty significant way ...
innocent though the change looks. The original (not Hosoya-san's
patch's fault) had an assertion which is being changed thus:
minoff = 0;
maxoff = boundinfo->ndatums;
...
if (partindices[minoff] < 0)
minoff++;
if (partindices[maxoff] < 0)
maxoff--;
result->scan_default = partition_bound_has_default(boundinfo);
- Assert(minoff >= 0 && maxoff >= 0);
+ Assert(partindices[minoff] >= 0 &&
+ partindices[maxoff] >= 0);
Note that the original Assert() here was verifying whether minoff and
maxoff are both >= 0. But that seems pretty useless since it seems
almost impossible to have them become that given what we do to them.
What I think this code *really* wants to check is whether *the partition
indexes* that they point to are not negative: the transformation that
the two "if" lines do means to ignore bounds that correspond to value
ranges uncovered by any partition. And after the incr/decr operators,
we expect that the bounds will be those of existing partitions ... so
they shouldn't be -1.
Other changes are addition of braces to some one-line blocks that had
significant comments, and minor code rearrangements to make things look
more easily understandable.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
So this is the best commit messages I could come up with at this stupid hour. I think the wording is pretty poor but at least it seems correct. I'm not sure I'll be able to get this pushed tomorrow, but I'll try. Improve pruning of a default partition When querying a partitioned table containing a default partition, we were wrongly deciding to include it in the scan too early in the process, failing to exclude it in some cases. If we reinterpret the PruneStepResult.scan_default flag slightly, we can do a better job at detecting that it can be excluded. The change is that we avoid setting the flag for that pruning step unless the step absolutely requires the default partition to be scanned (in contrast with the previous arrangement, which was to set it unless the step was able to prune it). So get_matching_partitions() must explicitly check the partition that each returned bound value corresponds to in order to determine whether the default one needs to be included, rather than relying on the flag from the final step result. Author: Yuzuko Hosoya <hosoya.yuzuko@lab.ntt.co.jp> Reviewed-by: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> Discussion: https://postgr.es/m/00e601d4ca86$932b8bc0$b982a340$@lab.ntt.co.jp -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-Aug-04, Alvaro Herrera wrote: > So this is the best commit messages I could come up with at this stupid > hour. I think the wording is pretty poor but at least it seems correct. > I'm not sure I'll be able to get this pushed tomorrow, but I'll try. Pushed. Since this is Sunday before minors, I'll be checking buildfarm and will summarily revert if anything goes wrong. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
I propose the comment rewordings as attached. Mostly, this updates the comment atop the function to cover the case being modified, and then the new comment just refers to the new explicitly stated policy, so it bcomes simpler. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Hi Alvaro, On Mon, Aug 5, 2019 at 12:24 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > On 2019-Aug-04, Alvaro Herrera wrote: > > > So this is the best commit messages I could come up with at this stupid > > hour. I think the wording is pretty poor but at least it seems correct. > > I'm not sure I'll be able to get this pushed tomorrow, but I'll try. > > Pushed. Since this is Sunday before minors, I'll be checking buildfarm > and will summarily revert if anything goes wrong. Thanks for the revisions and committing. I can imagine the stress when writing that commit message, but it seems correct to me. Thanks to Hosoya-san for spotting the problem and working on the fix. It had occurred to me when reviewing this patch, prompted by Horiguchi-san's comment [1], that our design of PruneStepResult might not be so good. Especially, we don't really need to track whether the default partition needs to be scanned on a per-step basis. Maybe, the result of each step should be simply a Bitmapset storing the set of bound offsets. We then check in the end if any of the bound offsets in the final set (that is, after having executed all the steps) require scanning the default partition, very much like what we did in the committed patch. ISTM, scan_null wouldn't cause the same problems as scan_default did, so we can add the null_index to a given step's result set when executing the step. IOW, we can get rid of the unnecessary abstraction that is the PruneStepResult struct. Thanks, Amit [1] https://www.postgresql.org/message-id/20190319.152756.202071463.horiguchi.kyotaro%40lab.ntt.co.jp
Hi Alvaro and Amit, Thanks for reviewing and fixing the patch. Also, I confirmed the commit message explained the modification clearly. Thanks a lot. Yuzuko Hosoya On Mon, Aug 5, 2019 at 12:24 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > On 2019-Aug-04, Alvaro Herrera wrote: > > > So this is the best commit messages I could come up with at this stupid > > hour. I think the wording is pretty poor but at least it seems correct. > > I'm not sure I'll be able to get this pushed tomorrow, but I'll try. > > Pushed. Since this is Sunday before minors, I'll be checking buildfarm > and will summarily revert if anything goes wrong. > > -- > Álvaro Herrera https://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Best regards, Yuzuko Hosoya NTT Open Source Software Center
Hi Alvaro,
Thanks for reviewing.
The modification you made seems correct to me.
However, I'm still concerned that the block
-----
if (partconstr)
{
partconstr = (List *)
expression_planner((Expr *) partconstr);
if (context->rel->relid != 1)
ChangeVarNodes((Node *) partconstr, 1,
context->rel->relid, 0);
if (predicate_refuted_by(partconstr,
list_make1(clause),
false))
{
context->contradictory = true;
return NIL;
}
}
-----
is written in the right place as Amit explained [1].
At first, we tried to fix the following problematic query
which was reported by Thibaut before:
create table p (a int) partition by range (a);
create table p1 partition of p for values from (0) to (20) partition
by range (a);
create table p11 partition of p1 for values from (0) to (10);
create table p1_def partition of p1 default;
explain select * from p1 where a = 25 or a = 5;
QUERY PLAN
──────────────────────────────────────
Append (cost=0.00..96.75 rows=50 width=4)
-> Seq Scan on p11 (cost=0.00..48.25 rows=25 width=4)
Filter: ((a = 25) OR (a = 5))
-> Seq Scan on p1_def (cost=0.00..48.25 rows=25 width=4)
Filter: ((a = 25) OR (a = 5))
(5 rows)
And Amit proposed the patch to fix this problem[2].
In this patch, the above if() block was written in another place.
After that, I found the following query also doesn't work correctly:
explain select * from p1 where a = 25;
QUERY PLAN
───────────────────────────────────────
Append (cost=0.00..41.94 rows=13 width=4)
-> Seq Scan on p1_def (cost=0.00..41.88 rows=13 width=4)
Filter: (a = 25)
(3 rows)
So I proposed moving the if() block to the current place.
The latest patch can solve both queries but I found the latter
problem can be solved by setting constraint_exclusion = on.
Which approach will be suitable?
[1] https://www.postgresql.org/message-id/CA%2BHiwqG%2BnSD0vcJacArYgYcFVtpTJQ0fx1gBgoZkA_isKd6Z2w%40mail.gmail.com
[2] https://www.postgresql.org/message-id/9bb31dfe-b0d0-53f3-3ea6-e64b811424cf%40lab.ntt.co.jp
Best regards,
Yuzuko Hosoya
NTT Open Source Software Center
On Mon, Aug 5, 2019 at 11:03 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> I propose the comment rewordings as attached. Mostly, this updates the
> comment atop the function to cover the case being modified, and then the
> new comment just refers to the new explicitly stated policy, so it
> bcomes simpler.
>
> --
> Álvaro Herrera https://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Best regards,
Yuzuko Hosoya
NTT Open Source Software Center
On 2019-Aug-05, yuzuko wrote: > However, I'm still concerned that the block > ----- > ... > ----- > is written in the right place as Amit explained [1]. Yeah, I have that patch installed locally and I'm looking about it. Thanks for the reminder. I also have an eye on Horiguchi's patch elsewhere in the thread. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-Aug-05, yuzuko wrote: > So I proposed moving the if() block to the current place. > The latest patch can solve both queries but I found the latter > problem can be solved by setting constraint_exclusion = on. So we have three locations for that test; one is where it currently is, which handles a small subset of the cases. The other is where Amit first proposed putting it, which handles some additional cases; and the third one is where your latest patch puts it, which seems to handle all cases. Isn't that what Amit is saying? If that's correct (and that's what I want to imply with the comment changes I proposed), then we should just accept that version of the patch. I don't think that we care about what happens with constraint_exclusion is on. That's not the recommended value for that setting anyway. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi Alvaro, On Mon, Aug 5, 2019 at 11:39 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > On 2019-Aug-05, yuzuko wrote: > > > So I proposed moving the if() block to the current place. > > The latest patch can solve both queries but I found the latter > > problem can be solved by setting constraint_exclusion = on. > > So we have three locations for that test; one is where it currently is, > which handles a small subset of the cases. The other is where Amit > first proposed putting it, which handles some additional cases; and the > third one is where your latest patch puts it, which seems to handle all > cases. Isn't that what Amit is saying? If that's correct (and that's > what I want to imply with the comment changes I proposed), then we > should just accept that version of the patch. That's a correct summary, thanks. > I don't think that we care about what happens with constraint_exclusion > is on. That's not the recommended value for that setting anyway. One issue I expressed with unconditionally applying constraint exclusion in partprune.c the way the third patch proposes to do it is that it means we end up performing the same processing twice for a given relation in come cases. Specifically, when constraint_exclusion is set to on, relation_excluded_by_constraints() that occurs when set_rel_sizes() is applied to that relation would perform the same proof. But maybe we shouldn't worry about the repetition too much, because it's not likely to be very problematic in practice, considering that setting constraint_exclusion to on is not recommended. Thanks, Amit
On 2019-Aug-05, Alvaro Herrera wrote: > So we have three locations for that test; one is where it currently is, > which handles a small subset of the cases. The other is where Amit > first proposed putting it, which handles some additional cases; and the > third one is where your latest patch puts it, which seems to handle all > cases. Isn't that what Amit is saying? If that's correct (and that's > what I want to imply with the comment changes I proposed), then we > should just accept that version of the patch. ... actually, there's a fourth possible location, which is outside the per-partitioning-attribute loop. Nothing in the moved block is to be done per attribute, so it'd be wasted work AFAICS. I propose the attached. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Hello, On 2019-Aug-06, Amit Langote wrote: > On Mon, Aug 5, 2019 at 11:39 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > I don't think that we care about what happens with constraint_exclusion > > is on. That's not the recommended value for that setting anyway. > > One issue I expressed with unconditionally applying constraint > exclusion in partprune.c the way the third patch proposes to do it is > that it means we end up performing the same processing twice for a > given relation in come cases. Specifically, when constraint_exclusion > is set to on, relation_excluded_by_constraints() that occurs when > set_rel_sizes() is applied to that relation would perform the same > proof. But maybe we shouldn't worry about the repetition too much, > because it's not likely to be very problematic in practice, > considering that setting constraint_exclusion to on is not > recommended. Well, if this is really all that duplicative, one thing we could do is run this check in get_partprune_steps_internal only if constraint_exclusion is a value other than on; we should achieve the same effect with no repetition. Patch for that is attached. However, if I run the server with constraint_exclusion=on, the regression test fail with the attached diff. I didn't look at what test is failing, but it seems to me that it's not really duplicative in all cases, only some. Therefore we can't do it. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On 2019-Aug-06, Alvaro Herrera wrote: > Well, if this is really all that duplicative, one thing we could do is > run this check in get_partprune_steps_internal only if > constraint_exclusion is a value other than on; we should achieve the > same effect with no repetition. Patch for that is attached. However, > if I run the server with constraint_exclusion=on, the regression test > fail with the attached diff. I didn't look at what test is failing, but > it seems to me that it's not really duplicative in all cases, only some. > Therefore we can't do it. Right ... One of the failing cases is one that was benefitting from constraint_exclusion in the location where we were doing it previously. I think trying to fix this problem by selectively moving where to apply constraint exclusion would be very bug-prone, and hard to detect whether we're missing one spot or doing it multiple times in some other cases. So I think we shouldn't try. If this is a real problem, then we should add a flag to the reloptinfo and set it when we've done pruning, then do nothing if we already did it. I'm not sure that this is correct, and I'm even less sure that it is worth the trouble. In short, I propose to get this done as the patch I posted in https://postgr.es/m/20190806133053.GA23706@alvherre.pgsql Cheers -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hello, > > Well, if this is really all that duplicative, one thing we could do is > > run this check in get_partprune_steps_internal only if > > constraint_exclusion is a value other than on; we should achieve the > > same effect with no repetition. Patch for that is attached. However, > > if I run the server with constraint_exclusion=on, the regression test > > fail with the attached diff. I didn't look at what test is failing, but > > it seems to me that it's not really duplicative in all cases, only some. > > Therefore we can't do it. > > Right ... One of the failing cases is one that was benefitting from > constraint_exclusion in the location where we were doing it previously. > Thanks for testing. > I think trying to fix this problem by selectively moving where to apply > constraint exclusion would be very bug-prone, and hard to detect whether > we're missing one spot or doing it multiple times in some other cases. > So I think we shouldn't try. If this is a real problem, then we should > add a flag to the reloptinfo and set it when we've done pruning, then > do nothing if we already did it. I'm not sure that this is correct, and > I'm even less sure that it is worth the trouble. > Indeed, we should not do that from the viewpoint of cost-effectiveness. I think we can ignore the duplicate processing considering it doesn't happen in all cases. > In short, I propose to get this done as the patch I posted in > https://postgr.es/m/20190806133053.GA23706@alvherre.pgsql > I agree with your proposal. Also, I confirmed a default partition was pruned as expected with your patch. -- Best regards, Yuzuko Hosoya NTT Open Source Software Center
On Wed, Aug 7, 2019 at 3:30 PM yuzuko <yuzukohosoya@gmail.com> wrote: > > In short, I propose to get this done as the patch I posted in > > https://postgr.es/m/20190806133053.GA23706@alvherre.pgsql > > > I agree with your proposal. Also, I confirmed a default partition was pruned > as expected with your patch. +1. Thanks, Amit
On Tue, 6 Aug 2019 at 23:18, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2019-Aug-06, Alvaro Herrera wrote:
> Well, if this is really all that duplicative, one thing we could do is
> run this check in get_partprune_steps_internal only if
> constraint_exclusion is a value other than on; we should achieve the
> same effect with no repetition. Patch for that is attached. However,
> if I run the server with constraint_exclusion=on, the regression test
> fail with the attached diff. I didn't look at what test is failing, but
> it seems to me that it's not really duplicative in all cases, only some.
> Therefore we can't do it.
Right ... One of the failing cases is one that was benefitting from
constraint_exclusion in the location where we were doing it previously.
I think trying to fix this problem by selectively moving where to apply
constraint exclusion would be very bug-prone, and hard to detect whether
we're missing one spot or doing it multiple times in some other cases.
So I think we shouldn't try. If this is a real problem, then we should
add a flag to the reloptinfo and set it when we've done pruning, then
do nothing if we already did it. I'm not sure that this is correct, and
I'm even less sure that it is worth the trouble.
In short, I propose to get this done as the patch I posted in
https://postgr.es/m/20190806133053.GA23706@alvherre.pgsql
I saw your recent commit and it scares me in various places, noted below.
"Commit: Apply constraint exclusion more generally in partitioning"
"This applies particularly to the default partition..."
"This applies particularly to the default partition..."
My understanding of the thread was the complaint was about removing the default partition. I would prefer to see code executed just for that case, so that people who do not define a default partition are unaffected.
"So in certain cases
we're scanning partitions that we don't need to."
Avoiding that has been the subject of months of work.
"This has the unwanted side-effect of testing some (most? all?)
constraints more than once if constraint_exclusion=on. That seems
unavoidable as far as I can tell without some additional work, but
that's not the recommended setting for that parameter anyway.
However, because this imposes additional processing cost for all
queries using partitioned tables..."
we're scanning partitions that we don't need to."
Avoiding that has been the subject of months of work.
"This has the unwanted side-effect of testing some (most? all?)
constraints more than once if constraint_exclusion=on. That seems
unavoidable as far as I can tell without some additional work, but
that's not the recommended setting for that parameter anyway.
However, because this imposes additional processing cost for all
queries using partitioned tables..."
One of the major features of PG12 is the additional performance and scalability of partitoning, but we don't seem to have benchmarked the effect of this patch on those issues.
Please could we do perf checks, with tests up to 1000s of partitions? And if there is a regression, I would vote to revoke this patch or address the request in a less general way.
Hopefully I have misunderstood and/or there is no regression.
On 2019-Aug-07, Simon Riggs wrote: > I saw your recent commit and it scares me in various places, noted below. > > "Commit: Apply constraint exclusion more generally in partitioning" > > "This applies particularly to the default partition..." > > My understanding of the thread was the complaint was about removing the > default partition. I would prefer to see code executed just for that case, > so that people who do not define a default partition are unaffected. Well, as the commit message noted, it applies to other cases also, not just the default partition. The default partition just happens to be the most visible case. > "So in certain cases > we're scanning partitions that we don't need to." > > Avoiding that has been the subject of months of work. Well, yes, avoiding that is the point of this commit also: we were scanning some partitions for some queries, after this patch we're supposed not to. > "This has the unwanted side-effect of testing some (most? all?) > constraints more than once if constraint_exclusion=on. That seems > unavoidable as far as I can tell without some additional work, but > that's not the recommended setting for that parameter anyway. > However, because this imposes additional processing cost for all > queries using partitioned tables..." > > One of the major features of PG12 is the additional performance and > scalability of partitoning, but we don't seem to have benchmarked the > effect of this patch on those issues. > > Please could we do perf checks, with tests up to 1000s of partitions? And > if there is a regression, I would vote to revoke this patch or address the > request in a less general way. I'll have a look. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi Alvaro, On Thu, Aug 8, 2019 at 5:27 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > On 2019-Aug-07, Simon Riggs wrote: > > I saw your recent commit and it scares me in various places, noted below. > > > > "Commit: Apply constraint exclusion more generally in partitioning" > > > > "This applies particularly to the default partition..." > > > > My understanding of the thread was the complaint was about removing the > > default partition. I would prefer to see code executed just for that case, > > so that people who do not define a default partition are unaffected. > > Well, as the commit message noted, it applies to other cases also, not > just the default partition. The default partition just happens to be > the most visible case. Just to be clear, I don't think there was any patch posted on this thread that was to address *non-default* partitions failing to be pruned by "partition pruning". If that had been the problem, we'd be fixing the bugs of the partition pruning code, not apply constraint exclusion more generally to paper over such bugs. I may be misreading what you wrote here though. The way I interpret the "generally" in the "apply constraint exclusion more generally" is thus: we can't prune the default partition without the constraint exclusion clutches for evidently a broader sets of clauses than the previous design assumed. The previous design assumed that only OR clauses whose arguments contradicted the parent's partition constraint are problematic, but evidently any clause set that contradicts the partition constraint is problematic. Again, the problem is that it's impossible to prune the "default" partition with such clauses, not the *non-default* ones -- values extracted from contradictory clauses would not match any of the bounds so all non-default partitions would be pruned that way. By the way, looking closer at the patch committed today, I realized I had misunderstood what you proposed as the *4th* possible place to move the constraint exclusion check to. I had misread the proposal and thought you meant to move it outside the outermost loop of gen_partprune_steps_internal(), but that's not where the check is now. I think it's better to call predicate_refuted_by() only once by passing the whole list of clauses instead of for each clause separately. The result would be the same but the former would be more efficient, because it avoids repeatedly paying the cost of setting up predtest.c data structures when predicate_refuted_by() is called. Sorry that I'm only saying this now. Also it wouldn't be incorrect to do the check only if the parent has a default partition. That will also address the Simon's concern this might slow down the cases where this effort is useless. I've attached a patch that does that. When working on it, I realized that the way RelOptInfo.partition_qual is processed is a bit duplicative, so I created a separate patch to make that a bit more consistent. Thanks, Amit
Attachment
Hi Hosoya-san,
I am sorry for so late to reply to you.
I merged the patches into master(commit: 44460d7017cde005d7a2e246db0b32375bfec15d).
I tested the case I used in the previous patches and didn't find any issues.
Now I find that you are rethinking some of the details.
I will continue to pay attention to this and will follow up and feedback in time.
Regards,
--
Shawn Wang
---- On Thu, 27 Jun 2019 10:34:13 +0800 yuzuko <yuzukohosoya@gmail.com> wrote ----
Hello,
On Tue, Jun 25, 2019 at 1:45 PM yuzuko <yuzukohosoya@gmail.com> wrote:
>
> Hello Shawn, Alvaro,
>
> Thank you for testing patches and comments.
> Yes, there are two patches:
> (1) v4_default_partition_pruning.patch fixes problems with default
> partition pruning
> and (2) v3_ignore_contradictory_where_clauses_at_partprune_step.patch determines
> if a given clause contradicts a sub-partitioned table's partition constraint.
> I'll post two patches together next time.
>
> Anyway, I'll rebase two patches to apply on master and fix space.
>
Attach the latest patches discussed in this thread. I rebased the second
patch (v5_ignore_contradictory_where_clauses_at_partprune_step.patch)
on the current master. Could you please check them again?
--
Best regards,
Yuzuko Hosoya
NTT Open Source Software Center
On Wed, 7 Aug 2019 at 21:27, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
On 2019-Aug-07, Simon Riggs wrote:
> I saw your recent commit and it scares me in various places, noted below.
>
> "Commit: Apply constraint exclusion more generally in partitioning"
>
> "This applies particularly to the default partition..."
>
> My understanding of the thread was the complaint was about removing the
> default partition. I would prefer to see code executed just for that case,
> so that people who do not define a default partition are unaffected.
Well, as the commit message noted, it applies to other cases also, not
just the default partition. The default partition just happens to be
the most visible case.
> "So in certain cases
> we're scanning partitions that we don't need to."
>
> Avoiding that has been the subject of months of work.
Well, yes, avoiding that is the point of this commit also: we were
scanning some partitions for some queries, after this patch we're
supposed not to.
Understood
My concern was about the additional execution time caused when there would be no benefit, especially if the algoithmic cost is O(N) or similar (i.e. worse than O(k))
If people have a default partition, I have no problem in there being additional execution time in that case only since there is only ever one default partition.
> Please could we do perf checks, with tests up to 1000s of partitions? And
> if there is a regression, I would vote to revoke this patch or address the
> request in a less general way.
I'll have a look.
Thanks
Hi Simon, On Thu, Aug 8, 2019 at 4:54 PM Simon Riggs <simon@2ndquadrant.com> wrote: > On Wed, 7 Aug 2019 at 21:27, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: >> Well, yes, avoiding that is the point of this commit also: we were >> scanning some partitions for some queries, after this patch we're >> supposed not to. > > > Understood > > My concern was about the additional execution time caused when there would be no benefit, especially if the algoithmiccost is O(N) or similar (i.e. worse than O(k)) Note that we apply constraint exclusion only to the (sub-partitioned) parent, not to all partitions, so the cost is not O(N) in the number of partitions. The idea is that if the parent is excluded, all of its partitions are. We normally wouldn't need to use constrain exclusion, because partition pruning can handle most cases. What partition pruning can't handle sufficiently well though is the case where a clause set that contradicts the partition constraint is specified -- while all non-default partitions are correctly pruned, the default partition is not. Using constraint exclusion is a workaround for that deficiency of the partition pruning logic. Thanks, Amit
Hello, On Thu, Aug 8, 2019 at 5:09 PM Amit Langote <amitlangote09@gmail.com> wrote: > > Hi Simon, > > On Thu, Aug 8, 2019 at 4:54 PM Simon Riggs <simon@2ndquadrant.com> wrote: > > On Wed, 7 Aug 2019 at 21:27, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > >> Well, yes, avoiding that is the point of this commit also: we were > >> scanning some partitions for some queries, after this patch we're > >> supposed not to. > > > > > > Understood > > > > My concern was about the additional execution time caused when there would be no benefit, especially if the algoithmiccost is O(N) or similar (i.e. worse than O(k)) > > Note that we apply constraint exclusion only to the (sub-partitioned) > parent, not to all partitions, so the cost is not O(N) in the number > of partitions. The idea is that if the parent is excluded, all of its > partitions are. We normally wouldn't need to use constrain exclusion, > because partition pruning can handle most cases. What partition > pruning can't handle sufficiently well though is the case where a > clause set that contradicts the partition constraint is specified -- > while all non-default partitions are correctly pruned, the default > partition is not. Using constraint exclusion is a workaround for that > deficiency of the partition pruning logic. > Besides that, the additional code will not be executed if people don't define any default partition in the latest patch Amit proposed. So I think this patch has no effect such as Simon's concern. I looked at Amit's patches and found it worked correctly. -- Best regards, Yuzuko Hosoya NTT Open Source Software Center
Sorry for being late. I found it is committed before I caught up this thread again.. At Thu, 8 Aug 2019 14:50:54 +0900, Amit Langote <amitlangote09@gmail.com> wrote in <CA+HiwqEmJizJ4DmuPWCL-WrHGO-hFVd08TS7HnCkSF4CyZr8tg@mail.gmail.com> > Hi Alvaro, > > On Thu, Aug 8, 2019 at 5:27 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > On 2019-Aug-07, Simon Riggs wrote: > > > I saw your recent commit and it scares me in various places, noted below. > > > > > > "Commit: Apply constraint exclusion more generally in partitioning" > > > > > > "This applies particularly to the default partition..." > > > > > > My understanding of the thread was the complaint was about removing the > > > default partition. I would prefer to see code executed just for that case, > > > so that people who do not define a default partition are unaffected. > > > > Well, as the commit message noted, it applies to other cases also, not > > just the default partition. The default partition just happens to be > > the most visible case. > > Just to be clear, I don't think there was any patch posted on this > thread that was to address *non-default* partitions failing to be > pruned by "partition pruning". If that had been the problem, we'd be > fixing the bugs of the partition pruning code, not apply constraint > exclusion more generally to paper over such bugs. I may be misreading > what you wrote here though. > > The way I interpret the "generally" in the "apply constraint exclusion > more generally" is thus: we can't prune the default partition without > the constraint exclusion clutches for evidently a broader sets of > clauses than the previous design assumed. The previous design assumed > that only OR clauses whose arguments contradicted the parent's > partition constraint are problematic, but evidently any clause set > that contradicts the partition constraint is problematic. Again, the > problem is that it's impossible to prune the "default" partition with > such clauses, not the *non-default* ones -- values extracted from > contradictory clauses would not match any of the bounds so all > non-default partitions would be pruned that way. > > By the way, looking closer at the patch committed today, I realized I > had misunderstood what you proposed as the *4th* possible place to > move the constraint exclusion check to. I had misread the proposal > and thought you meant to move it outside the outermost loop of > gen_partprune_steps_internal(), but that's not where the check is now. > I think it's better to call predicate_refuted_by() only once by > passing the whole list of clauses instead of for each clause > separately. The result would be the same but the former would be more > efficient, because it avoids repeatedly paying the cost of setting up > predtest.c data structures when predicate_refuted_by() is called. > Sorry that I'm only saying this now. +1 as I mentioned in [1]. > Also it wouldn't be incorrect to do the check only if the parent has a > default partition. That will also address the Simon's concern this > might slow down the cases where this effort is useless. > > I've attached a patch that does that. When working on it, I realized > that the way RelOptInfo.partition_qual is processed is a bit > duplicative, so I created a separate patch to make that a bit more > consistent. 0001 seems reasonable. By the way, the patch doesn't touch get_relation_constraints(), but I suppose it can use the modified partition constraint qual already stored in rel->partition_qual in set_relation_partition_info. And we could move constifying to set_rlation_partition_info? Also, I'd like to see comments that the partition_quals is already varnode-fixed. And 0002, yeah, just +1 from me. [1] https://www.postgresql.org/message-id/20190409.173725.31175835.horiguchi.kyotaro@lab.ntt.co.jp regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Horiguchi-san, Thanks for the review. On Fri, Aug 9, 2019 at 12:09 PM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote: > At Thu, 8 Aug 2019 14:50:54 +0900, Amit Langote wrote: > > When working on it, I realized > > that the way RelOptInfo.partition_qual is processed is a bit > > duplicative, so I created a separate patch to make that a bit more > > consistent. > > 0001 seems reasonable. By the way, the patch doesn't touch > get_relation_constraints(), but I suppose it can use the modified > partition constraint qual already stored in rel->partition_qual > in set_relation_partition_info. And we could move constifying to > set_rlation_partition_info? Ah, good advice. This make partition constraint usage within the planner quite a bit more consistent. > Also, I'd like to see comments that the partition_quals is > already varnode-fixed. Added a one-line comment. > And 0002, yeah, just +1 from me. Thanks. Attached updated patches; only 0001 changed per above comments. Regards, Amit
Attachment
On Fri, Aug 9, 2019 at 1:17 PM Amit Langote <amitlangote09@gmail.com> wrote: > On Fri, Aug 9, 2019 at 12:09 PM Kyotaro Horiguchi > <horikyota.ntt@gmail.com> wrote: > > At Thu, 8 Aug 2019 14:50:54 +0900, Amit Langote wrote: > > > When working on it, I realized > > > that the way RelOptInfo.partition_qual is processed is a bit > > > duplicative, so I created a separate patch to make that a bit more > > > consistent. > > > > 0001 seems reasonable. By the way, the patch doesn't touch > > get_relation_constraints(), but I suppose it can use the modified > > partition constraint qual already stored in rel->partition_qual > > in set_relation_partition_info. And we could move constifying to > > set_rlation_partition_info? > > Ah, good advice. This make partition constraint usage within the > planner quite a bit more consistent. Hmm, oops. I think that judgement was a bit too rushed on my part. I unintentionally ended up making the partition constraint to *always* be fetched, whereas we don't need it in most cases. I've reverted that change. RelOptInfo.partition_qual is poorly named in retrospect. :( It's not set for all partitions, only those that are partitioned themselves. Attached updated patches. Thanks, Amit
Attachment
At Fri, 9 Aug 2019 14:02:36 +0900, Amit Langote <amitlangote09@gmail.com> wrote in
<CA+HiwqGm18B8UQ5Sip_nsNYmDiHtoaVORvCPumo_bbXTXHPRBw@mail.gmail.com>
> On Fri, Aug 9, 2019 at 1:17 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Fri, Aug 9, 2019 at 12:09 PM Kyotaro Horiguchi
> > <horikyota.ntt@gmail.com> wrote:
> > > At Thu, 8 Aug 2019 14:50:54 +0900, Amit Langote wrote:
> > > > When working on it, I realized
> > > > that the way RelOptInfo.partition_qual is processed is a bit
> > > > duplicative, so I created a separate patch to make that a bit more
> > > > consistent.
> > >
> > > 0001 seems reasonable. By the way, the patch doesn't touch
> > > get_relation_constraints(), but I suppose it can use the modified
> > > partition constraint qual already stored in rel->partition_qual
> > > in set_relation_partition_info. And we could move constifying to
> > > set_rlation_partition_info?
> >
> > Ah, good advice. This make partition constraint usage within the
> > planner quite a bit more consistent.
>
> Hmm, oops. I think that judgement was a bit too rushed on my part. I
> unintentionally ended up making the partition constraint to *always*
> be fetched, whereas we don't need it in most cases. I've reverted
(v2 has been withdrawn before I see it:p)
Yeah, I agreed. It is needed only by (sub)partition parents.
> that change. RelOptInfo.partition_qual is poorly named in retrospect.
> :( It's not set for all partitions, only those that are partitioned
> themselves.
>
> Attached updated patches.
+++ b/src/backend/optimizer/util/plancat.c
@@ -1267,10 +1267,14 @@ get_relation_constraints(PlannerInfo *root,
*/
if (include_partition && relation->rd_rel->relispartition)
{
...
+ else
{
+ /* Nope, fetch from the relcache. */
I seems to me that include_partition is true both and only for
modern and old-fasheoned partition parents.
set_relation_partition_info() is currently called only for modern
partition parents. If we need that at the place above,
set_relation_partition_info can be called also for old-fashioned
partition parent, and get_relation_constraints may forget the
else case in a broad way.
+ /* Nope, fetch from the relcache. */
+ List *pcqual = RelationGetPartitionQual(relation);
If the comment above is right, This would be duplicate. What we
should do instaed is only eval_const_expression. And we could
move it to set_relation_partition_info completely. Consitify must
be useful in both cases.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Thanks for the comments.
On Fri, Aug 9, 2019 at 2:44 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> +++ b/src/backend/optimizer/util/plancat.c
> @@ -1267,10 +1267,14 @@ get_relation_constraints(PlannerInfo *root,
> */
> if (include_partition && relation->rd_rel->relispartition)
> {
> ...
> + else
> {
> + /* Nope, fetch from the relcache. */
>
> I seems to me that include_partition is true both and only for
> modern and old-fasheoned partition parents.
> set_relation_partition_info() is currently called only for modern
> partition parents. If we need that at the place above,
> set_relation_partition_info can be called also for old-fashioned
> partition parent, and get_relation_constraints may forget the
> else case in a broad way.
"include_partition" doesn't have anything to do with what kind the
partition parent is. It is true when the input relation that is a
partition is directly mentioned in the query (RELOPT_BASEREL) and
constraint_exclusion is on (inheritance_planner considerations makes
the actual code a bit hard to follow but we'll hopefully simplify that
in the near future). That is also the only case where we need to
consider the partition constraint when doing constraint exclusion.
Regarding how this relates to partition_qual:
* get_relation_constraints() can use it if it's set, which would be
the case if the partition in question is partitioned itself
* It wouldn't be set if the partition in question is a leaf partition,
so it will have to get it directly from the relcache
> + /* Nope, fetch from the relcache. */
> + List *pcqual = RelationGetPartitionQual(relation);
>
> If the comment above is right, This would be duplicate. What we
> should do instaed is only eval_const_expression. And we could
> move it to set_relation_partition_info completely. Consitify must
> be useful in both cases.
As described above, this block of code is not really duplicative in
the sense that when it runs, that would be the first time in a query
to fetch the partition constraint of the relation in question.
Also, note that expression_planner() calls eval_const_expressions(),
so constification happens in both cases. I guess different places
have grown different styles of processing constraint expressions as
the APIs have evolved over time.
Thanks,
Amit
On 2019-Aug-09, Amit Langote wrote: > Hmm, oops. I think that judgement was a bit too rushed on my part. I > unintentionally ended up making the partition constraint to *always* > be fetched, whereas we don't need it in most cases. I've reverted > that change. Yeah, I was quite confused about this point yesterday while I was trying to make sense of your patches. > RelOptInfo.partition_qual is poorly named in retrospect. > :( It's not set for all partitions, only those that are partitioned > themselves. Oh. Hmm, I think this realization further clarifies things. Since we're only changing this in the master branch anyway, maybe we can find a better name for it. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
v3-0001 still seems to leave things a bit duplicative. I think we can make it better if we move the logic to set RelOptInfo->partition_qual to a separate routine (set_baserel_partition_constraint mirroring the existing set_baserel_partition_key_exprs), and then call that from both places that need access to partition_qual. So I propose that the attached v4 patch should be the final form of this (also rebased across today's list_concat API change). I verified that constraint exclusion is not being called by partprune unless a default partition exists (thanks errbacktrace()); I think that should appease Simon's performance concern for the most common case of default partition not existing. I think I was not really understanding the comments being added by Amit's v3, so I reworded them. I hope I understood the intent of the code correctly. I'm not comfortable with RelOptInfo->partition_qual. But I'd rather leave that for another time. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On Mon, 12 Aug 2019 at 18:45, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
I think that should appease
Simon's performance concern for the most common case of default
partition not existing.
Much appreciated, thank you.
Hi Alvaro, On Tue, Aug 13, 2019 at 2:45 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > v3-0001 still seems to leave things a bit duplicative. I think we can > make it better if we move the logic to set RelOptInfo->partition_qual to > a separate routine (set_baserel_partition_constraint mirroring the > existing set_baserel_partition_key_exprs), and then call that from both > places that need access to partition_qual. > > So I propose that the attached v4 patch should be the final form of this > (also rebased across today's list_concat API change). I verified that > constraint exclusion is not being called by partprune unless a default > partition exists (thanks errbacktrace()); I think that should appease > Simon's performance concern for the most common case of default > partition not existing. > > I think I was not really understanding the comments being added by > Amit's v3, so I reworded them. I hope I understood the intent of the > code correctly. Thanks a lot for revising. Looks neat, except: + * This is a measure of last resort only to be used because the default + * partition cannot be pruned using the steps; regular pruning, which is + * cheaper, is sufficient when no default partition exists. This text appears to imply that the default can *never* be pruned with steps. Maybe, the first sentence should read something like: "...the default cannot be pruned using the steps generated from clauses that contradict the parent's partition constraint". > I'm not comfortable with RelOptInfo->partition_qual. But I'd rather > leave that for another time. Sure. Regards, Amit
On 2019-Aug-13, Amit Langote wrote: > Thanks a lot for revising. Looks neat, except: > > + * This is a measure of last resort only to be used because the default > + * partition cannot be pruned using the steps; regular pruning, which is > + * cheaper, is sufficient when no default partition exists. > > This text appears to imply that the default can *never* be pruned with > steps. Maybe, the first sentence should read something like: "...the > default cannot be pruned using the steps generated from clauses that > contradict the parent's partition constraint". Thanks! I have pushed it with this change. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Aug 14, 2019 at 12:25 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > On 2019-Aug-13, Amit Langote wrote: > > > Thanks a lot for revising. Looks neat, except: > > > > + * This is a measure of last resort only to be used because the default > > + * partition cannot be pruned using the steps; regular pruning, which is > > + * cheaper, is sufficient when no default partition exists. > > > > This text appears to imply that the default can *never* be pruned with > > steps. Maybe, the first sentence should read something like: "...the > > default cannot be pruned using the steps generated from clauses that > > contradict the parent's partition constraint". > > Thanks! I have pushed it with this change. Thank you Alvaro. This takes care of all the issues around default partition pruning reported on this thread. Thanks everyone. Regards, Amit