speeding up planning with partitions

On 30 August 2018 at 00:06, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:

nparts master 0001 0002 0003
====== ====== ==== ==== ====
0 2856 2893 2862 2816
8 507 1115 1447 1872
16 260 765 1173 1892
32 119 483 922 1884
64 59 282 615 1881
128 29 153 378 1835
256 14 79 210 1803
512 5 40 113 1728
1024 2 17 57 1616
2048 0* 9 30 1471
4096 0+ 4 15 1236
8192 0= 2 7 975

Those look promising. Are you going to submit these patches to the
September 'fest?

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

From: Amit Langote [mailto:Langote_Amit_f8@lab.ntt.co.jp]

I measured the gain in performance due to each patch on a modest virtual
machine. Details of the measurement and results follow.

Amazing!

UPDATE

nparts master 0001 0002 0003
====== ====== ==== ==== ====
0 2856 2893 2862 2816
8 507 1115 1447 1872

SELECT

nparts master 0001 0002 0003
====== ====== ==== ==== ====
0 2290 2329 2319 2268
8 1058 1077 1414 1788

Even a small number of partitions still introduces a not-small overhead (UPDATE:34%, SELECT:22%). Do you think this overhead can be reduced further? What part do you guess would be relevant? This one?

that it is now performed after pruning. However, it doesn't do anything
about the fact that partitions are all still locked in the earlier phase.

Regards
Takayuki Tsunakawa

On 2018/08/30 7:27, David Rowley wrote:

On 30 August 2018 at 00:06, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:

nparts master 0001 0002 0003
====== ====== ==== ==== ====
0 2856 2893 2862 2816
8 507 1115 1447 1872
16 260 765 1173 1892
32 119 483 922 1884
64 59 282 615 1881
128 29 153 378 1835
256 14 79 210 1803
512 5 40 113 1728
1024 2 17 57 1616
2048 0* 9 30 1471
4096 0+ 4 15 1236
8192 0= 2 7 975

Those look promising. Are you going to submit these patches to the
September 'fest?

Thanks, I just did.

https://commitfest.postgresql.org/19/1778/

Regards,
Amit

On 2018/08/30 10:09, Tsunakawa, Takayuki wrote:

From: Amit Langote [mailto:Langote_Amit_f8@lab.ntt.co.jp]

I measured the gain in performance due to each patch on a modest virtual
machine. Details of the measurement and results follow.

Amazing!

Thanks.

UPDATE

nparts master 0001 0002 0003
====== ====== ==== ==== ====
0 2856 2893 2862 2816
8 507 1115 1447 1872

SELECT

nparts master 0001 0002 0003
====== ====== ==== ==== ====
0 2290 2329 2319 2268
8 1058 1077 1414 1788

Even a small number of partitions still introduces a not-small overhead (UPDATE:34%, SELECT:22%).

Yeah, that's true.

Do you think this overhead can be reduced further?

We can definitely try, but I'm not immediately sure if the further
improvements will come from continuing to fix the planner. Maybe, the
overhead of partitioning could be attributed to other parts of the system.

What part do you guess would be relevant? This one?

that it is now performed after pruning. However, it doesn't do anything
about the fact that partitions are all still locked in the earlier phase.

Actually, I wrote that for patch 0002. The next patch (0003) is meant to
fix that. So, the overhead you're seeing is even after making sure that
only the selected partitions are locked.

Thanks,
Amit

From: Amit Langote [mailto:Langote_Amit_f8@lab.ntt.co.jp]

We can definitely try, but I'm not immediately sure if the further
improvements will come from continuing to fix the planner. Maybe, the
overhead of partitioning could be attributed to other parts of the system.

Actually, I wrote that for patch 0002. The next patch (0003) is meant to
fix that. So, the overhead you're seeing is even after making sure that
only the selected partitions are locked.

Thanks for telling your thought. I understood we should find the bottleneck with profiling first.

Regards
Takayuki Tsunakawa

On 2018/08/29 21:06, Amit Langote wrote:

I measured the gain in performance due to each patch on a modest virtual
machine. Details of the measurement and results follow.

UPDATE:

nparts master 0001 0002 0003
====== ====== ==== ==== ====
0 2856 2893 2862 2816
8 507 1115 1447 1872
16 260 765 1173 1892
32 119 483 922 1884
64 59 282 615 1881
128 29 153 378 1835
256 14 79 210 1803
512 5 40 113 1728
1024 2 17 57 1616
2048 0* 9 30 1471
4096 0+ 4 15 1236
8192 0= 2 7 975

For SELECT:

nparts master 0001 0002 0003
====== ====== ==== ==== ====
0 2290 2329 2319 2268
8 1058 1077 1414 1788
16 711 729 1124 1789
32 450 475 879 1773
64 265 272 603 1765
128 146 149 371 1685
256 76 77 214 1678
512 39 39 112 1636
1024 16 17 59 1525
2048 8 9 29 1416
4096 4 4 15 1195
8192 2 2 7 932

Prompted by Tsunakawa-san's comment, I tried to look at the profiles when
running the benchmark with partitioning and noticed a few things that made
clear why, even with 0003 applied, tps numbers decreased as the number of
partitions increased. Some functions that appeared high up in the
profiles were related to partitioning:

* set_relation_partition_info calling partition_bounds_copy(), which calls
datumCopy() on N Datums, where N is the number of partitions. The more
the number of partitions, higher up it is in profiles. I suspect that
this copying might be redundant; planner can keep using the same pointer
as relcache

There are a few existing and newly introduced sites in the planner where
the code iterates over *all* partitions of a table where processing just
the partition selected for scanning would suffice. I observed the
following functions in profiles:

* make_partitionedrel_pruneinfo, which goes over all partitions to
generate subplan_map and subpart_map arrays to put into the
PartitionedRelPruneInfo data structure that it's in the charge of
generating

* apply_scanjoin_target_to_paths, which goes over all partitions to adjust
their Paths for applying required scanjoin target, although most of
those are dummy ones that won't need the adjustment

* For UPDATE, a couple of functions I introduced in patch 0001 were doing
the same thing as apply_scanjoin_target_to_paths, which is unnecessary

To fix the above three instances of redundant processing, I added a
Bitmapset 'live_parts' to the RelOptInfo which stores the set of indexes
of only the unpruned partitions (into the RelOptInfo.part_rels array) and
replaced the for (i = 0; i < rel->nparts; i++) loops in those sites with
the loop that iterates over the members of 'live_parts'.

Results looked were promising indeed, especially after applying 0003 which
gets rid of locking all partitions.

UPDATE:

nparts master 0001 0002 0003
====== ====== ==== ==== ====
0 2856 2893 2862 2816
8 507 1115 1466 1845
16 260 765 1161 1876
32 119 483 910 1862
64 59 282 609 1895
128 29 153 376 1884
256 14 79 212 1874
512 5 40 115 1859
1024 2 17 58 1847
2048 0 9 29 1883
4096 0 4 15 1867
8192 0 2 7 1826

SELECT:

nparts master 0001 0002 0003
====== ====== ==== ==== ====
0 2290 2329 2319 2268
8 1058 1077 1431 1800
16 711 729 1158 1781
32 450 475 908 1777
64 265 272 612 1791
128 146 149 379 1777
256 76 77 213 1785
512 39 39 114 1776
1024 16 17 59 1756
2048 8 9 30 1746
4096 4 4 15 1722
8192 2 2 7 1706

Note that with 0003, tps doesn't degrade as the number of partitions increase.

Attached updated patches, with 0002 containing the changes mentioned above.

Thanks,
Amit

Hi, Amit

Great!
With the total number of records being 6400, I benchmarked while increasing the number of partitions from 100 to 6400.
Applying three all patches, 20% performance improved for 100 partitions.

I have the same number of records for each partition, do you do the same?

Also, in my case, performance was better when not prepare.
I think these patches do not improve execute case, so we need faster runtime pruning patch[1]/messages/by-id/CAKJS1f_QN-nmf6jCQ4gRU_8ab0zrd0ms-U=_Dj0KUARJiuGpOA@mail.gmail.com, right?

Details of measurement conditions and results are as follows.

- base source
master(@777e6ddf17) + Speeding up Insert v8 patch[1]/messages/by-id/CAKJS1f_QN-nmf6jCQ4gRU_8ab0zrd0ms-U=_Dj0KUARJiuGpOA@mail.gmail.com

- table definition(e.g. 100 partition)
create table test.accounts(aid serial, abalance int) partition by range(aid);
create table test.accounts_history(id serial, aid int, delta int, mtime timestamp without time zone) partition by range(aid);

create table test.account_part_1 partition of test.accounts for values from (1) to (65);
create table test.account_part_2 partition of test.accounts for values from (65) to (129);
...
create table test.account_part_100 partition of test.accounts for values from (6337) to (6400);

create table test.ah_part_1 partition of test.accounts_history for values from (1) to (65);
create table test.ah_part_2 partition of test.accounts_history for values from (65) to (129);
...
create table test.ah_part_100 partition of test.accounts_history for values from (6337) to (6400);

- benchmark script
\set aid random(1, 6400)
\set delta random(-5000, 5000)
BEGIN;
UPDATE test.accounts SET abalance = abalance + :delta WHERE aid = :aid;
SELECT abalance FROM test.accounts WHERE aid = :aid;
INSERT INTO test.accounts_history (aid, delta, mtime) VALUES (:aid, :delta, CURRENT_TIMESTAMP);
END;

- command option
pgbench -d testdb -f benchmark.pgbench -T 180 -r -n -M prepare
pgbench -d testdb -f benchmark.pgbench -T 180 -r -n

-results
base source no prepared
part_num | tps_ex | update_latency | select_latency | insert_latency
----------+------------+----------------+----------------+----------------
100 | 662.414805 | 0.357 | 0.265 | 0.421
200 | 494.478431 | 0.439 | 0.349 | 0.579
400 | 307.982089 | 0.651 | 0.558 | 0.927
800 | 191.360676 | 0.979 | 0.876 | 1.548
1600 | 75.344947 | 2.253 | 2.003 | 4.301
3200 | 30.643902 | 5.716 | 4.955 | 10.118
6400 | 16.726056 | 12.512 | 8.582 | 18.054

0001 no prepared
part_num | tps_ex | update_latency | select_latency | insert_latency
----------+------------+----------------+----------------+----------------
100 | 429.816329 | 0.811 | 0.75 | 0.365
200 | 275.211531 | 1.333 | 1.248 | 0.501
400 | 160.499833 | 2.384 | 2.252 | 0.754
800 | 79.387776 | 4.935 | 4.698 | 1.468
1600 | 24.787377 | 16.593 | 15.954 | 4.302
3200 | 9.846421 | 42.96 | 42.139 | 8.848
6400 | 4.919772 | 87.43 | 83.109 | 16.56

0001 prepared
part_num | tps_ex | update_latency | select_latency | insert_latency
----------+------------+----------------+----------------+----------------
100 | 245.100776 | 2.728 | 0.374 | 0.476
200 | 140.249283 | 5.116 | 0.603 | 0.686
400 | 67.608559 | 11.383 | 1.055 | 1.179
800 | 23.085806 | 35.781 | 2.585 | 2.677
1600 | 6.211247 | 141.093 | 7.096 | 6.785
3200 | 1.808214 | 508.045 | 15.741 | 13.243
6400 | 0.495635 | 1919.362 | 37.691 | 28.177

0001 + 0002 no prepared
part_num | tps_ex | update_latency | select_latency | insert_latency
----------+------------+----------------+----------------+----------------
100 | 682.53091 | 0.388 | 0.35 | 0.35
200 | 469.906601 | 0.543 | 0.496 | 0.51
400 | 321.915349 | 0.78 | 0.721 | 0.752
800 | 201.620975 | 1.246 | 1.156 | 1.236
1600 | 94.438204 | 2.612 | 2.335 | 2.745
3200 | 38.292922 | 6.657 | 5.579 | 6.808
6400 | 21.48462 | 11.989 | 10.104 | 12.601

0001 + 0002 prepared
part_num | tps_ex | update_latency | select_latency | insert_latency
----------+------------+----------------+----------------+----------------
100 | 591.10863 | 0.433 | 0.342 | 0.422
200 | 393.223638 | 0.625 | 0.522 | 0.614
400 | 253.672736 | 0.946 | 0.828 | 0.928
800 | 146.840262 | 1.615 | 1.448 | 1.604
1600 | 52.805593 | 4.656 | 3.811 | 4.473
3200 | 21.461606 | 11.48 | 9.56 | 10.661
6400 | 11.888232 | 22.869 | 16.841 | 18.871

0001 + 0002 + 0003 no prepared
part_num | tps_ex | update_latency | select_latency | insert_latency
----------+------------+----------------+----------------+----------------
100 | 798.962029 | 0.304 | 0.267 | 0.339
200 | 577.893396 | 0.384 | 0.346 | 0.487
400 | 426.542177 | 0.472 | 0.435 | 0.717
800 | 288.616213 | 0.63 | 0.591 | 1.162
1600 | 154.247034 | 1.056 | 0.987 | 2.384
3200 | 59.711446 | 2.416 | 2.233 | 6.514
6400 | 37.109761 | 3.387 | 3.099 | 11.762

0001 + 0002 + 0003 prepared
part_num | tps_ex | update_latency | select_latency | insert_latency
----------+------------+----------------+----------------+----------------
100 | 662.414805 | 0.357 | 0.265 | 0.421
200 | 494.478431 | 0.439 | 0.349 | 0.579
400 | 307.982089 | 0.651 | 0.558 | 0.927
800 | 191.360676 | 0.979 | 0.876 | 1.548
1600 | 75.344947 | 2.253 | 2.003 | 4.301
3200 | 30.643902 | 5.716 | 4.955 | 10.118
6400 | 16.726056 | 12.512 | 8.582 | 18.054

Although it may not be related to this, when measured with pg11 beta2, somehow the performance was better.

11beta2 + v1-0001-Speed-up-INSERT-and-UPDATE-on-partitioned-tables.patch[3]/messages/by-id/CAKJS1f_1RJyFquuCKRFHTdcXqoPX-PYqAd7nz=GVBwvGh4a6xA@mail.gmail.com prepared
part_num | tps_ex | update_latency | select_latency | insert_latency
----------+-------------+----------------+----------------+----------------
100 | 756.07228 | 0.942 | 0.091 | 0.123

[1]: /messages/by-id/CAKJS1f_QN-nmf6jCQ4gRU_8ab0zrd0ms-U=_Dj0KUARJiuGpOA@mail.gmail.com
[2]: /messages/by-id/CAKJS1f9T_32Xpb-p8cWwo5ezSfVhXviUW8QTWncP8ksPHDRK8g@mail.gmail.com
[3]: /messages/by-id/CAKJS1f_1RJyFquuCKRFHTdcXqoPX-PYqAd7nz=GVBwvGh4a6xA@mail.gmail.com

regards,
Sho Kato
-----Original Message-----
From: Amit Langote [mailto:Langote_Amit_f8@lab.ntt.co.jp]
Sent: Wednesday, August 29, 2018 9:06 PM
To: Pg Hackers <pgsql-hackers@postgresql.org>
Subject: speeding up planning with partitions

It is more or less well known that the planner doesn't perform well with more than a few hundred partitions even when only a handful of partitions are ultimately included in the plan. Situation has improved a bit in PG
11 where we replaced the older method of pruning partitions one-by-one using constraint exclusion with a much faster method that finds relevant partitions by using partitioning metadata. However, we could only use it for SELECT queries, because UPDATE/DELETE are handled by a completely different code path, whose structure doesn't allow it to call the new pruning module's functionality. Actually, not being able to use the new pruning is not the only problem for UPDATE/DELETE, more on which further below.

While situation improved with new pruning where it could, there are still overheads in the way planner handles partitions. As things stand today, it will spend cycles and allocate memory for partitions even before pruning is performed, meaning most of that effort could be for partitions that were better left untouched. Currently, planner will lock, heap_open
*all* partitions, create range table entries and AppendRelInfos for them, and finally initialize RelOptInfos for them, even touching the disk file of each partition in the process, in an earlier phase of planning. All of that processing is vain for partitions that are pruned, because they won't be included in the final plan. This problem grows worse as the number of partitions grows beyond thousands, because range table grows too big.

That could be fixed by delaying all that per-partition activity to a point where pruning has already been performed, so that we know the partitions to open and create planning data structures for, such as somewhere downstream to query_planner. But before we can do that we must do something about the fact that UPDATE/DELETE won't be able to cope with that because the code path that currently handles the planning of UPDATE/DELETE on partitioned tables (inheritance_planner called from
subquery_planner) relies on AppendRelInfos for all partitions having been initialized by an earlier planning phase. Delaying it to query_planner would be too late, because inheritance_planner calls query_planner for each partition, not for the parent. That is, if query_planner, which is downstream to inheritance_planner, was in the charge of determining which partitions to open, the latter wouldn't know which partitions to call the former for. :)

That would be fixed if there is no longer this ordering dependency, which is what I propose to do with the attached patch 0001. I've tried to describe how the patch manages to do that in its commit message, but I'll summarize here. As things stand today, inheritance_planner modifies the query for each leaf partition to make the partition act as the query's result relation instead of the original partitioned table and calls grouping_planner on the query. That means anything that's joined to partitioned table looks to instead be joined to the partition and join paths are generated likewise. Also, the resulting path's targetlist is adjusted to be suitable for the result partition. Upon studying how this works, I concluded that the same result can be achieved if we call grouping_planner only once and repeat the portions of query_planner's and grouping_planner's processing that generate the join paths and appropriate target list, respectively, for each partition. That way, we can rely on query_planner determining result partitions for us, which in turn relies on the faster partprune.c based method of pruning. That speeds things up in two ways. Faster pruning and we no longer repeat common processing for each partition.

With 0001 in place, there is nothing that requires that partitions be opened by an earlier planning phase, so, I propose patch 0002, which refactors the opening and creation of planner data structures for partitions such that it is now performed after pruning. However, it doesn't do anything about the fact that partitions are all still locked in the earlier phase.

With various overheads gone thanks to 0001 and 0002, locking of all partitions via find_all_inheritos can be seen as the single largest bottleneck, which 0003 tries to address. I've kept it a separate patch, because I'll need to think a bit more to say that it's actually to safe to defer locking to late planning, due mainly to the concern about the change in the order of locking from the current method. I'm attaching it here, because I also want to show the performance improvement we can expect with it.

I measured the gain in performance due to each patch on a modest virtual machine. Details of the measurement and results follow.

* Benchmark scripts

update.sql
update ht set a = 0 where b = 1;

select.sql
select * from ht where b = 1;

* Table:

create table ht (a int, b int) partition by hash (b) create table ht_1 partition of ht for values with (modulus N, remainder 0) ..
create table ht_N partition of ht for values with (modulus N, remainder N-1)

* Rounded tps with update.sql and select.sql against regular table (nparts = 0) and partitioned table with various partition counts:

pgbench -n -T 60 -f update.sql

nparts master 0001 0002 0003
====== ====== ==== ==== ====
0 2856 2893 2862 2816
8 507 1115 1447 1872
16 260 765 1173 1892
32 119 483 922 1884
64 59 282 615 1881
128 29 153 378 1835
256 14 79 210 1803
512 5 40 113 1728
1024 2 17 57 1616
2048 0* 9 30 1471
4096 0+ 4 15 1236
8192 0= 2 7 975

* 0.46
+ 0.0064
= 0 (OOM on a virtual machine with 4GB RAM)

As can be seen here, 0001 is a big help for update queries.

pgbench -n -T 60 -f select.sql

For a select query that doesn't contain join and needs to scan only one
partition:

nparts master 0001 0002 0003
====== ====== ==== ==== ====
0 2290 2329 2319 2268
8 1058 1077 1414 1788
16 711 729 1124 1789
32 450 475 879 1773
64 265 272 603 1765
128 146 149 371 1685
256 76 77 214 1678
512 39 39 112 1636
1024 16 17 59 1525
2048 8 9 29 1416
4096 4 4 15 1195
8192 2 2 7 932

Actually, here we get almost same numbers with 0001 as with master, because 0001 changes nothing for SELECT queries. We start seeing improvement with 0002, the patch to delay opening partitions.

Thanks,
Amit

Thank you Kato-san for testing.

On 2018/08/31 19:48, Kato, Sho wrote:

Hi, Amit

Great!
With the total number of records being 6400, I benchmarked while increasing the number of partitions from 100 to 6400.
Applying three all patches, 20% performance improved for 100 partitions.

I have the same number of records for each partition, do you do the same?

I didn't load any data into tables when running the tests, because these
patches are meant for reducing planner latency. More specifically,
they're addressed to fix the current planner behavior that its latency
increases with increasing number of partitions, with focus on the common
case where only a single partition will need to be scanned by a given query.

I'd try to avoid using a benchmark whose results is affected by anything
other than the planning latency. It will be a good idea if you leave the
tables empty and just vary the number of partitions and nothing else.

Also, we're interested in planning latency, so using just SELECT and
UPDATE in your benchmark script will be enough, because their planning
time is affected by the number of partitions. No need to try to measure
the INSERT latency, because its planning latency is not affected by the
number of partitions. Moreover, I'd rather suggest you take out the
INSERT statement from the benchmark for now, because its execution time
does vary unfavorably with increasing number of partitions. Sure, there
are other patches to address that, but it's better not to mix the patches
to avoid confusion.

Also, in my case, performance was better when not prepare.

Patches in this thread do nothing for the execution, so, there is no need
to compare prepared vs non-prepared. In fact, just measure non-prepared
tps and latency, because we're only interested in planning time here.

I think these patches do not improve execute case, so we need faster runtime pruning patch[1], right?

We already have run-time pruning code (that is the code in the patch you
linked) committed into the tree in PG 11, so the master tree also has it.
But since we're not interested in execution time, no need to worry about
run-time pruning.

Details of measurement conditions and results are as follows.
- base source
master(@777e6ddf17) + Speeding up Insert v8 patch[1]

- table definition(e.g. 100 partition)
create table test.accounts(aid serial, abalance int)
partition by range(aid);
create table test.accounts_history(id serial, aid int, delta int,
mtime timestamp without time zone)
partition by range(aid);

- command option
pgbench -d testdb -f benchmark.pgbench -T 180 -r -n -M prepare
pgbench -d testdb -f benchmark.pgbench -T 180 -r -n

-results
base source no prepared
part_num | tps_ex | update_latency | select_latency | insert_latency
----------+------------+----------------+----------------+----------------
100 | 662.414805 | 0.357 | 0.265 | 0.421
200 | 494.478431 | 0.439 | 0.349 | 0.579
400 | 307.982089 | 0.651 | 0.558 | 0.927
800 | 191.360676 | 0.979 | 0.876 | 1.548
1600 | 75.344947 | 2.253 | 2.003 | 4.301
3200 | 30.643902 | 5.716 | 4.955 | 10.118
6400 | 16.726056 | 12.512 | 8.582 | 18.054

0001 no prepared
part_num | tps_ex | update_latency | select_latency | insert_latency
----------+------------+----------------+----------------+----------------
100 | 429.816329 | 0.811 | 0.75 | 0.365
200 | 275.211531 | 1.333 | 1.248 | 0.501
400 | 160.499833 | 2.384 | 2.252 | 0.754
800 | 79.387776 | 4.935 | 4.698 | 1.468
1600 | 24.787377 | 16.593 | 15.954 | 4.302
3200 | 9.846421 | 42.96 | 42.139 | 8.848
6400 | 4.919772 | 87.43 | 83.109 | 16.56

Hmm, since 0001 is meant to improve update planning time, it seems odd
that you'd get poorer results compared to base source. But, it seems base
source is actually master + the patch to improve the execution time of
update, so maybe that patch is playing a part here, although I'm not sure
why even that's making this much of a difference.

I suggest that you use un-patched master as base source, that is, leave
out any patches to improve execution time.

[ ... ]

0001 + 0002 no prepared
part_num | tps_ex | update_latency | select_latency | insert_latency
----------+------------+----------------+----------------+----------------
100 | 682.53091 | 0.388 | 0.35 | 0.35
200 | 469.906601 | 0.543 | 0.496 | 0.51
400 | 321.915349 | 0.78 | 0.721 | 0.752
800 | 201.620975 | 1.246 | 1.156 | 1.236
1600 | 94.438204 | 2.612 | 2.335 | 2.745
3200 | 38.292922 | 6.657 | 5.579 | 6.808
6400 | 21.48462 | 11.989 | 10.104 | 12.601

[ ... ]

0001 + 0002 + 0003 no prepared
part_num | tps_ex | update_latency | select_latency | insert_latency
----------+------------+----------------+----------------+----------------
100 | 798.962029 | 0.304 | 0.267 | 0.339
200 | 577.893396 | 0.384 | 0.346 | 0.487
400 | 426.542177 | 0.472 | 0.435 | 0.717
800 | 288.616213 | 0.63 | 0.591 | 1.162
1600 | 154.247034 | 1.056 | 0.987 | 2.384
3200 | 59.711446 | 2.416 | 2.233 | 6.514
6400 | 37.109761 | 3.387 | 3.099 | 11.762

[ ... ]

By the way, as you may have noticed, I posted a version 2 of the patches
on this thread. If you apply them, you will be be able to see almost same
TPS for any number of partitions with master + 0001 + 0002 + 0003.

Although it may not be related to this, when measured with pg11 beta2, somehow the performance was better.

11beta2 + v1-0001-Speed-up-INSERT-and-UPDATE-on-partitioned-tables.patch[3] prepared
part_num | tps_ex | update_latency | select_latency | insert_latency
----------+-------------+----------------+----------------+----------------
100 | 756.07228 | 0.942 | 0.091 | 0.123

I guess if you had applied the latest version of
"Speed-up-INSERT-and-UPDATE-on-partitioned-tables.patch" (which is v8
posted at [1]/messages/by-id/CAKJS1f9T_32Xpb-p8cWwo5ezSfVhXviUW8QTWncP8ksPHDRK8g@mail.gmail.com) on top of master + 0001 + 0002 + 0003, you'd get better
performance too. But, as mentioned above, we're interested in measuring
planning latency, not execution latency, so we should leave out any
patches that are meant toward improving execution latency to avoid confusion.

Thanks again.

Regards,
Amit

[1]: /messages/by-id/CAKJS1f9T_32Xpb-p8cWwo5ezSfVhXviUW8QTWncP8ksPHDRK8g@mail.gmail.com
/messages/by-id/CAKJS1f9T_32Xpb-p8cWwo5ezSfVhXviUW8QTWncP8ksPHDRK8g@mail.gmail.com

On Wed, Aug 29, 2018 at 5:36 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

It is more or less well known that the planner doesn't perform well with
more than a few hundred partitions even when only a handful of partitions
are ultimately included in the plan. Situation has improved a bit in PG
11 where we replaced the older method of pruning partitions one-by-one
using constraint exclusion with a much faster method that finds relevant
partitions by using partitioning metadata. However, we could only use it
for SELECT queries, because UPDATE/DELETE are handled by a completely
different code path, whose structure doesn't allow it to call the new
pruning module's functionality. Actually, not being able to use the new
pruning is not the only problem for UPDATE/DELETE, more on which further
below.

pgbench -n -T 60 -f update.sql

nparts master 0001 0002 0003
====== ====== ==== ==== ====
0 2856 2893 2862 2816
8 507 1115 1447 1872
16 260 765 1173 1892
32 119 483 922 1884
64 59 282 615 1881
128 29 153 378 1835
256 14 79 210 1803
512 5 40 113 1728
1024 2 17 57 1616
2048 0* 9 30 1471
4096 0+ 4 15 1236
8192 0= 2 7 975

* 0.46
+ 0.0064
= 0 (OOM on a virtual machine with 4GB RAM)

The idea looks interesting while going through the patch I observed
this comment.

/*
* inheritance_planner
* Generate Paths in the case where the result relation is an
* inheritance set.
*
* We have to handle this case differently from cases where a source relation
* is an inheritance set. Source inheritance is expanded at the bottom of the
* plan tree (see allpaths.c), but target inheritance has to be expanded at
* the top.

I think with your patch these comments needs to be change?

  if (parse->resultRelation &&
- rt_fetch(parse->resultRelation, parse->rtable)->inh)
+ rt_fetch(parse->resultRelation, parse->rtable)->inh &&
+ rt_fetch(parse->resultRelation, parse->rtable)->relkind !=
+ RELKIND_PARTITIONED_TABLE)
  inheritance_planner(root);
  else
  grouping_planner(root, false, tuple_fraction);

I think we can add some comments to explain if the target rel itself
is partitioned rel then why
we can directly go to the grouping planner.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Hi, Amit. Thank you for your reply.

I didn't load any data into tables when running the tests, because these patches are meant for reducing planner latency. More specifically, they're addressed to fix the current planner behavior that its latency increases with increasing number of partitions, with focus on the common case where only a single partition will need to be scanned by a given query.

Thank you for telling me details. It is very helpful.

No need to try to measure the INSERT latency, because its planning latency is not affected by the number of partitions. Moreover, I'd rather suggest you take out the INSERT statement from the benchmark for now, because its execution time does vary unfavorably with increasing number of partitions.

Thank you for your advice.

In fact, just measure non-prepared tps and latency, because we're only interested in planning time here.

I got it.

Hmm, since 0001 is meant to improve update planning time, it seems odd that you'd get poorer results compared to base source. But, it seems base source is actually master + the patch to improve the execution time of update, so maybe that patch is playing a part here, although I'm not sure why even that's making this much of a difference.
I suggest that you use un-patched master as base source, that is, leave out any patches to improve execution time.

By the way, as you may have noticed, I posted a version 2 of the patches on this thread. If you apply them, you will be be able to see almost same TPS for any number of partitions with master + 0001 + 0002 + 0003.

I guess if you had applied the latest version of "Speed-up-INSERT-and-UPDATE-on-partitioned-tables.patch" (which is v8 posted at [1]) on top of master + 0001 + 0002 + 0003, you'd get better performance too.

Thank you. I will try out these cases.

Thanks,

--
Sho Kato
-----Original Message-----
From: Amit Langote [mailto:Langote_Amit_f8@lab.ntt.co.jp]
Sent: Monday, September 3, 2018 2:12 PM
To: Kato, Sho/加藤 翔 <kato-sho@jp.fujitsu.com>; Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: speeding up planning with partitions

Thank you Kato-san for testing.

On 2018/08/31 19:48, Kato, Sho wrote:

Hi, Amit

Great!
With the total number of records being 6400, I benchmarked while increasing the number of partitions from 100 to 6400.
Applying three all patches, 20% performance improved for 100 partitions.

I have the same number of records for each partition, do you do the same?

I didn't load any data into tables when running the tests, because these patches are meant for reducing planner latency. More specifically, they're addressed to fix the current planner behavior that its latency increases with increasing number of partitions, with focus on the common case where only a single partition will need to be scanned by a given query.

I'd try to avoid using a benchmark whose results is affected by anything other than the planning latency. It will be a good idea if you leave the tables empty and just vary the number of partitions and nothing else.

Also, we're interested in planning latency, so using just SELECT and UPDATE in your benchmark script will be enough, because their planning time is affected by the number of partitions. No need to try to measure the INSERT latency, because its planning latency is not affected by the number of partitions. Moreover, I'd rather suggest you take out the INSERT statement from the benchmark for now, because its execution time does vary unfavorably with increasing number of partitions. Sure, there are other patches to address that, but it's better not to mix the patches to avoid confusion.

Also, in my case, performance was better when not prepare.

Patches in this thread do nothing for the execution, so, there is no need to compare prepared vs non-prepared. In fact, just measure non-prepared tps and latency, because we're only interested in planning time here.

I think these patches do not improve execute case, so we need faster runtime pruning patch[1], right?

We already have run-time pruning code (that is the code in the patch you
linked) committed into the tree in PG 11, so the master tree also has it.
But since we're not interested in execution time, no need to worry about run-time pruning.

Details of measurement conditions and results are as follows.
- base source
master(@777e6ddf17) + Speeding up Insert v8 patch[1]

- table definition(e.g. 100 partition)
create table test.accounts(aid serial, abalance int)
partition by range(aid);
create table test.accounts_history(id serial, aid int, delta int,
mtime timestamp without time zone)
partition by range(aid);

- command option
pgbench -d testdb -f benchmark.pgbench -T 180 -r -n -M prepare
pgbench -d testdb -f benchmark.pgbench -T 180 -r -n

-results
base source no prepared
part_num | tps_ex | update_latency | select_latency | insert_latency
----------+------------+----------------+----------------+----------------
100 | 662.414805 | 0.357 | 0.265 | 0.421
200 | 494.478431 | 0.439 | 0.349 | 0.579
400 | 307.982089 | 0.651 | 0.558 | 0.927
800 | 191.360676 | 0.979 | 0.876 | 1.548
1600 | 75.344947 | 2.253 | 2.003 | 4.301
3200 | 30.643902 | 5.716 | 4.955 | 10.118
6400 | 16.726056 | 12.512 | 8.582 | 18.054

0001 no prepared
part_num | tps_ex | update_latency | select_latency | insert_latency
----------+------------+----------------+----------------+----------------
100 | 429.816329 | 0.811 | 0.75 | 0.365
200 | 275.211531 | 1.333 | 1.248 | 0.501
400 | 160.499833 | 2.384 | 2.252 | 0.754
800 | 79.387776 | 4.935 | 4.698 | 1.468
1600 | 24.787377 | 16.593 | 15.954 | 4.302
3200 | 9.846421 | 42.96 | 42.139 | 8.848
6400 | 4.919772 | 87.43 | 83.109 | 16.56

Hmm, since 0001 is meant to improve update planning time, it seems odd that you'd get poorer results compared to base source. But, it seems base source is actually master + the patch to improve the execution time of update, so maybe that patch is playing a part here, although I'm not sure why even that's making this much of a difference.

I suggest that you use un-patched master as base source, that is, leave out any patches to improve execution time.

[ ... ]

0001 + 0002 no prepared
part_num | tps_ex | update_latency | select_latency | insert_latency
----------+------------+----------------+----------------+----------------
100 | 682.53091 | 0.388 | 0.35 | 0.35
200 | 469.906601 | 0.543 | 0.496 | 0.51
400 | 321.915349 | 0.78 | 0.721 | 0.752
800 | 201.620975 | 1.246 | 1.156 | 1.236
1600 | 94.438204 | 2.612 | 2.335 | 2.745
3200 | 38.292922 | 6.657 | 5.579 | 6.808
6400 | 21.48462 | 11.989 | 10.104 | 12.601

[ ... ]

0001 + 0002 + 0003 no prepared
part_num | tps_ex | update_latency | select_latency | insert_latency
----------+------------+----------------+----------------+----------------
100 | 798.962029 | 0.304 | 0.267 | 0.339
200 | 577.893396 | 0.384 | 0.346 | 0.487
400 | 426.542177 | 0.472 | 0.435 | 0.717
800 | 288.616213 | 0.63 | 0.591 | 1.162
1600 | 154.247034 | 1.056 | 0.987 | 2.384
3200 | 59.711446 | 2.416 | 2.233 | 6.514
6400 | 37.109761 | 3.387 | 3.099 | 11.762

[ ... ]

By the way, as you may have noticed, I posted a version 2 of the patches on this thread. If you apply them, you will be be able to see almost same TPS for any number of partitions with master + 0001 + 0002 + 0003.

Although it may not be related to this, when measured with pg11 beta2, somehow the performance was better.

11beta2 + v1-0001-Speed-up-INSERT-and-UPDATE-on-partitioned-tables.patch[3] prepared
part_num | tps_ex | update_latency | select_latency | insert_latency
----------+-------------+----------------+----------------+----------------
100 | 756.07228 | 0.942 | 0.091 | 0.123

I guess if you had applied the latest version of "Speed-up-INSERT-and-UPDATE-on-partitioned-tables.patch" (which is v8 posted at [1]/messages/by-id/CAKJS1f9T_32Xpb-p8cWwo5ezSfVhXviUW8QTWncP8ksPHDRK8g@mail.gmail.com) on top of master + 0001 + 0002 + 0003, you'd get better performance too. But, as mentioned above, we're interested in measuring planning latency, not execution latency, so we should leave out any patches that are meant toward improving execution latency to avoid confusion.

Thanks again.

Regards,
Amit

[1]: /messages/by-id/CAKJS1f9T_32Xpb-p8cWwo5ezSfVhXviUW8QTWncP8ksPHDRK8g@mail.gmail.com
/messages/by-id/CAKJS1f9T_32Xpb-p8cWwo5ezSfVhXviUW8QTWncP8ksPHDRK8g@mail.gmail.com

Hi Dilip,

Thanks for taking a look.

On 2018/09/03 20:57, Dilip Kumar wrote:

The idea looks interesting while going through the patch I observed
this comment.

/*
* inheritance_planner
* Generate Paths in the case where the result relation is an
* inheritance set.
*
* We have to handle this case differently from cases where a source relation
* is an inheritance set. Source inheritance is expanded at the bottom of the
* plan tree (see allpaths.c), but target inheritance has to be expanded at
* the top.

I think with your patch these comments needs to be change?

Yes, maybe a good idea to mention that partitioned table result relations
are not handled here.

Actually, I've been wondering if this patch (0001) shouldn't get rid of
inheritance_planner altogether and implement the new approach for *all*
inheritance sets, not just partitioned tables, but haven't spent much time
on that idea yet.

if (parse->resultRelation &&
- rt_fetch(parse->resultRelation, parse->rtable)->inh)
+ rt_fetch(parse->resultRelation, parse->rtable)->inh &&
+ rt_fetch(parse->resultRelation, parse->rtable)->relkind !=
+ RELKIND_PARTITIONED_TABLE)
inheritance_planner(root);
else
grouping_planner(root, false, tuple_fraction);

I think we can add some comments to explain if the target rel itself
is partitioned rel then why
we can directly go to the grouping planner.

Okay, I will try to add more explanatory comments here and there in the
next version I will post later this week.

Thanks,
Amit

On 30 August 2018 at 00:06, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:

With various overheads gone thanks to 0001 and 0002, locking of all
partitions via find_all_inheritos can be seen as the single largest
bottleneck, which 0003 tries to address. I've kept it a separate patch,
because I'll need to think a bit more to say that it's actually to safe to
defer locking to late planning, due mainly to the concern about the change
in the order of locking from the current method. I'm attaching it here,
because I also want to show the performance improvement we can expect with it.

For now, find_all_inheritors() locks the tables in ascending Oid
order. This makes sense with inheritance parent tables as it's much
cheaper to sort on this rather than on something like the table's
namespace and name. I see no reason why what we sort on really
matters, as long as we always sort on the same thing and the key we
sort on is always unique so that the locking order is well defined.

For partitioned tables, there's really not much sense in sticking to
the same lock by Oid order. The order of the PartitionDesc is already
well defined so I don't see any reason why we can't just perform the
locking in PartitionDesc order. This would mean you could perform the
locking of the partitions once pruning is complete somewhere around
add_rel_partitions_to_query(). Also, doing this would remove the need
for scanning pg_inherits during find_all_inheritors() and would likely
further speed up the planning of queries to partitioned tables with
many partitions. I wrote a function named
get_partition_descendants_worker() to do this in patch 0002 in [1]/messages/by-id/CAKJS1f9QjUwQrio20Pi=yCHmnouf4z3SfN8sqXaAcwREG6k0zQ@mail.gmail.com (it
may have been a bad choice to have made this a separate function
rather than just part of find_all_inheritors() as it meant I had to
change a bit too much code in tablecmds.c). There might be something
you can salvage from that patch to help here.

[1]: /messages/by-id/CAKJS1f9QjUwQrio20Pi=yCHmnouf4z3SfN8sqXaAcwREG6k0zQ@mail.gmail.com

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2018/09/04 22:24, David Rowley wrote:

On 30 August 2018 at 00:06, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:

With various overheads gone thanks to 0001 and 0002, locking of all
partitions via find_all_inheritos can be seen as the single largest
bottleneck, which 0003 tries to address. I've kept it a separate patch,
because I'll need to think a bit more to say that it's actually to safe to
defer locking to late planning, due mainly to the concern about the change
in the order of locking from the current method. I'm attaching it here,
because I also want to show the performance improvement we can expect with it.

For now, find_all_inheritors() locks the tables in ascending Oid
order. This makes sense with inheritance parent tables as it's much
cheaper to sort on this rather than on something like the table's
namespace and name. I see no reason why what we sort on really
matters, as long as we always sort on the same thing and the key we
sort on is always unique so that the locking order is well defined.

For partitioned tables, there's really not much sense in sticking to
the same lock by Oid order. The order of the PartitionDesc is already
well defined so I don't see any reason why we can't just perform the
locking in PartitionDesc order.

The reason we do the locking with find_all_inheritors for regular queries
(planner (expand_inherited_rtentry) does it for select/update/delete and
executor (ExecSetupPartitionTupleRouting) for insert) is that that's the
order used by various DDL commands when locking partitions, such as when
adding a column. So, we'd have one piece of code locking partitions by
Oid order and others by canonical partition bound order or PartitionDesc
order. I'm no longer sure if that's problematic though, about which more
below.

This would mean you could perform the
locking of the partitions once pruning is complete somewhere around
add_rel_partitions_to_query(). Also, doing this would remove the need
for scanning pg_inherits during find_all_inheritors() and would likely
further speed up the planning of queries to partitioned tables with
many partitions.

That's what happens with 0003; note the following hunk in it:

@@ -1555,14 +1555,15 @@ expand_inherited_rtentry(PlannerInfo *root,
RangeTblEntry *rte, Index rti)
lockmode = AccessShareLock;

     /* Scan for all members of inheritance set, acquire needed locks */
-    inhOIDs = find_all_inheritors(parentOID, lockmode, NULL);
+    if (rte->relkind != RELKIND_PARTITIONED_TABLE)
+        inhOIDs = find_all_inheritors(parentOID, lockmode, NULL);

I wrote a function named
get_partition_descendants_worker() to do this in patch 0002 in [1] (it
may have been a bad choice to have made this a separate function
rather than just part of find_all_inheritors() as it meant I had to
change a bit too much code in tablecmds.c). There might be something
you can salvage from that patch to help here.

[1] /messages/by-id/CAKJS1f9QjUwQrio20Pi=yCHmnouf4z3SfN8sqXaAcwREG6k0zQ@mail.gmail.com

Actually, I had written a similar patch to replace the usages of
find_all_inheritors and find_inheritance_children by different
partitioning-specific functions which would collect the the partition OIDs
from the already open parent table's PartitionDesc, more or less like the
patch you mention does. But I think we don't need any new function(s) to
do that, that is, we don't need to change all the sites that call
find_all_inheritors or find_inheritance_children in favor of new functions
that return partition OIDs in PartitionDesc order, if only *because* we
want to change planner to lock partitions in the PartitionDesc order. I'm
failing to see why the difference in locking order matters. I understood
the concern as that locking partitions in different order could lead to a
deadlock if concurrent backends request mutually conflicting locks, but
only one of the conflicting backends, the one that got lock on the parent,
would be allowed to lock children. Thoughts on that?

Thanks,
Amit

On 30 August 2018 at 21:29, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:

Attached updated patches, with 0002 containing the changes mentioned above.

Here's my first pass review on this:

0001:

1. I think the following paragraph should probably mention something
about some performance difference between the two methods:

<para>
Constraint exclusion works in a very similar way to partition
pruning, except that it uses each table's <literal>CHECK</literal>
constraints &mdash; which gives it its name &mdash; whereas partition
pruning uses the table's partition bounds, which exist only in the
case of declarative partitioning. Another difference is that
constraint exclusion is only applied at plan time; there is no attempt
to remove partitions at execution time.
</para>

Perhaps tagging on. "Constraint exclusion is also a much less
efficient way of eliminating unneeded partitions as metadata for each
partition must be loaded in the planner before constraint exclusion
can be applied. This is not a requirement for partition pruning."

2. I think "rootrel" should be named "targetpart" in:

+ RelOptInfo *rootrel = root->simple_rel_array[root->parse->resultRelation];

3. Why does the following need to list_copy()?

+ List *saved_join_info_list = list_copy(root->join_info_list);

4. Is the "root->parse->commandType != CMD_INSERT" required in:

@@ -181,13 +185,30 @@ make_one_rel(PlannerInfo *root, List *joinlist)

  /*
  * Generate access paths for the entire join tree.
+ *
+ * If we're doing this for an UPDATE or DELETE query whose target is a
+ * partitioned table, we must do the join planning against each of its
+ * leaf partitions instead.
  */
- rel = make_rel_from_joinlist(root, joinlist);
+ if (root->parse->resultRelation &&
+ root->parse->commandType != CMD_INSERT &&
+ root->simple_rel_array[root->parse->resultRelation] &&
+ root->simple_rel_array[root->parse->resultRelation]->part_scheme)
+ {

Won't the simple_rel_array entry for the resultRelation always be NULL
for an INSERT?

5. In regards to:

+ /*
+ * Hack to make the join planning code believe that 'partrel' can
+ * be joined against.
+ */
+ partrel->reloptkind = RELOPT_BASEREL;

Have you thought about other implications of join planning for other
member rels, for example, equivalence classes and em_is_child?

6. It would be good to not have to rt_fetch the same RangeTblEntry twice here:

@@ -959,7 +969,9 @@ subquery_planner(PlannerGlobal *glob, Query *parse,
  * needs special processing, else go straight to grouping_planner.
  */
  if (parse->resultRelation &&
- rt_fetch(parse->resultRelation, parse->rtable)->inh)
+ rt_fetch(parse->resultRelation, parse->rtable)->inh &&
+ rt_fetch(parse->resultRelation, parse->rtable)->relkind !=
+ RELKIND_PARTITIONED_TABLE)
  inheritance_planner(root);

7. Why don't you just pass the Parse into the function as a parameter
instead of overwriting PlannerInfo's copy in:

+ root->parse = partition_parse;
+ partitionwise_adjust_scanjoin_target(root, child_rel,
+ subroots,
+ partitioned_rels,
+ resultRelations,
+ subpaths,
+ WCOLists,
+ returningLists,
+ rowMarks);
+ /* Restore the Query for processing the next partition. */
+ root->parse = parse;

8. Can you update the following comment to mention why you're not
calling add_paths_to_append_rel for this case?

@@ -6964,7 +7164,9 @@ apply_scanjoin_target_to_paths(PlannerInfo *root,
}

  /* Build new paths for this relation by appending child paths. */
- if (live_children != NIL)
+ if (live_children != NIL &&
+ !(rel->reloptkind == RELOPT_BASEREL &&
+   rel->relid == root->parse->resultRelation))
  add_paths_to_append_rel(root, rel, live_children);

9. The following should use >= 0, not > 0

+ while ((relid = bms_next_member(child_rel->relids, relid)) > 0)

0002:

10. I think moving the PlannerInfo->total_table_pages calculation
needs to be done in its own patch. This is a behavioural change where
we no longer count pruned relations in the calculation which can
result in plan changes. I posted a patch in [1]https://commitfest.postgresql.org/19/1768/ to fix this as a bug
fix as I think the current code is incorrect. My patch also updates
the first paragraph of the comment. You've not done that.

11. "pruning"

+ * And do prunin. Note that this adds AppendRelInfo's of only the

12. It's much more efficient just to do bms_add_range() outside the loop in:

+ for (i = 0; i < rel->nparts; i++)
+ {
+ rel->part_rels[i] = build_partition_rel(root, rel,
+ rel->part_oids[i]);
+ rel->live_parts = bms_add_member(rel->live_parts, i);
+ }

13. In set_append_rel_size() the foreach(l, root->append_rel_list)
loop could be made to loop over RelOptInfo->live_parts instead which
would save having to skip over AppendRelInfos that don't belong to
this parent. You'd need to make live_parts more general purpose and
also use it to mark children of inheritance parents.

14. I think you can skip the following if both are NULL. You could
likely add more smarts for different join types, but I think you
should at least skip when both are NULL. Perhaps the loop could be
driven off of bms_intersec of the two Rel's live_parts?

+ if (child_rel1 == NULL)
+ child_rel1 = build_dummy_partition_rel(root, rel1, cnt_parts);
+ if (child_rel2 == NULL)
+ child_rel2 = build_dummy_partition_rel(root, rel2, cnt_parts);

15. The following is not required when append_rel_array was previously NULL.

+ MemSet(root->append_rel_array + root->simple_rel_array_size,
+    0, sizeof(AppendRelInfo *) * num_added_parts);

16. I don't think scan_all_parts is worth the extra code. The cost of
doing bms_num_members is going to be pretty small in comparison to
building paths for and maybe doing a join search for all partitions.

+ num_added_parts = scan_all_parts ? rel->nparts :
+ bms_num_members(partindexes);

In any case, you've got a bug in prune_append_rel_partitions() where
you're setting scan_all_parts to true instead of returning when
contradictory == true.

17. lockmode be of type LOCKMODE, not int.

+ Oid childOID, int lockmode,

18. Comment contradicts the code.

+ /* Open rel if needed; we already have required locks */
+ if (childOID != parentOID)
+ {
+ childrel = heap_open(childOID, lockmode);

I think you should be using NoLock here.

19. Comment contradicts the code.

+ /* Close child relations, but keep locks */
+ if (childOID != parentOID)
+ {
+ Assert(childrel != NULL);
+ heap_close(childrel, lockmode);
+ }

20. I assume translated_vars can now be NIL due to
build_dummy_partition_rel() not setting it.

- if (var->varlevelsup == 0 && appinfo)
+ if (var->varlevelsup == 0 && appinfo && appinfo->translated_vars)

It might be worth writing a comment to explain that, otherwise it's
not quite clear why you're doing this.

21. Unrelated change;

Assert(relid > 0 && relid < root->simple_rel_array_size);
+

22. The following comment mentions that Oids are copied, but that does
not happen in the code.

+ /*
+ * For partitioned tables, we just allocate space for RelOptInfo's.
+ * pointers for all partitions and copy the partition OIDs from the
+ * relcache.  Actual RelOptInfo is built for a partition only if it is
+ * not pruned.
+ */

The Oid copying already happened during get_relation_info().

23. Traditionally translated_vars populated with a sequence of Vars in
the same order to mean no translation. Here you're changing how that
works:

+ /* leaving translated_vars to NIL to mean no translation needed */

This seems to be about the extent of your documentation on this, which
is not good enough.

24. "each" -> "reach"? ... Actually, I don't understand the comment.
In a partitioned hierarchy, how can the one before the top-level
partitioned table not be a partitioned table?

/*
* Keep moving up until we each the parent rel that's not a
* partitioned table. The one before that one would be the root
* parent.
*/

25. "already"

+ * expand_inherited_rtentry alreay locked all partitions, so pass

26. Your changes to make_partitionedrel_pruneinfo() look a bit broken.
You're wrongly assuming live_parts is the same as present_parts. If a
CHECK constraint eliminated the partition then those will be present
in live_parts but won't be part of the Append/MergeAppend subplans.
You might be able to maintain some of this optimisation by checking
for dummy rels inside the loop, but you're going to need to put back
the code that sets present_parts.

+ present_parts = bms_copy(subpart->live_parts);

27. Comment contradicts the code:

+ Bitmapset *live_parts; /* unpruned parts; NULL if all are live */

in add_rel_partitions_to_query() you're doing:

+ if (scan_all_parts)
+ {
+ for (i = 0; i < rel->nparts; i++)
+ {
+ rel->part_rels[i] = build_partition_rel(root, rel,
+ rel->part_oids[i]);
+ rel->live_parts = bms_add_member(rel->live_parts, i);
+ }
+ }

so the NULL part seems untrue.

28. Missing comments:

+ TupleDesc tupdesc;
+ Oid reltype;

29. The comment for prune_append_rel_partitions claims it "Returns RT
indexes", but that's not the case, per:

-Relids
-prune_append_rel_partitions(RelOptInfo *rel)
+void
+prune_append_rel_partitions(PlannerInfo *root, RelOptInfo *rel)

0003:

30. 2nd test can be tested inside the first test to remove redundant
partition check.

- inhOIDs = find_all_inheritors(parentOID, lockmode, NULL);
+ if (rte->relkind != RELKIND_PARTITIONED_TABLE)
+ inhOIDs = find_all_inheritors(parentOID, lockmode, NULL);

  /*
  * Check that there's at least one descendant, else treat as no-child
  * case.  This could happen despite above has_subclass() check, if table
  * once had a child but no longer does.
  */
- if (list_length(inhOIDs) < 2)
+ if (rte->relkind != RELKIND_PARTITIONED_TABLE && list_length(inhOIDs) < 2)
  {

31. The following code is wrong:

+ /* Determine the correct lockmode to use. */
+ if (rootRTindex == root->parse->resultRelation)
+ lockmode = RowExclusiveLock;
+ else if (rootrc && RowMarkRequiresRowShareLock(rootrc->markType))
+ lockmode = RowShareLock;
+ else
+ lockmode = AccessShareLock;

rootRTIndex remains at 0 if there are no row marks and resultRelation
will be 0 for SELECT queries, this means you'll use a RowExclusiveLock
for a SELECT instead of an AccessShareLock.

Why not just check the lockmode of the parent and use that?

[1]: https://commitfest.postgresql.org/19/1768/

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Aug 29, 2018 at 5:06 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

It is more or less well known that the planner doesn't perform well with
more than a few hundred partitions even when only a handful of partitions
are ultimately included in the plan. Situation has improved a bit in PG
11 where we replaced the older method of pruning partitions one-by-one
using constraint exclusion with a much faster method that finds relevant
partitions by using partitioning metadata. However, we could only use it
for SELECT queries, because UPDATE/DELETE are handled by a completely
different code path, whose structure doesn't allow it to call the new
pruning module's functionality. Actually, not being able to use the new
pruning is not the only problem for UPDATE/DELETE, more on which further
below.

This was a big problem for the SQL MERGE patch. I hope that this
problem can be fixed.

--
Peter Geoghegan

On 2018/09/11 10:11, Peter Geoghegan wrote:

On Wed, Aug 29, 2018 at 5:06 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

It is more or less well known that the planner doesn't perform well with
more than a few hundred partitions even when only a handful of partitions
are ultimately included in the plan. Situation has improved a bit in PG
11 where we replaced the older method of pruning partitions one-by-one
using constraint exclusion with a much faster method that finds relevant
partitions by using partitioning metadata. However, we could only use it
for SELECT queries, because UPDATE/DELETE are handled by a completely
different code path, whose structure doesn't allow it to call the new
pruning module's functionality. Actually, not being able to use the new
pruning is not the only problem for UPDATE/DELETE, more on which further
below.

This was a big problem for the SQL MERGE patch. I hope that this
problem can be fixed.

Sorry if I've missed some prior discussion about this, but could you
clarify which aspect of UPDATE/DELETE planning got in the way of SQL MERGE
patch? It'd be great if you can point to an email or portion of the SQL
MERGE discussion thread where this aspect was discussed.

In the updated patch that I'm still hacking on (also need to look at
David's comments earlier today before posting it), I have managed to
eliminate the separation of code paths handling SELECT vs UPDATE/DELETE on
inheritance tables, but I can't be sure if the new approach (also) solves
any problems that were faced during SQL MERGE development.

Thanks,
Amit

On Tue, Sep 4, 2018 at 12:44 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

Hi Dilip,

Thanks for taking a look.

On 2018/09/03 20:57, Dilip Kumar wrote:

The idea looks interesting while going through the patch I observed
this comment.

/*
* inheritance_planner
* Generate Paths in the case where the result relation is an
* inheritance set.
*
* We have to handle this case differently from cases where a source relation
* is an inheritance set. Source inheritance is expanded at the bottom of the
* plan tree (see allpaths.c), but target inheritance has to be expanded at
* the top.

I think with your patch these comments needs to be change?

Yes, maybe a good idea to mention that partitioned table result relations
are not handled here.

Actually, I've been wondering if this patch (0001) shouldn't get rid of
inheritance_planner altogether and implement the new approach for *all*
inheritance sets, not just partitioned tables, but haven't spent much time
on that idea yet.

That will be interesting.

if (parse->resultRelation &&
- rt_fetch(parse->resultRelation, parse->rtable)->inh)
+ rt_fetch(parse->resultRelation, parse->rtable)->inh &&
+ rt_fetch(parse->resultRelation, parse->rtable)->relkind !=
+ RELKIND_PARTITIONED_TABLE)
inheritance_planner(root);
else
grouping_planner(root, false, tuple_fraction);

I think we can add some comments to explain if the target rel itself
is partitioned rel then why
we can directly go to the grouping planner.

Okay, I will try to add more explanatory comments here and there in the
next version I will post later this week.

Okay.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Thanks a lot for your detailed review.

On 2018/09/11 8:23, David Rowley wrote:

On 30 August 2018 at 21:29, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:

Attached updated patches, with 0002 containing the changes mentioned above.

Here's my first pass review on this:

I've gone through your comments on 0001 so far, but didn't go through
others yet, to which I'll reply separately.

0001:

1. I think the following paragraph should probably mention something
about some performance difference between the two methods:

<para>
Constraint exclusion works in a very similar way to partition
pruning, except that it uses each table's <literal>CHECK</literal>
constraints &mdash; which gives it its name &mdash; whereas partition
pruning uses the table's partition bounds, which exist only in the
case of declarative partitioning. Another difference is that
constraint exclusion is only applied at plan time; there is no attempt
to remove partitions at execution time.
</para>

Perhaps tagging on. "Constraint exclusion is also a much less
efficient way of eliminating unneeded partitions as metadata for each
partition must be loaded in the planner before constraint exclusion
can be applied. This is not a requirement for partition pruning."

Hmm, isn't that implied by the existing text? It already says that
constraint exclusion uses *each* table's/partition's CHECK constraints,
which should make it clear that for a whole lot of partitions, that will
be a slower than partition pruning which requires accessing only one
table's metadata.

If we will have dissociated constraint exclusion completely from
partitioned tables with these patches, I'm not sure if we have to stress
that it is inefficient for large number of tables.

2. I think "rootrel" should be named "targetpart" in:

+ RelOptInfo *rootrel = root->simple_rel_array[root->parse->resultRelation];

To me, "targetpart" makes a partitioned table sound like a partition,
which it is not. I get that using "root" can be ambiguous, because a
query can specify a non-root partitioned table.

How about "targetrel"?

3. Why does the following need to list_copy()?

+ List *saved_join_info_list = list_copy(root->join_info_list);

In earlier versions of this code, root->join_info_list would be modified
during make_one_rel_from_joinlist, but that no longer seems true.

Removed the list_copy.

4. Is the "root->parse->commandType != CMD_INSERT" required in:

@@ -181,13 +185,30 @@ make_one_rel(PlannerInfo *root, List *joinlist)

/*
* Generate access paths for the entire join tree.
+ *
+ * If we're doing this for an UPDATE or DELETE query whose target is a
+ * partitioned table, we must do the join planning against each of its
+ * leaf partitions instead.
*/
- rel = make_rel_from_joinlist(root, joinlist);
+ if (root->parse->resultRelation &&
+ root->parse->commandType != CMD_INSERT &&
+ root->simple_rel_array[root->parse->resultRelation] &&
+ root->simple_rel_array[root->parse->resultRelation]->part_scheme)
+ {

Won't the simple_rel_array entry for the resultRelation always be NULL
for an INSERT?

Yep, you're right. Removed.

5. In regards to:

+ /*
+ * Hack to make the join planning code believe that 'partrel' can
+ * be joined against.
+ */
+ partrel->reloptkind = RELOPT_BASEREL;

Have you thought about other implications of join planning for other
member rels, for example, equivalence classes and em_is_child?

Haven't really, but that seems like an important point. I will study it
more closely.

6. It would be good to not have to rt_fetch the same RangeTblEntry twice

here:

@@ -959,7 +969,9 @@ subquery_planner(PlannerGlobal *glob, Query *parse,
* needs special processing, else go straight to grouping_planner.
*/
if (parse->resultRelation &&
- rt_fetch(parse->resultRelation, parse->rtable)->inh)
+ rt_fetch(parse->resultRelation, parse->rtable)->inh &&
+ rt_fetch(parse->resultRelation, parse->rtable)->relkind !=
+ RELKIND_PARTITIONED_TABLE)
inheritance_planner(root);

The new version doesn't call inheritance_planner at all; there is no
inheritance_planner in the new version.

7. Why don't you just pass the Parse into the function as a parameter
instead of overwriting PlannerInfo's copy in:

+ root->parse = partition_parse;
+ partitionwise_adjust_scanjoin_target(root, child_rel,
+ subroots,
+ partitioned_rels,
+ resultRelations,
+ subpaths,
+ WCOLists,
+ returningLists,
+ rowMarks);
+ /* Restore the Query for processing the next partition. */
+ root->parse = parse;

Okay, done.

8. Can you update the following comment to mention why you're not
calling add_paths_to_append_rel for this case?

@@ -6964,7 +7164,9 @@ apply_scanjoin_target_to_paths(PlannerInfo *root,
}

/* Build new paths for this relation by appending child paths. */
- if (live_children != NIL)
+ if (live_children != NIL &&
+ !(rel->reloptkind == RELOPT_BASEREL &&
+   rel->relid == root->parse->resultRelation))
add_paths_to_append_rel(root, rel, live_children);

Oops, stray code from a previous revision. Removed this hunk.

9. The following should use >= 0, not > 0

+ while ((relid = bms_next_member(child_rel->relids, relid)) > 0)

Yeah, fixed.

Sorry, I haven't yet worked on your comments on 0002 and 0003. For time
being, I'd like to report what I've been up to these past couple of days,
because starting tomorrow until the end of this month, I won't be able to
reply to emails on -hackers due to personal vacation.

So, I spent a better part of last few days on trying to revise the patches
so that it changes the planning code for *all* inheritance cases instead
of just focusing on partitioning. Because, really, the only difference
between the partitioning code and regular inheritance code should be that
partitioning is able to use faster pruning, and all the other parts should
look and work more or less the same. There shouldn't be code here that
deals with partitioning and code there to deal with inheritance.
Minimizing code this way should be a good end to aim for, imho.

Attached is what I have at the moment. As part of the effort mentioned
above, I made 0001 to remove inheritance_planner altogether, instead of
just taking out the partitioning case out of it. Then there is the WIP
patch 0004 which tries to move even regular inheritance expansion to late
planning stage, just like the earlier versions did for partitioning. It
will need quite a bit of polishing before we could consider it for merging
with 0002. Of course, I'll need to address your comments before
considering doing that any seriously.

Thanks,
Amit

On Fri, Sep 14, 2018 at 3:58 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

Thanks a lot for your detailed review.

I was going through your patch (v3-0002) and I have some suggestions

1.
- if (nparts > 0)
+ /*
+ * For partitioned tables, we just allocate space for RelOptInfo's.
+ * pointers for all partitions and copy the partition OIDs from the
+ * relcache.  Actual RelOptInfo is built for a partition only if it is
+ * not pruned.
+ */
+ if (rte->relkind == RELKIND_PARTITIONED_TABLE)
+ {
  rel->part_rels = (RelOptInfo **)
- palloc(sizeof(RelOptInfo *) * nparts);
+ palloc0(sizeof(RelOptInfo *) * rel->nparts);
+ return rel;
+ }

I think we can delay allocating memory for rel->part_rels? And we can
allocate in add_rel_partitions_to_query only
for those partitions which survive pruning.

2.
add_rel_partitions_to_query
...
+ /* Expand the PlannerInfo arrays to hold new partition objects. */
+ num_added_parts = scan_all_parts ? rel->nparts :
+ bms_num_members(partindexes);
+ new_size = root->simple_rel_array_size + num_added_parts;
+ root->simple_rte_array = (RangeTblEntry **)
+ repalloc(root->simple_rte_array,
+ sizeof(RangeTblEntry *) * new_size);
+ root->simple_rel_array = (RelOptInfo **)
+ repalloc(root->simple_rel_array,
+ sizeof(RelOptInfo *) * new_size);
+ if (root->append_rel_array)
+ root->append_rel_array = (AppendRelInfo **)
+ repalloc(root->append_rel_array,
+ sizeof(AppendRelInfo *) * new_size);
+ else
+ root->append_rel_array = (AppendRelInfo **)
+ palloc0(sizeof(AppendRelInfo *) *
+ new_size);

Instead of re-pallocing for every partitioned relation can't we first
count the total number of surviving partitions and
repalloc at once.

3.
+ /*
+ * And do prunin.  Note that this adds AppendRelInfo's of only the
+ * partitions that are not pruned.
+ */

prunin/pruning

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Sat, Sep 29, 2018 at 07:00:02PM +0530, Dilip Kumar wrote:

I think we can delay allocating memory for rel->part_rels? And we can
allocate in add_rel_partitions_to_query only
for those partitions which survive pruning.

This last review set has not been answered, and as it is recent I am
moving the patch to next CF, waiting on author.
--
Michael

On 2018/10/02 10:20, Michael Paquier wrote:

On Sat, Sep 29, 2018 at 07:00:02PM +0530, Dilip Kumar wrote:

I think we can delay allocating memory for rel->part_rels? And we can
allocate in add_rel_partitions_to_query only
for those partitions which survive pruning.

This last review set has not been answered, and as it is recent I am
moving the patch to next CF, waiting on author.

I was thinking of doing that myself today, thanks for taking care of that.

Will get back to this by the end of this week.

Thanks,
Amit

On Fri, Sep 14, 2018 at 10:29 PM Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

Attached is what I have at the moment.

I realise this is a WIP but FYI the docs don't build (you removed a
<note> element that is still needed, when removing a paragraph).

--
Thomas Munro
http://www.enterprisedb.com

On 2018/10/03 8:31, Thomas Munro wrote:

On Fri, Sep 14, 2018 at 10:29 PM Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

Attached is what I have at the moment.

I realise this is a WIP but FYI the docs don't build (you removed a
<note> element that is still needed, when removing a paragraph).

Thanks Thomas for the heads up, will fix.

Regards,
Amit

Hi, Amit!

On Thu, Sept 13, 2018 at 10:29 PM, Amit Langote wrote:

Attached is what I have at the moment.

I also do the code review of the patch.
I could only see a v3-0001.patch so far, so below are all about v3-0001.patch.

I am new to inheritance/partitioning codes and code review, so my review below might have some mistakes. If there are mistakes, please point out that kindly :)

v3-0001:
1. Is there any reason inheritance_make_rel_from_joinlist returns "parent" that is passed to it? I think we can refer parent in the caller even if inheritance_make_rel_from_joinlist returns nothing.

+static RelOptInfo *
+inheritance_make_rel_from_joinlist(PlannerInfo *root,
+								   RelOptInfo *parent,
+								   List *joinlist)
+{
    ...
+	return parent;
+}

2. Is there the possible case that IS_DUMMY_REL(child_joinrel) is true in inheritance_adjust_scanjoin_target()?

+inheritance_adjust_scanjoin_target(PlannerInfo *root,
	...
+{
	...
+	foreach(lc, root->inh_target_child_rels)
+	{
		...
+		/*
+		 * If the child was pruned, its corresponding joinrel will be empty,
+		 * which we can ignore safely.
+		 */
+		if (IS_DUMMY_REL(child_joinrel))
+			continue;

I think inheritance_make_rel_from_joinlist() doesn't make dummy joinrel for the child that was pruned.

+inheritance_make_rel_from_joinlist(PlannerInfo *root,
...
+{
	...
+	foreach(lc, root->append_rel_list)
+	{
+		RelOptInfo *childrel;
		...
+		/* Ignore excluded/pruned children. */
+		if (IS_DUMMY_REL(childrel))
+			continue;
		...
+		/*
+		 * Save child joinrel to be processed later in
+		 * inheritance_adjust_scanjoin_target, which adjusts its paths to
+		 * be able to emit expressions in query's top-level target list.
+		 */
+		root->inh_target_child_rels = lappend(root->inh_target_child_rels,
+											  childrel);
+	}
+}

3.
Is the "root->parse->commandType != CMD_INSERT" required in:

@@ -2018,13 +1514,45 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
...
+		/*
+		 * For UPDATE/DELETE on an inheritance parent, we must generate and
+		 * apply scanjoin target based on targetlist computed using each
+		 * of the child relations.
+		 */
+		if (parse->commandType != CMD_INSERT &&
+			current_rel->reloptkind == RELOPT_BASEREL &&
+			current_rel->relid == root->parse->resultRelation &&
+			root->simple_rte_array[current_rel->relid]->inh)
...

@@ -2137,92 +1665,123 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
final_rel->fdwroutine = current_rel->fdwroutine;

...
-	foreach(lc, current_rel->pathlist)
+	if (current_rel->reloptkind == RELOPT_BASEREL &&
+		current_rel->relid == root->parse->resultRelation &&
+		!IS_DUMMY_REL(current_rel) &&
+		root->simple_rte_array[current_rel->relid]->inh &&
+		parse->commandType != CMD_INSERT)

I think if a condition would be "current_rel->relid == root->parse->resultRelation && parse->commandType != CMD_INSERT" at the above if clause, elog() is called in the query_planner and planner don't reach above if clause.
Of course there is the case that query_planner returns the dummy joinrel and elog is not called, but in that case, current_rel->relid may be 0(current_rel is dummy joinrel) and root->parse->resultRelation may be not 0(a query is INSERT).

4. Can't we use define value(IS_PARTITIONED or something like IS_INHERITANCE?) to identify inheritance and partitioned table in below code? It was little confusing to me that which code is related to inheritance/partitioned when looking codes.

In make_one_rel():
+	if (root->parse->resultRelation &&
+		root->simple_rte_array[root->parse->resultRelation]->inh)
+	{
...
+		rel = inheritance_make_rel_from_joinlist(root, targetrel, joinlist);

In inheritance_make_rel_from_joinlist():
+		if (childrel->part_scheme != NULL)
+			childrel =
+				inheritance_make_rel_from_joinlist(root, childrel,
+												   translated_joinlist);

I can't review inheritance_adjust_scanjoin_target() deeply, because it is difficult to me to understand fully codes about join processing.

--
Yoshikazu Imai

Imai-san,

On 2018/10/04 17:11, Imai, Yoshikazu wrote:

Hi, Amit!

On Thu, Sept 13, 2018 at 10:29 PM, Amit Langote wrote:

Attached is what I have at the moment.

I also do the code review of the patch.

Thanks a lot for your review. I'm working on updating the patches as
mentioned upthread and should be able to post a new version sometime next
week. I will consider your comments along with David's and Dilip's before
posting the new patch.

Regards,
Amit

Hi Dilip,

Thanks for your review comments. Sorry it took me a while replying to them.

On 2018/09/29 22:30, Dilip Kumar wrote:

I was going through your patch (v3-0002) and I have some suggestions

1.
- if (nparts > 0)
+ /*
+ * For partitioned tables, we just allocate space for RelOptInfo's.
+ * pointers for all partitions and copy the partition OIDs from the
+ * relcache.  Actual RelOptInfo is built for a partition only if it is
+ * not pruned.
+ */
+ if (rte->relkind == RELKIND_PARTITIONED_TABLE)
+ {
rel->part_rels = (RelOptInfo **)
- palloc(sizeof(RelOptInfo *) * nparts);
+ palloc0(sizeof(RelOptInfo *) * rel->nparts);
+ return rel;
+ }

I think we can delay allocating memory for rel->part_rels? And we can
allocate in add_rel_partitions_to_query only
for those partitions which survive pruning.

Unfortunately, we can't do that. The part_rels array is designed such
that there is one entry in it for each partition of a partitioned table
that physically exists. Since the pruning code returns a set of indexes
into the array of *all* partitions, the planner's array must also be able
to hold *all* partitions, even if most of them would be NULL in the common
case.

Also, partition wise join code depends on finding a valid (even if dummy)
RelOptInfo for *all* partitions of a partitioned table to support outer
join planning. Maybe, we can change partition wise join code to not
depend on such dummy-marked RelOptInfos on the nullable side, but until
then we'll have to leave part_rels like this.

2.
add_rel_partitions_to_query
...
+ /* Expand the PlannerInfo arrays to hold new partition objects. */
+ num_added_parts = scan_all_parts ? rel->nparts :
+ bms_num_members(partindexes);
+ new_size = root->simple_rel_array_size + num_added_parts;
+ root->simple_rte_array = (RangeTblEntry **)
+ repalloc(root->simple_rte_array,
+ sizeof(RangeTblEntry *) * new_size);
+ root->simple_rel_array = (RelOptInfo **)
+ repalloc(root->simple_rel_array,
+ sizeof(RelOptInfo *) * new_size);
+ if (root->append_rel_array)
+ root->append_rel_array = (AppendRelInfo **)
+ repalloc(root->append_rel_array,
+ sizeof(AppendRelInfo *) * new_size);
+ else
+ root->append_rel_array = (AppendRelInfo **)
+ palloc0(sizeof(AppendRelInfo *) *
+ new_size);

Instead of re-pallocing for every partitioned relation can't we first
count the total number of surviving partitions and
repalloc at once.

Hmm, doing this seems a bit hard too. Since the function to perform
partition pruning (prune_append_rel_partitions), which determines the
number of partitions that will be added, currently contains a RelOptInfo
argument for using the partitioning info created by
set_relation_partition_info, we cannot call it on a partitioned table
unless we've created a RelOptInfo for it. So, we cannot delay creating
partition RelOptInfos to a point where we've figured out all the children,
because some of them might be partitioned tables themselves. IOW, we must
create them as we process each partitioned table (and its partitioned
children recursively) and expand the PlannerInfo arrays as we go.

I've thought about the alternative(s) that will allow us to do what you
suggest, but it cannot be implemented without breaking how we initialize
partitioning info in RelOptInfo. For example, we could refactor
prune_append_rel_array's interface to accept a Relation pointer instead of
RelOptInfo, but we don't have a Relation pointer handy at the point where
we can do pruning without re-opening it, which is something to avoid.
Actually, that's not the only refactoring needed as I've come to know when
trying to implement it.

On a related note, with the latest patch, I've also delayed regular
inheritance expansion as a whole, to avoid making the partitioning
expansion a special case. That means we'll expand the planner arrays
every time a inheritance parent is encountered in the range table. The
only aspect where partitioning becomes a special case in the new code is
that we can call the pruning code even before we've expanded the planner
arrays and the pruning limits the size to which the arrays must be
expanded to. Regular inheritance requires that the planner arrays be
expanded by the amount given by
list_length(find_all_inheritors(root_parent_oid)).

3.
+ /*
+ * And do prunin.  Note that this adds AppendRelInfo's of only the
+ * partitions that are not pruned.
+ */

prunin/pruning

I've since rewritten this comment and fixed the misspelling.

I'm still working on some other comments.

Thanks,
Amit

Hi David,

I've managed to get back to the rest of your comments. Sorry that it took
me a while.

On 2018/09/11 8:23, David Rowley wrote:

On 30 August 2018 at 21:29, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:

Attached updated patches, with 0002 containing the changes mentioned above.

Here's my first pass review on this:

0002:

10. I think moving the PlannerInfo->total_table_pages calculation
needs to be done in its own patch. This is a behavioural change where
we no longer count pruned relations in the calculation which can
result in plan changes. I posted a patch in [1] to fix this as a bug
fix as I think the current code is incorrect. My patch also updates
the first paragraph of the comment. You've not done that.

As mentioned on the other thread, I've included your patch in my own series.

11. "pruning"

+ * And do prunin. Note that this adds AppendRelInfo's of only the

Fixed.

12. It's much more efficient just to do bms_add_range() outside the loop in:

+ for (i = 0; i < rel->nparts; i++)
+ {
+ rel->part_rels[i] = build_partition_rel(root, rel,
+ rel->part_oids[i]);
+ rel->live_parts = bms_add_member(rel->live_parts, i);
+ }

You're right, I almost forgot about bms_add_range that we just recently added.

13. In set_append_rel_size() the foreach(l, root->append_rel_list)
loop could be made to loop over RelOptInfo->live_parts instead which
would save having to skip over AppendRelInfos that don't belong to
this parent. You'd need to make live_parts more general purpose and
also use it to mark children of inheritance parents.

Hmm, I've thought about it, but live_parts is a set of indexes into the
part_rels array, which is meaningful only for partitioned tables.

I'm not really sure if we should generalize part_rels to child_rels and
set it even for regular inheritance, if only for the efficiency of
set_append_rel_size and set_append_rel_pathlists.

In the updated patches, I've managed to make this whole effort applicable
to regular inheritance in general (as I said I would last month), where
the only special case code needed for partitioning is to utilize the
functionality of partprune.c, but I wasn't generalize everything. We can,
as a follow on patch maybe.

14. I think you can skip the following if both are NULL. You could
likely add more smarts for different join types, but I think you
should at least skip when both are NULL. Perhaps the loop could be
driven off of bms_intersec of the two Rel's live_parts?

+ if (child_rel1 == NULL)
+ child_rel1 = build_dummy_partition_rel(root, rel1, cnt_parts);
+ if (child_rel2 == NULL)
+ child_rel2 = build_dummy_partition_rel(root, rel2, cnt_parts);

Actually, not needing to build the dummy partition rel here is something
we could get back to doing someday, but not in this patch. The logic
involved to do so is too much related to join planning, so I thought we
could pursue it later.

15. The following is not required when append_rel_array was previously NULL.

+ MemSet(root->append_rel_array + root->simple_rel_array_size,
+    0, sizeof(AppendRelInfo *) * num_added_parts);

Yeah, I've fixed that. If previously NULL, it's palloc'd.

16. I don't think scan_all_parts is worth the extra code. The cost of
doing bms_num_members is going to be pretty small in comparison to
building paths for and maybe doing a join search for all partitions.

+ num_added_parts = scan_all_parts ? rel->nparts :
+ bms_num_members(partindexes);

In any case, you've got a bug in prune_append_rel_partitions() where
you're setting scan_all_parts to true instead of returning when
contradictory == true.

I too started disliking it, so got rid of scan_all_parts.

17. lockmode be of type LOCKMODE, not int.

+ Oid childOID, int lockmode,

This might no longer be the same code as you saw when reviewing, but I've
changed all lockmode variables in the patch to use LOCKMODE.

18. Comment contradicts the code.

+ /* Open rel if needed; we already have required locks */
+ if (childOID != parentOID)
+ {
+ childrel = heap_open(childOID, lockmode);

I think you should be using NoLock here.

19. Comment contradicts the code.

+ /* Close child relations, but keep locks */
+ if (childOID != parentOID)
+ {
+ Assert(childrel != NULL);
+ heap_close(childrel, lockmode);
+ }

These two blocks, too. There is no such code with the latest patch.
Code's been moved such that there is no need for such special handling.

20. I assume translated_vars can now be NIL due to
build_dummy_partition_rel() not setting it.

- if (var->varlevelsup == 0 && appinfo)
+ if (var->varlevelsup == 0 && appinfo && appinfo->translated_vars)

It might be worth writing a comment to explain that, otherwise it's
not quite clear why you're doing this.

Regarding this and and one more comment below, I've replied below.

21. Unrelated change;

Assert(relid > 0 && relid < root->simple_rel_array_size);
+

Couldn't find it in the latest patch, so must be gone.

22. The following comment mentions that Oids are copied, but that does
not happen in the code.

+ /*
+ * For partitioned tables, we just allocate space for RelOptInfo's.
+ * pointers for all partitions and copy the partition OIDs from the
+ * relcache.  Actual RelOptInfo is built for a partition only if it is
+ * not pruned.
+ */

The Oid copying already happened during get_relation_info().

Couldn't find this comment in the new code.

23. Traditionally translated_vars populated with a sequence of Vars in
the same order to mean no translation. Here you're changing how that
works:

+ /* leaving translated_vars to NIL to mean no translation needed */

This seems to be about the extent of your documentation on this, which
is not good enough.

I've changed the other code such that only the existing meaning of no-op
translation list is preserved, that is, the one you wrote above. I
modified build_dummy_partition_rel such that a no-op translated vars is
built based on parent's properties, instead of leaving it NIL. I've also
removed the special case code in adjust_appendrel_attrs_mutator which
checked translated_vars for NIL.

24. "each" -> "reach"? ... Actually, I don't understand the comment.
In a partitioned hierarchy, how can the one before the top-level
partitioned table not be a partitioned table?

/*
* Keep moving up until we each the parent rel that's not a
* partitioned table. The one before that one would be the root
* parent.
*/

This comment and the code has since been rewritten, but to clarify, this
is the new comment:

* Figuring out if we're the root partitioned table is a bit involved,
* because the root table mentioned in the query itself might be a
* child of UNION ALL subquery.
*/

25. "already"

+ * expand_inherited_rtentry alreay locked all partitions, so pass

No longer appears in the latest patch.

26. Your changes to make_partitionedrel_pruneinfo() look a bit broken.
You're wrongly assuming live_parts is the same as present_parts. If a
CHECK constraint eliminated the partition then those will be present
in live_parts but won't be part of the Append/MergeAppend subplans.
You might be able to maintain some of this optimisation by checking
for dummy rels inside the loop, but you're going to need to put back
the code that sets present_parts.

+ present_parts = bms_copy(subpart->live_parts);

You're right. The new code structure is such that it allows to delete the
partition index of a partition that's excluded by constraints, so I fixed
it so that live_parts no longer contains the partitions that are excluded
by constraints.

The existing code that sets present parts goes through all partitions by
looping over nparts members of part_rels, which is a pattern I think we
should avoid, as I'd think you'd agree.

27. Comment contradicts the code:

+ Bitmapset *live_parts; /* unpruned parts; NULL if all are live */

in add_rel_partitions_to_query() you're doing:

+ if (scan_all_parts)
+ {
+ for (i = 0; i < rel->nparts; i++)
+ {
+ rel->part_rels[i] = build_partition_rel(root, rel,
+ rel->part_oids[i]);
+ rel->live_parts = bms_add_member(rel->live_parts, i);
+ }
+ }

so the NULL part seems untrue.

I've rewritten the comment.

28. Missing comments:

+ TupleDesc tupdesc;
+ Oid reltype;

29. The comment for prune_append_rel_partitions claims it "Returns RT
indexes", but that's not the case, per:

-Relids
-prune_append_rel_partitions(RelOptInfo *rel)
+void
+prune_append_rel_partitions(PlannerInfo *root, RelOptInfo *rel)

Has changed in the latest patch and now it returns a bitmapset of
partition indexes, same as what get_matching_partitions does.

0003:

30. 2nd test can be tested inside the first test to remove redundant
partition check.

- inhOIDs = find_all_inheritors(parentOID, lockmode, NULL);
+ if (rte->relkind != RELKIND_PARTITIONED_TABLE)
+ inhOIDs = find_all_inheritors(parentOID, lockmode, NULL);

/*
* Check that there's at least one descendant, else treat as no-child
* case.  This could happen despite above has_subclass() check, if table
* once had a child but no longer does.
*/
- if (list_length(inhOIDs) < 2)
+ if (rte->relkind != RELKIND_PARTITIONED_TABLE && list_length(inhOIDs) < 2)
{

This code has changed quite a bit in the latest patch, so the comment no
longer applies.

31. The following code is wrong:

+ /* Determine the correct lockmode to use. */
+ if (rootRTindex == root->parse->resultRelation)
+ lockmode = RowExclusiveLock;
+ else if (rootrc && RowMarkRequiresRowShareLock(rootrc->markType))
+ lockmode = RowShareLock;
+ else
+ lockmode = AccessShareLock;

rootRTIndex remains at 0 if there are no row marks and resultRelation
will be 0 for SELECT queries, this means you'll use a RowExclusiveLock
for a SELECT instead of an AccessShareLock.

Why not just check the lockmode of the parent and use that?

No such logic exists anymore due to recent developments (addition of
rellockmode to RTE).

I'll post the latest patches in this week.

Thanks,
Amit

Imai-san,

Thank you for reviewing.

On 2018/10/04 17:11, Imai, Yoshikazu wrote:

Hi, Amit!

On Thu, Sept 13, 2018 at 10:29 PM, Amit Langote wrote:

Attached is what I have at the moment.

I also do the code review of the patch.
I could only see a v3-0001.patch so far, so below are all about v3-0001.patch.

I am new to inheritance/partitioning codes and code review, so my review below might have some mistakes. If there are mistakes, please point out that kindly :)

v3-0001:
1. Is there any reason inheritance_make_rel_from_joinlist returns "parent" that is passed to it? I think we can refer parent in the caller even if inheritance_make_rel_from_joinlist returns nothing.

+static RelOptInfo *
+inheritance_make_rel_from_joinlist(PlannerInfo *root,
+								   RelOptInfo *parent,
+								   List *joinlist)
+{
...
+	return parent;
+}

There used to be a reason to do that in previous versions, but seems it
doesn't hold anymore. I've changed it to not return any value.

2. Is there the possible case that IS_DUMMY_REL(child_joinrel) is true in inheritance_adjust_scanjoin_target()?

+inheritance_adjust_scanjoin_target(PlannerInfo *root,
...
+{
...
+	foreach(lc, root->inh_target_child_rels)
+	{
...
+		/*
+		 * If the child was pruned, its corresponding joinrel will be empty,
+		 * which we can ignore safely.
+		 */
+		if (IS_DUMMY_REL(child_joinrel))
+			continue;

I think inheritance_make_rel_from_joinlist() doesn't make dummy joinrel for the child that was pruned.

+inheritance_make_rel_from_joinlist(PlannerInfo *root,
...
+{
...
+	foreach(lc, root->append_rel_list)
+	{
+		RelOptInfo *childrel;
...
+		/* Ignore excluded/pruned children. */
+		if (IS_DUMMY_REL(childrel))
+			continue;
...
+		/*
+		 * Save child joinrel to be processed later in
+		 * inheritance_adjust_scanjoin_target, which adjusts its paths to
+		 * be able to emit expressions in query's top-level target list.
+		 */
+		root->inh_target_child_rels = lappend(root->inh_target_child_rels,
+											  childrel);
+	}
+}

You're right. Checking that in inheritance_adjust_scanjoin_target was
redundant.

3.
Is the "root->parse->commandType != CMD_INSERT" required in:

@@ -2018,13 +1514,45 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
...
+		/*
+		 * For UPDATE/DELETE on an inheritance parent, we must generate and
+		 * apply scanjoin target based on targetlist computed using each
+		 * of the child relations.
+		 */
+		if (parse->commandType != CMD_INSERT &&
+			current_rel->reloptkind == RELOPT_BASEREL &&
+			current_rel->relid == root->parse->resultRelation &&
+			root->simple_rte_array[current_rel->relid]->inh)
...

@@ -2137,92 +1665,123 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
final_rel->fdwroutine = current_rel->fdwroutine;

...
-	foreach(lc, current_rel->pathlist)
+	if (current_rel->reloptkind == RELOPT_BASEREL &&
+		current_rel->relid == root->parse->resultRelation &&
+		!IS_DUMMY_REL(current_rel) &&
+		root->simple_rte_array[current_rel->relid]->inh &&
+		parse->commandType != CMD_INSERT)

I think if a condition would be "current_rel->relid == root->parse->resultRelation && parse->commandType != CMD_INSERT" at the above if clause, elog() is called in the query_planner and planner don't reach above if clause.
Of course there is the case that query_planner returns the dummy joinrel and elog is not called, but in that case, current_rel->relid may be 0(current_rel is dummy joinrel) and root->parse->resultRelation may be not 0(a query is INSERT).

Yeah, I realized that we can actually Assert(parse->commandType !=
CMD_INSERT) if the inh flag of the target/resultRelation RTE is true. So,
checking it explicitly is redundant.

4. Can't we use define value(IS_PARTITIONED or something like IS_INHERITANCE?) to identify inheritance and partitioned table in below code? It was little confusing to me that which code is related to inheritance/partitioned when looking codes.

In make_one_rel():
+	if (root->parse->resultRelation &&
+		root->simple_rte_array[root->parse->resultRelation]->inh)
+	{
...
+		rel = inheritance_make_rel_from_joinlist(root, targetrel, joinlist);

In inheritance_make_rel_from_joinlist():
+		if (childrel->part_scheme != NULL)
+			childrel =
+				inheritance_make_rel_from_joinlist(root, childrel,
+												   translated_joinlist);

As it might have been clear, the value of inh flag is checked to detect
whether the operation uses inheritance, because it requires special
handling -- the operation must be applied to every child relation that
satisfies the conditions of the query.

part_scheme is checked to detect partitioning which has some special
behavior with respect to how the AppendRelInfos are set up. For
partitioning, AppendRelInfos map partitions to their immediate parent, not
the top-most root parent, so the current code uses recursion to apply
certain transformations. That's not required for non-partitioned case,
because all children are mapped to the root inheritance parent.

I'm trying to unify the two so that the partitioning case doesn't need any
special handling.

I can't review inheritance_adjust_scanjoin_target() deeply, because it is difficult to me to understand fully codes about join processing.

Thanks for your comments, they are valuable.

Regards,
Amit

And here is the latest set of patches. Sorry it took me a while.

* To reiterate, this set of patches is intended to delay adding partitions
to the query's range table and PlannerInfo, which leads to significantly
reduced overheads in the common case where queries accessing partitioned
tables need to scan only 1 or few partitions. As I mentioned before on
this thread [1]/messages/by-id/8097576f-f795-fc42-3c00-073f68bfb0b4@lab.ntt.co.jp, I wanted to change the patches such that the new approach
is adopted for all inheritance tables, not just partitioned tables,
because most of the code and data structures are shared and it no longer
made sense to me to create a diversion between the two cases by making the
partitioning a special case in a number of places in the planner code. I
think I've managed to get to that point with the latest patches. The new
code that performs inheritance expansion is common to partitioning,
inheritance, and also flattened UNION ALL to some degree, because all of
these cases use more or less the same code. For partitioning, child
relations are opened and RTEs are added for them *after* performing
pruning, whereas for inheritance that's not possible, because pruning
using constraint exclusion cannot be performed without opening the
children. This consideration is the only main thing that differs between
the handling of the two case, among other minor details that also differ.

* With the unification described above, inheritance expansion is no longer
part planner's prep phase, so I've located the new expansion code in a new
file under optimizer/utils named append.c, because it manages creation of
an appendrel's child relations. That means a lot of helper code that once
was in prepunion.c is now in the new file. Actually, I've moved *some* of
the helper routines such as adjust_appendre_* that do the mapping of
expressions between appendrel parent and children to a different file
appendinfo.c.

* I've also rewritten the patch to change how inherited update/delete
planning is done, so that it doesn't end up messing too much with
grouping_planner. Instead of removing inheritance_planner altogether as
the earlier patches did, I've rewritten it such that the CPU and memory
intensive query_planner portion is run only once at the beginning to
create scan/join paths for non-excluded/unpruned child target relations,
followed by running grouping_planner to apply top-level targetlist
suitable for each child target relation. grouping_planner is modified
slightly such that when it runs for every child target relation, it
doesn't again need to perform query planning. It instead receives the
RelOptInfos that contain the scan/join paths of child target relations
that were generated during the initial query_planner run. Beside this
main change, I've removed quite a bit of code in inheritance_planner that
relied on the existing way of redoing planning for each child target
relation. The changes are described in the commit message of patch 0002.

* A few notes on the patches:

0001 is a separate patch, because it might be useful in some other
projects like [2]/messages/by-id/de957e5b-faa0-6fb9-c0ab-0b389d71cf5a@lab.ntt.co.jp.

0003 is David's patch that he posted in [3]Re: Calculate total_table_pages after set_base_rel_sizes() /messages/by-id/CAKJS1f9NiQXO9KCv_cGgBShwqwT78wmArOht-5kWL+Bt0v-AnQ@mail.gmail.com.

I didn't get time today to repeat the performance tests, but I'm planning
to next week. In the meantime, please feel free to test them and provide
comments on the code changes.

Thanks,
Amit

[1]: /messages/by-id/8097576f-f795-fc42-3c00-073f68bfb0b4@lab.ntt.co.jp
/messages/by-id/8097576f-f795-fc42-3c00-073f68bfb0b4@lab.ntt.co.jp

[2]: /messages/by-id/de957e5b-faa0-6fb9-c0ab-0b389d71cf5a@lab.ntt.co.jp
/messages/by-id/de957e5b-faa0-6fb9-c0ab-0b389d71cf5a@lab.ntt.co.jp

[3]: Re: Calculate total_table_pages after set_base_rel_sizes() /messages/by-id/CAKJS1f9NiQXO9KCv_cGgBShwqwT78wmArOht-5kWL+Bt0v-AnQ@mail.gmail.com
/messages/by-id/CAKJS1f9NiQXO9KCv_cGgBShwqwT78wmArOht-5kWL+Bt0v-AnQ@mail.gmail.com

Hi,

On Thu, Oct 25, 2018 at 10:38 PM, Amit Langote wrote:

And here is the latest set of patches. Sorry it took me a while.

Thanks for revising the patch!

I didn't get time today to repeat the performance tests, but I'm planning
to next week. In the meantime, please feel free to test them and provide
comments on the code changes.

Since it takes me a lot of time to see all of the patches, I will post comments
little by little from easy parts like typo check.

1.
0002:
+ * inheritance_make_rel_from_joinlist
+ *		Perform join planning for all non-dummy leaf inheritance chilren
+ *		in their role as query's target relation

"inheritance chilren" -> "inheritance children"

2.
0002:
+		/*
+		 * Sub-partitions have to be processed recursively, because
+		 * AppendRelInfoss link sub-partitions to their immediate parents, not
+		 * the root partitioned table.
+		 */

AppendRelInfoss -> AppendRelInfos (?)

3.
0002:
+ /* Reset interal join planning data structures. */

interal -> internal

4.
0004:
-static void expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte,
- Index rti);

Comments referring to expand_inherited_rtentry() is left.

backend/optimizer/plan/planner.c:1310:
* Because of the way expand_inherited_rtentry works, that should be
backend/optimizer/plan/planner.c:1317:
* Instead the duplicate child RTE created by expand_inherited_rtentry
backend/optimizer/util/plancat.c:118:
* the rewriter or when expand_inherited_rtentry() added it to the query's
backend/optimizer/util/plancat.c:640:
* the rewriter or when expand_inherited_rtentry() added it to the query's

About the last two comments in the above,
"the rewriter or when expand_inherited_rtentry() added it to the query's"
would be
"the rewriter or when add_inheritance_child_rel() added it to the query's".

I don't know how to correct the first two comments.

5.
0004:
-static void expand_partitioned_rtentry(PlannerInfo *root,
- RangeTblEntry *parentrte,
- Index parentRTindex, Relation parentrel,
- PlanRowMark *top_parentrc, LOCKMODE lockmode,
- List **appinfos);

Comments referring to expand_partitioned_rtentry() is also left.

backend/executor/execPartition.c:941:
/*
* get_partition_dispatch_recurse
* Recursively expand partition tree rooted at rel
*
* As the partition tree is expanded in a depth-first manner, we maintain two
* global lists: of PartitionDispatch objects corresponding to partitioned
* tables in *pds and of the leaf partition OIDs in *leaf_part_oids.
*
* Note that the order of OIDs of leaf partitions in leaf_part_oids matches
* the order in which the planner's expand_partitioned_rtentry() processes
* them. It's not necessarily the case that the offsets match up exactly,
* because constraint exclusion might prune away some partitions on the
* planner side, whereas we'll always have the complete list; but unpruned
* partitions will appear in the same order in the plan as they are returned
* here.
*/

I think the second paragraph of the comments is no longer correct.
expand_partitioned_rtentry() expands the partition tree in a depth-first
manner, whereas expand_append_rel() doesn't neither in a depth-first manner
nor a breadth-first manner as below.

partitioned table tree image:
pt
sub1
sub1_1
leaf0
leaf1
sub2
leaf2
leaf3

append_rel_list(expanded by expand_partitioned_rtentry):
[sub1, sub1_1, leaf0, leaf1, sub2, leaf2, leaf3]

append_rel_list(expanded by expand_append_rel):
[sub1, sub2, leaf3, sub1_1, leaf1, leaf0, leaf2]

6.
0004
+	/*
+	 * A partitioned child table with 0 children is a dummy rel, so don't
+	 * bother creating planner objects for it.
+	 */
+	if (childrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+	{
+		PartitionDesc partdesc = RelationGetPartitionDesc(childrel);
+
+		Assert(!RELATION_IS_OTHER_TEMP(childrel));
+		if (partdesc->nparts == 0)
+		{
+			heap_close(childrel, NoLock);
+			return NULL;
+		}
+	}
+
+	/*
+	 * If childrel doesn't belong to this session, skip it, also relinquishing
+	 * the lock.
+	 */
+	if (RELATION_IS_OTHER_TEMP(childrel))
+	{
+		heap_close(childrel, lockmode);
+		return NULL;
+	}

If we process the latter if block before the former one, Assert can be excluded
from the code.

It might be difficult to me to examine the codes whether there exists any logical
wrongness along with significant planner code changes, but I will try to look it.
Since it passes make check-world successfully, I think as of now there might be
not any wrong points.

If there's time, I will also do the performance tests.

--
Yoshikazu Imai

About inheritance_make_rel_from_joinlist(), I considered how it processes
joins for sub-partitioned-table.

sub-partitioned-table image:
part
sub1
leaf1
leaf2

inheritance_make_rel_from_joinlist() translates join_list and join_info_list
for each leafs(leaf1, leaf2 in above image). To translate those two lists for
leaf1, inheritance_make_rel_from_joinlist() translates lists from part to sub1
and nextly from sub1 to leaf1. For leaf2, inheritance_make_rel_from_joinlist()
translates lists from part to sub1 and from sub1 to leaf2. There are same
translation from part to sub1, and I think it is better if we can eliminate it.
I attached 0002-delta.patch.

In the patch, inheritance_make_rel_from_joinlist() translates lists not only for
leafs but for mid-nodes, in a depth-first manner, so it can use lists of
mid-nodes for translating lists from mid-node to leafs, which eliminates same
translation.

I think it might be better if we can apply same logic to inheritance_planner(),
which once implemented the same logic as below.

foreach(lc, root->append_rel_list)
{
AppendRelInfo *appinfo = lfirst_node(AppendRelInfo, lc);
...
/*
* expand_inherited_rtentry() always processes a parent before any of
* that parent's children, so the parent_root for this relation should
* already be available.
*/
parent_root = parent_roots[appinfo->parent_relid];
Assert(parent_root != NULL);
parent_parse = parent_root->parse;
...
subroot->parse = (Query *)
adjust_appendrel_attrs(parent_root,
(Node *) parent_parse,
1, &appinfo);

--
Yoshikazu Imai

ISTM patch 0004 is impossible to review just because of size -- I
suppose the bulk of it is just code that moves from one file to another,
but that there are also code changes in it. Maybe it would be better to
split it so that one patch moves the code to the new files without
changing it, then another patch does the actual functional changes? If
git shows the first half as a rename, it's easier to be confident about
it.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Thanks for looking.

On 2018/11/07 12:32, Alvaro Herrera wrote:

ISTM patch 0004 is impossible to review just because of size -- I
suppose the bulk of it is just code that moves from one file to another,
but that there are also code changes in it. Maybe it would be better to
split it so that one patch moves the code to the new files without
changing it, then another patch does the actual functional changes? If
git shows the first half as a rename, it's easier to be confident about
it.

Okay, will post a new version structured that way shortly.

Thanks,
Amit

Imai-san,

Thank you for reviewing.

On 2018/11/05 17:28, Imai, Yoshikazu wrote:

Since it takes me a lot of time to see all of the patches, I will post comments
little by little from easy parts like typo check.

I've fixed the typos you pointed out.

4.
0004:
-static void expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte,
- Index rti);

Comments referring to expand_inherited_rtentry() is left.

backend/optimizer/plan/planner.c:1310:
* Because of the way expand_inherited_rtentry works, that should be
backend/optimizer/plan/planner.c:1317:
* Instead the duplicate child RTE created by expand_inherited_rtentry
backend/optimizer/util/plancat.c:118:
* the rewriter or when expand_inherited_rtentry() added it to the query's
backend/optimizer/util/plancat.c:640:
* the rewriter or when expand_inherited_rtentry() added it to the query's

About the last two comments in the above,
"the rewriter or when expand_inherited_rtentry() added it to the query's"
would be
"the rewriter or when add_inheritance_child_rel() added it to the query's".

I don't know how to correct the first two comments.

I've since modified the patch to preserve expand_inherited_rtentry name,
although with a new implementation.

5.
0004:
-static void expand_partitioned_rtentry(PlannerInfo *root,
- RangeTblEntry *parentrte,
- Index parentRTindex, Relation parentrel,
- PlanRowMark *top_parentrc, LOCKMODE lockmode,
- List **appinfos);

Comments referring to expand_partitioned_rtentry() is also left.

I've preserved this name as well.

backend/executor/execPartition.c:941:
/*
* get_partition_dispatch_recurse
* Recursively expand partition tree rooted at rel
*
* As the partition tree is expanded in a depth-first manner, we maintain two
* global lists: of PartitionDispatch objects corresponding to partitioned
* tables in *pds and of the leaf partition OIDs in *leaf_part_oids.
*
* Note that the order of OIDs of leaf partitions in leaf_part_oids matches
* the order in which the planner's expand_partitioned_rtentry() processes
* them. It's not necessarily the case that the offsets match up exactly,
* because constraint exclusion might prune away some partitions on the
* planner side, whereas we'll always have the complete list; but unpruned
* partitions will appear in the same order in the plan as they are returned
* here.
*/

I think the second paragraph of the comments is no longer correct.
expand_partitioned_rtentry() expands the partition tree in a depth-first
manner, whereas expand_append_rel() doesn't neither in a depth-first manner
nor a breadth-first manner as below.

Well, expand_append_rel doesn't process a whole partitioning tree on its
own, only one partitioned table at a time. As set_append_rel_size()
traverses the root->append_rel_list, leaf partitions get added to the
resulting plan in what's effectively a depth-first order.

I've updated this comment as far this patch is concerned, although the
patch in the other thread about removal of executor overhead in
partitioning is trying to remove this function
(get_partition_dispatch_recurse) altogether anyway.

6.
0004
+	/*
+	 * A partitioned child table with 0 children is a dummy rel, so don't
+	 * bother creating planner objects for it.
+	 */
+	if (childrel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+	{
+		PartitionDesc partdesc = RelationGetPartitionDesc(childrel);
+
+		Assert(!RELATION_IS_OTHER_TEMP(childrel));
+		if (partdesc->nparts == 0)
+		{
+			heap_close(childrel, NoLock);
+			return NULL;
+		}
+	}
+
+	/*
+	 * If childrel doesn't belong to this session, skip it, also relinquishing
+	 * the lock.
+	 */
+	if (RELATION_IS_OTHER_TEMP(childrel))
+	{
+		heap_close(childrel, lockmode);
+		return NULL;
+	}

If we process the latter if block before the former one, Assert can be excluded
from the code.

I've since moved the partitioning-related code to a partitioning specific
function, so this no longer applies.

Thanks,
Amit

Imai-san,

On 2018/11/07 10:00, Imai, Yoshikazu wrote:

About inheritance_make_rel_from_joinlist(), I considered how it processes
joins for sub-partitioned-table.

sub-partitioned-table image:
part
sub1
leaf1
leaf2

inheritance_make_rel_from_joinlist() translates join_list and join_info_list
for each leafs(leaf1, leaf2 in above image). To translate those two lists for
leaf1, inheritance_make_rel_from_joinlist() translates lists from part to sub1
and nextly from sub1 to leaf1. For leaf2, inheritance_make_rel_from_joinlist()
translates lists from part to sub1 and from sub1 to leaf2. There are same
translation from part to sub1, and I think it is better if we can eliminate it.
I attached 0002-delta.patch.

In the patch, inheritance_make_rel_from_joinlist() translates lists not only for
leafs but for mid-nodes, in a depth-first manner, so it can use lists of
mid-nodes for translating lists from mid-node to leafs, which eliminates same
translation.

I don't think the translation happens twice for the same leaf partitions.

Applying adjust_appendrel_attrs_*multilevel* for only leaf nodes, as
happens with the current code, is same as first translating using
adjust_appendrel_attrs from part to sub1 and then from sub1 to leaf1 and
leaf2 during recursion with sub1 as the parent.

I think it might be better if we can apply same logic to inheritance_planner(),
which once implemented the same logic as below.

foreach(lc, root->append_rel_list)
{
AppendRelInfo *appinfo = lfirst_node(AppendRelInfo, lc);
...
/*
* expand_inherited_rtentry() always processes a parent before any of
* that parent's children, so the parent_root for this relation should
* already be available.
*/
parent_root = parent_roots[appinfo->parent_relid];
Assert(parent_root != NULL);
parent_parse = parent_root->parse;
...
subroot->parse = (Query *)
adjust_appendrel_attrs(parent_root,
(Node *) parent_parse,
1, &appinfo);

Actually, inheritance_planner is also using
adjust_appendrel_attrs_multilevel. I'm not sure if you're looking at the
latest (10/26) patch.

Thanks,
Amit

On 2018/11/07 12:44, Amit Langote wrote:

Thanks for looking.

On 2018/11/07 12:32, Alvaro Herrera wrote:

ISTM patch 0004 is impossible to review just because of size -- I
suppose the bulk of it is just code that moves from one file to another,
but that there are also code changes in it. Maybe it would be better to
split it so that one patch moves the code to the new files without
changing it, then another patch does the actual functional changes? If
git shows the first half as a rename, it's easier to be confident about
it.

Okay, will post a new version structured that way shortly.

And here are patches structured that way. I've addressed some of the
comments posted by Imai-san upthread. Also, since David's patch to
postpone PlannerInfo.total_pages calculation went into the tree earlier
this week, it's no longer included in this set.

With this breakdown, patches are as follows:

v5-0001-Store-inheritance-root-parent-index-in-otherrel-s.patch

Adds a inh_root_parent field that's set in inheritance child otherrel
RelOptInfos to store the RT index of their root parent

v5-0002-Overhaul-inheritance-update-delete-planning.patch

Patch that adjusts planner so that inheritance_planner can use partition
pruning.

v5-0003-Lazy-creation-of-RTEs-for-inheritance-children.patch

Patch that adjusts planner such that inheritance expansion step is
postponed from early subquery_planner to the beginning of
set_append_rel_size, so that child tables are added to the Query and
PlannerInfo after partition pruning. While the move only benefits
partitioning, because non-partition children cannot be pruned until
after they're fully opened, the new code handles both partitioned tables
and regular inheritance parents.

v5-0004-Move-append-expansion-code-into-its-own-file.patch

With patch 0003, inheritance expansion is no longer a part of the prep
phase of planning, so it seems odd that inheritance expansion code is in
optimizer/prep/prepunion.c. This patch moves the code to a new file
optimizer/utils/append.c. Also, some of the appendrel translation
subroutines are moved over to optimizer/utils/appendinfo.c.

No functional changes with this patch.

v5-0005-Teach-planner-to-only-process-unpruned-partitions.patch

Patch that adds a live_parts field to RelOptInfo which is set in
partitioned rels to contain partition indexes of only non-dummy children
replace the loops of the following form:

for (i = 0; i < rel->nparts; i++)
{
RelOptInfo *partrel = rel->part_rels[i];

...some processing
}

at various points within the planner with:

i = -1
while ((i = bms_get_next(rel->live_parts)) >= 0)
{
RelOptInfo *partrel = rel->part_rels[i];

...some processing
}

v5-0006-Do-not-lock-all-partitions-at-the-beginning.patch

A simple patch that removes the find_all_inheritors called for
partitioned root parent only to lock *all* partitions, in favor of
locking only the unpruned partitions.

Thanks,
Amit

Hi Amit,

On 11/9/18 3:55 AM, Amit Langote wrote:

And here are patches structured that way. I've addressed some of the
comments posted by Imai-san upthread. Also, since David's patch to
postpone PlannerInfo.total_pages calculation went into the tree earlier
this week, it's no longer included in this set.

Thanks for doing the split this way. The patch passes check-world.

I ran a SELECT test using hash partitions, and got

Master v5
64: 6k 59k
1024: 283 59k

The non-partitioned case gives 77k. The difference in TPS between the
partition case vs. the non-partitioned case comes down to
set_plain_rel_size() vs. set_append_rel_size() under
set_base_rel_sizes(); flamegraphs for this sent off-list.

Best regards,
Jesper

On 9 November 2018 at 21:55, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:

v5-0001-Store-inheritance-root-parent-index-in-otherrel-s.patch

Adds a inh_root_parent field that's set in inheritance child otherrel
RelOptInfos to store the RT index of their root parent

v5-0002-Overhaul-inheritance-update-delete-planning.patch

Patch that adjusts planner so that inheritance_planner can use partition
pruning.

I've started looking at these two, but only so far made it through
0001 and 0002.

Here's what I noted down during the review.

0001:

1. Why do we need the new field that this patch adds? I see in 0002
it's used like:

+ if (childrel->inh_root_parent > 0 &&
+ childrel->inh_root_parent == root->parse->resultRelation)

Would something like...
int relid;
if (childrel->part_schema == NULL &&
bms_get_singleton_member(childrel->top_parent_relids, &relid) &&
relid == root->parse->resultRelation)

...not do the trick?

0002:

2. What's wrong with childrel->relids?

+ child_relids = bms_make_singleton(appinfo->child_relid);

3. Why not use childrel->top_parent_relids?

+ top_parent_relids = bms_make_singleton(childrel->inh_root_parent);

4. The following comment in inheritance_make_rel_from_joinlist()
implies that the function will not be called for SELECT, but the
comment above the function does not mention that.

/*
* For UPDATE/DELETE queries, the top parent can only ever be a table.
* As a contrast, it could be a UNION ALL subquery in the case of SELECT.
*/
Assert(planner_rt_fetch(top_parent_relid, root)->rtekind == RTE_RELATION);

5. Should the following comment talk about "Sub-partitioned tables"
rather than "sub-partitions"?

+ * Sub-partitions have to be processed recursively, because
+ * AppendRelInfos link sub-partitions to their immediate parents, not
+ * the root partitioned table.

6. Can't you just pass childrel->relids and
childrel->top_parent_relids instead of making new ones?

+ child_relids = bms_make_singleton(appinfo->child_relid);
+ Assert(childrel->inh_root_parent > 0);
+ top_parent_relids = bms_make_singleton(childrel->inh_root_parent);
+ translated_joinlist = (List *)
+ adjust_appendrel_attrs_multilevel(root,
+   (Node *) joinlist,
+   child_relids,
+   top_parent_relids);

7. I'm just wondering what your idea is here?

+ /* Reset join planning specific data structures. */
+ root->join_rel_list = NIL;
+ root->join_rel_hash = NULL;

Is there a reason to nullify these? You're not releasing any memory
and the new structures that will be built won't overlap with the ones
built last round. I don't mean to imply that the code is wrong, it
just does not appear to be particularly right.

8. In regards to:

+ * NB: Do we need to change the child EC members to be marked
+ * as non-child somehow?
+ */
+ childrel->reloptkind = RELOPT_BASEREL;

I know we talked a bit about this before, but this time I put together
a crude patch that runs some code each time we skip an em_is_child ==
true EquivalenceMember. The code checks if any of the em_relids are
RELOPT_BASEREL. What I'm looking for here are places where we
erroneously skip the member when we shouldn't. Running the regression
tests with this patch in place shows a number of problems. Likely I
should only trigger the warning when bms_membership(em->em_relids) ==
BMS_SINGLETON, but it never-the-less appears to highlight various
possible issues. Applying the same on master only appears to show the
cases where em->em_relids isn't a singleton set. I've attached the
patch to let you see what I mean.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hi Amit,

On Thu, Nov 8, 2018 at 8:26 PM, Amit Langote wrote:

On 2018/11/07 10:00, Imai, Yoshikazu wrote:

About inheritance_make_rel_from_joinlist(), I considered how it processes
joins for sub-partitioned-table.

sub-partitioned-table image:
part
sub1
leaf1
leaf2

inheritance_make_rel_from_joinlist() translates join_list and join_info_list
for each leafs(leaf1, leaf2 in above image). To translate those two lists for
leaf1, inheritance_make_rel_from_joinlist() translates lists from part to sub1
and nextly from sub1 to leaf1. For leaf2, inheritance_make_rel_from_joinlist()
translates lists from part to sub1 and from sub1 to leaf2. There are same
translation from part to sub1, and I think it is better if we can eliminate it.
I attached 0002-delta.patch.

In the patch, inheritance_make_rel_from_joinlist() translates lists not only for
leafs but for mid-nodes, in a depth-first manner, so it can use lists of
mid-nodes for translating lists from mid-node to leafs, which eliminates same
translation.

I don't think the translation happens twice for the same leaf partitions.

Applying adjust_appendrel_attrs_*multilevel* for only leaf nodes, as
happens with the current code, is same as first translating using
adjust_appendrel_attrs from part to sub1 and then from sub1 to leaf1 and
leaf2 during recursion with sub1 as the parent.

Thanks for replying.
I interpreted your thoughts about translation as below.

adjust_appendrel_attrs_multilevel for leaf1: root -> sub1 -> leaf1
adjust_appendrel_attrs_multilevel for leaf2: sub1(produced at above) -> leaf2

But I wonder translation of leaf2 actually reuses the results of sub1 which is
produced at leaf1 translation. ISTM translation for leaf1, leaf2 are executed
as below.

adjust_appendrel_attrs_multilevel for leaf1: root -> sub1 -> leaf1
adjust_appendrel_attrs_multilevel for leaf2: root -> sub1 -> leaf2

I think it might be better if we can apply same logic to inheritance_planner(),
which once implemented the same logic as below.

foreach(lc, root->append_rel_list)
{
AppendRelInfo *appinfo = lfirst_node(AppendRelInfo, lc);
...
/*
* expand_inherited_rtentry() always processes a parent before any of
* that parent's children, so the parent_root for this relation should
* already be available.
*/
parent_root = parent_roots[appinfo->parent_relid];
Assert(parent_root != NULL);
parent_parse = parent_root->parse;
...
subroot->parse = (Query *)
adjust_appendrel_attrs(parent_root,
(Node *) parent_parse,
1, &appinfo);

Actually, inheritance_planner is also using
adjust_appendrel_attrs_multilevel. I'm not sure if you're looking at the
latest (10/26) patch.

Sorry for my poor explanation. I described the above code as old code which is
not patch applied.

Since it is difficult to explain my thoughts with words, I will show the
performance degration case.

Partition tables are below two sets.

Set1:
[create 800 partitions directly under root]
CREATE TABLE rt (a int, b int) PARTITION BY RANGE (a);
\o /dev/null
SELECT 'CREATE TABLE leaf' || x::text || ' PARTITION OF rt FOR VALUES FROM ('
|| (x)::text || ') TO (' || (x+1)::text || ');' FROM generate_series(0, 800) x;
\gexec
\o

Set2:
[create 800 partitions under a partitioned table which is under root]
CREATE TABLE rt (a int, b int) PARTITION BY RANGE (a);
CREATE TABLE sub1 PARTITION OF rt FOR VALUES FROM (0) TO (100) PARTITION BY
RANGE (b);
\o /dev/null
SELECT 'CREATE TABLE leaf' || x::text || ' PARTITION OF sub1 FOR VALUES FROM ('
|| (x)::text || ') TO (' || (x+1)::text || ');' FROM generate_series(0, 800) x;
\gexec
\o

Create a generic plan of updation or deletion.

[create a delete generic plan]
set plan_cache_mode = 'force_generic_plan';
prepare delete_stmt(int) as delete from rt where b = $1;
execute delete_stmt(1);

In creating generic plan, paths/plans for all partitions are created because
we don't know which plan is used before "EXECUTE" command happens.
In creating paths in inheritance_planner(),
adjust_appendrel_attrs()/adjust_appendrel_attrs_multilevel() is executed many
times and it allocates a lot of memory in total if there are many partitions.

How amount of memory is used with above tests is...

without v5 patches, Set1: 242MB
without v5 patches, Set2: 247MB
with v5 patches, Set1: 420MB
with v5 patches, Set2: 820MB
# Thanks for supplying v5 patches :)

Without sub-partition(Set1), memory allocated by adjust_appendrel_attrs() with
v5 patches is 1.7x larger than without v5 patches. With sub-partition(Set2),
memory allocated by adjust_appendrel_attrs() with v5 patches is 3.3x larger
than without v5 patches.
I think why memory usage in "with v5 patches, Set2" is almost 2 times than
"with v5 patches, Set1" is that adjust_appendrel_attrs() from root to sub1 is
occurred at each translation for each partitions in "with v5 patches, Set2".

Currently, a query to a partition table is processed faster by a custom plan
than by a generic plan, so we would not use a generic plan that I don't know
whether we should solve this large memory allocation problem.

--
Yoshikazu Imai

David, Imai-san,

Thanks for reviewing. I've included both replies in this email so that I
can attach the latest patch as well.

On 2018/11/10 20:59, David Rowley wrote:

I've started looking at these two, but only so far made it through
0001 and 0002.

Here's what I noted down during the review.

0001:

1. Why do we need the new field that this patch adds? I see in 0002
it's used like:

+ if (childrel->inh_root_parent > 0 &&
+ childrel->inh_root_parent == root->parse->resultRelation)

Would something like...
int relid;
if (childrel->part_schema == NULL &&
bms_get_singleton_member(childrel->top_parent_relids, &relid) &&
relid == root->parse->resultRelation)

...not do the trick?

Actually, that's one way and earlier patches relied on that, but it gets a
bit ugly given that it's not always the top_parent_relid we're looking for
in the partitioning/inheritance specific code. Consider this:

select *
from (select a from parted_table p union all
select a from inherited_table i) s
where s.a = 1;

top_parent_relids refers to the union all parent subquery 's RT index,
which makes the partitioning/inheritance code scramble through a chain of
parent relation relids to figure out the RT index of the table in the query.

So, inh_root_parent is useful to distinguish the inheritance root parent
from the Append relation root parent, without much code.

That said, in the latest version, I've modified the UPDATE/DELETE planning
patch such that it doesn't use inh_root_parent (or the new code doesn't
need to refer to root parent where it previously did), so I've moved the
patch that adds inh_root_parent to the 2nd in the series.

0002:

2. What's wrong with childrel->relids?

+ child_relids = bms_make_singleton(appinfo->child_relid);

I've modified the code to not have to use child_relids.

3. Why not use childrel->top_parent_relids?

+ top_parent_relids = bms_make_singleton(childrel->inh_root_parent);

Ditto.

4. The following comment in inheritance_make_rel_from_joinlist()
implies that the function will not be called for SELECT, but the
comment above the function does not mention that.

/*
* For UPDATE/DELETE queries, the top parent can only ever be a table.
* As a contrast, it could be a UNION ALL subquery in the case of SELECT.
*/
Assert(planner_rt_fetch(top_parent_relid, root)->rtekind == RTE_RELATION);

Fixed the function's header comment to be clear.

5. Should the following comment talk about "Sub-partitioned tables"
rather than "sub-partitions"?

+ * Sub-partitions have to be processed recursively, because
+ * AppendRelInfos link sub-partitions to their immediate parents, not
+ * the root partitioned table.

Okay, done.

6. Can't you just pass childrel->relids and
childrel->top_parent_relids instead of making new ones?

+ child_relids = bms_make_singleton(appinfo->child_relid);
+ Assert(childrel->inh_root_parent > 0);
+ top_parent_relids = bms_make_singleton(childrel->inh_root_parent);
+ translated_joinlist = (List *)
+ adjust_appendrel_attrs_multilevel(root,
+   (Node *) joinlist,
+   child_relids,
+   top_parent_relids);

The new code uses adjust_appendrel_attrs, so those Relids variables are
not needed.

7. I'm just wondering what your idea is here?

+ /* Reset join planning specific data structures. */
+ root->join_rel_list = NIL;
+ root->join_rel_hash = NULL;

Is there a reason to nullify these? You're not releasing any memory
and the new structures that will be built won't overlap with the ones
built last round. I don't mean to imply that the code is wrong, it
just does not appear to be particularly right.

In initial versions of the patch, the same top-level PlannerInfo was used
when calling make_rel_from_joinlist for all child tables. So,
join_rel_hash built for a given child would be assumed to be valid for the
next which isn't true, because joinrels would be different across children.

In the latest patch, inheritance_make_rel_from_joinlist uses different
PlannerInfos for different child target relations, so the above problem
doesn't exist. IOW, you won't see above two lines in the latest patch.

8. In regards to:

+ * NB: Do we need to change the child EC members to be marked
+ * as non-child somehow?
+ */
+ childrel->reloptkind = RELOPT_BASEREL;

I know we talked a bit about this before, but this time I put together
a crude patch that runs some code each time we skip an em_is_child ==
true EquivalenceMember. The code checks if any of the em_relids are
RELOPT_BASEREL. What I'm looking for here are places where we
erroneously skip the member when we shouldn't. Running the regression
tests with this patch in place shows a number of problems. Likely I
should only trigger the warning when bms_membership(em->em_relids) ==
BMS_SINGLETON, but it never-the-less appears to highlight various
possible issues. Applying the same on master only appears to show the
cases where em->em_relids isn't a singleton set. I've attached the
patch to let you see what I mean.

Thanks for this. I've been thinking about what to do about it, but
haven't decided what's that yet. Please let me spend some more time
thinking on it. AFAICT, dealing with this will ensure that join planning
against target child relations can use EC-based optimizations, but it's
not incorrect as is per se.

On 2018/11/12 13:35, Imai, Yoshikazu wrote:

adjust_appendrel_attrs_multilevel for leaf1: root -> sub1 -> leaf1
adjust_appendrel_attrs_multilevel for leaf2: root -> sub1 -> leaf2

Ah, I see what you mean.

The root -> sub1 translation will be repeated for each leaf partition if
done via adjust_appendrel_attrs_multilevel. On the other hand, if we
could do the root to sub1 translation once and pass it to the recursive
call using sub1 as the parent.

I've changed the patch use adjust_appendrel_attrs.

Since it is difficult to explain my thoughts with words, I will show the
performance degration case.

Partition tables are below two sets.

[ ... ]

Create a generic plan of updation or deletion.

[create a delete generic plan]
set plan_cache_mode = 'force_generic_plan';
prepare delete_stmt(int) as delete from rt where b = $1;
execute delete_stmt(1);

[ ... ]

How amount of memory is used with above tests is...

without v5 patches, Set1: 242MB
without v5 patches, Set2: 247MB
with v5 patches, Set1: 420MB
with v5 patches, Set2: 820MB

Although I didn't aim to fix planning for the generic plan case where no
pruning occurs, the above result is not acceptable. That is, the new
implementation of inheritance update/delete planning shouldn't consume
more memory than the previous. In fact, it should've consumed less,
because the patch claims that it gets rid of redundant processing per
partition.

I understood why update/delete planning consumed more memory with the
patch. It was due to a problem with the patch that modifies inheritance
update/delete planning. The exact problem was that the query tree would
be translated (hence copied) *twice* for every partition! First during
query planning where the query tree would be translated to figure out a
targetlist for partitions and then again before calling grouping_planner.
Also, the adjust_appendrel_attrs_multilevel made it worse for multi-level
partitioning case, because of repeated copying for root to intermediate
partitioned tables, as Imai-san pointed out.

I've fixed that making sure that query tree is translated only once and
saved for later steps to use. Imai-san, please check the memory
consumption with the latest patch.

Attached updated patches. Significant revisions are as follows (note that
I reversed the order of 0001 and 0002):

v6-0001-Overhaul-inheritance-update-delete-planning.patch

The major changes is fixing the problem above.

v6-0002-Store-inheritance-root-parent-index-in-otherrel-s.patch

No change.

v6-0003-Lazy-creation-of-RTEs-for-inheritance-children.patch

Made inheritance expansion a separate step of make_one_rel whereas
previously it would be invoked at the beginning of set_append_rel_size.
Now, it runs just before set_base_rel_sizes. The same step also
recursively expands (and performs pruning for) any child partitioned
tables that were added by the expansion of partitioned tables originally
mentioned in the query. With this change, we don't need to worry about
the range table changing as set_base_rel_size is executing, which could
lead to problems.

v6-0004-Move-append-expansion-code-into-its-own-file.patch
v6-0005-Teach-planner-to-only-process-unpruned-partitions.patch
v6-0006-Do-not-lock-all-partitions-at-the-beginning.patch

No change.

Thanks,
Amit

On 2018/11/14 19:28, Amit Langote wrote:

Attached updated patches. Significant revisions are as follows (note that
I reversed the order of 0001 and 0002):

v6-0001-Overhaul-inheritance-update-delete-planning.patch

The major changes is fixing the problem above.

v6-0002-Store-inheritance-root-parent-index-in-otherrel-s.patch

No change.

v6-0003-Lazy-creation-of-RTEs-for-inheritance-children.patch

Made inheritance expansion a separate step of make_one_rel whereas
previously it would be invoked at the beginning of set_append_rel_size.
Now, it runs just before set_base_rel_sizes. The same step also
recursively expands (and performs pruning for) any child partitioned
tables that were added by the expansion of partitioned tables originally
mentioned in the query. With this change, we don't need to worry about
the range table changing as set_base_rel_size is executing, which could
lead to problems.

v6-0004-Move-append-expansion-code-into-its-own-file.patch
v6-0005-Teach-planner-to-only-process-unpruned-partitions.patch
v6-0006-Do-not-lock-all-partitions-at-the-beginning.patch

This went out sooner than it should have. I hadn't waited for make
check-world to finish which showed that a file_fdw test exercising
inheritance crashed with 0001 patch due to a bogus Assert.

Fixed it in the attached version.

Thanks,
Amit

Hi Amit,

On Tue, Nov 13, 2018 at 10:29 PM, Amit Langote wrote:

On 2018/11/12 13:35, Imai, Yoshikazu wrote:

adjust_appendrel_attrs_multilevel for leaf1: root -> sub1 -> leaf1
adjust_appendrel_attrs_multilevel for leaf2: root -> sub1 -> leaf2

Ah, I see what you mean.

The root -> sub1 translation will be repeated for each leaf partition
if done via adjust_appendrel_attrs_multilevel. On the other hand, if
we could do the root to sub1 translation once and pass it to the recursive
call using sub1 as the parent.

I've changed the patch use adjust_appendrel_attrs.

Since it is difficult to explain my thoughts with words, I will show
the performance degration case.

Partition tables are below two sets.

[ ... ]

Create a generic plan of updation or deletion.

[create a delete generic plan]
set plan_cache_mode = 'force_generic_plan'; prepare delete_stmt(int)
as delete from rt where b = $1; execute delete_stmt(1);

[ ... ]

How amount of memory is used with above tests is...

without v5 patches, Set1: 242MB
without v5 patches, Set2: 247MB
with v5 patches, Set1: 420MB
with v5 patches, Set2: 820MB

Although I didn't aim to fix planning for the generic plan case where
no pruning occurs, the above result is not acceptable. That is, the new
implementation of inheritance update/delete planning shouldn't consume
more memory than the previous. In fact, it should've consumed less,
because the patch claims that it gets rid of redundant processing per
partition.

I understood why update/delete planning consumed more memory with the
patch. It was due to a problem with the patch that modifies inheritance
update/delete planning. The exact problem was that the query tree would
be translated (hence copied) *twice* for every partition! First during
query planning where the query tree would be translated to figure out
a targetlist for partitions and then again before calling
grouping_planner.
Also, the adjust_appendrel_attrs_multilevel made it worse for
multi-level partitioning case, because of repeated copying for root to
intermediate partitioned tables, as Imai-san pointed out.

I've fixed that making sure that query tree is translated only once and
saved for later steps to use. Imai-san, please check the memory
consumption with the latest patch.

Thanks for fixing!
Now, memory consumption is lower than the previous.

with v7 patches, Set1: 223MB
with v7 patches, Set2: 226MB

Thanks,

--
Yoshikazu Imai

On 2018/11/15 10:19, Imai, Yoshikazu wrote:

On Tue, Nov 13, 2018 at 10:29 PM, Amit Langote wrote:

On 2018/11/12 13:35, Imai, Yoshikazu wrote:

How amount of memory is used with above tests is...

without v5 patches, Set1: 242MB
without v5 patches, Set2: 247MB
with v5 patches, Set1: 420MB
with v5 patches, Set2: 820MB

I understood why update/delete planning consumed more memory with the
patch. It was due to a problem with the patch that modifies inheritance
update/delete planning. The exact problem was that the query tree would
be translated (hence copied) *twice* for every partition! First during
query planning where the query tree would be translated to figure out
a targetlist for partitions and then again before calling
grouping_planner.
Also, the adjust_appendrel_attrs_multilevel made it worse for
multi-level partitioning case, because of repeated copying for root to
intermediate partitioned tables, as Imai-san pointed out.

I've fixed that making sure that query tree is translated only once and
saved for later steps to use. Imai-san, please check the memory
consumption with the latest patch.

Thanks for fixing!
Now, memory consumption is lower than the previous.

with v7 patches, Set1: 223MB
with v7 patches, Set2: 226MB

Thanks for checking. So at least we no longer have any memory
over-allocation bug with the patch, but perhaps other bugs are still
lurking. :)

Regards,
Amit

On 2018/11/14 19:28, Amit Langote wrote:

On 2018/11/10 20:59, David Rowley wrote:

8. In regards to:

+ * NB: Do we need to change the child EC members to be marked
+ * as non-child somehow?
+ */
+ childrel->reloptkind = RELOPT_BASEREL;

I know we talked a bit about this before, but this time I put together
a crude patch that runs some code each time we skip an em_is_child ==
true EquivalenceMember. The code checks if any of the em_relids are
RELOPT_BASEREL. What I'm looking for here are places where we
erroneously skip the member when we shouldn't. Running the regression
tests with this patch in place shows a number of problems. Likely I
should only trigger the warning when bms_membership(em->em_relids) ==
BMS_SINGLETON, but it never-the-less appears to highlight various
possible issues. Applying the same on master only appears to show the
cases where em->em_relids isn't a singleton set. I've attached the
patch to let you see what I mean.

Thanks for this. I've been thinking about what to do about it, but
haven't decided what's that yet. Please let me spend some more time
thinking on it. AFAICT, dealing with this will ensure that join planning
against target child relations can use EC-based optimizations, but it's
not incorrect as is per se.

I've been considered this a bit more and have some observations to share.
I found that the new inheritance_planner causes regression when the query
involves equivalence classes referencing the target relation, such as in
the following example:

create table ht (a int, b int) partition by hash (a);
create table ht1 partition of ht for values with (modulus 1024, remainder 0);
...
create table ht1024 partition of ht for values with (modulus 1024,
remainder 1023);
create table foo (a int, b int);
update ht set a = foo.a from foo where ht.b = foo.b;

For the above query, an EC containing ht.b and foo.b would be built. With
the new approach this EC will need to be expanded to add em_is_child EC
members for all un-pruned child tables, whereas with the previous approach
there would be no child members because the EC would be built for the
child as the query's target relation to begin with. So, with the old
approach there will be {ht1.b, foo.b} for query with ht1 as target
relation, {ht2.b, foo.b} for query with ht2 as target relation and so on.
Whereas with the new approach there will be just one query_planner run and
resulting EC will be {foo.b, ht.b, ht1.b, ht2.b, ...}. So, the planning
steps that manipulate ECs now have to iterate through many members and
become a bottleneck if there are many un-pruned children. To my surprise,
those bottlenecks are worse than having to rerun query_planner for each
child table.

So with master, I get the following planning time for the above update
query which btw doesn't prune (3 repeated runs)

Planning Time: 688.830 ms
Planning Time: 690.950 ms
Planning Time: 704.702 ms

And with the previous v7 patch:

Planning Time: 1373.398 ms
Planning Time: 1360.685 ms
Planning Time: 1356.313 ms

I've fixed that in the attached by utilizing the fact that now we build
the child PlannerInfo before we add child EC members. By modifying
add_child_rel_equivalences such that it *replaces* the parent EC member
with the corresponding child member instead of appending it to the list,
if the child is the target relation. That happens inside the child
target's private PlannerInfo, so it's harmless. Also, it is no longer
marked as em_is_child=true, so as a whole, this more or less restores the
original behavior wrt to ECs (also proved by the fact that I now get the
same regression.diffs by applying David's verify_em_child.diff patch [1]/messages/by-id/CAKJS1f8g9_BzE678BLBm-eoMMEYUUXhDABSpqtAHRUUTrm_vFA@mail.gmail.com
to the patched tree as with applying it to master modulo the varno
differences due to the patched).

With the attached updated patch (again this is with 0 partitions pruned):

Planning Time: 332.503 ms
Planning Time: 334.003 ms
Planning Time: 334.212 ms

If I add an additional condition so that only 1 partition is joined with
foo and the rest pruned, such as the following query (the case we're
trying to optimize):

update ht set a = foo.a from foo where foo.a = ht.b and foo.a = 1

I get the following numbers with master (no change from 0 pruned case):

Planning Time: 727.473 ms
Planning Time: 726.145 ms
Planning Time: 734.458 ms

But with the patches:

Planning Time: 0.797 ms
Planning Time: 0.751 ms
Planning Time: 0.801 ms

Attached v8 patches.

Thanks,
Amit

[1]: /messages/by-id/CAKJS1f8g9_BzE678BLBm-eoMMEYUUXhDABSpqtAHRUUTrm_vFA@mail.gmail.com
/messages/by-id/CAKJS1f8g9_BzE678BLBm-eoMMEYUUXhDABSpqtAHRUUTrm_vFA@mail.gmail.com

Hi Amit,

On Tue, Nov 20, 2018 at 10:24 PM, Amit Langote wrote:

Attached v8 patches.

Thanks for the patch. I took a look 0003, 0005, 0006 of v8 patch.

1.
0003: line 267-268
+		 * Child relation may have be marked dummy if build_append_child_rel
+		 * found self-contradictory quals.

/s/have be marked/have been marked/

2.
0003: around line 1077
In append.c(or prepunion.c)
228 * Check that there's at least one descendant, else treat as no-child
229 * case. This could happen despite above has_subclass() check, if table
230 * once had a child but no longer does.

has_subclass() check is moved to subquery_planner from above this code,
so the comments need to be modified like below.

s/above has_subclass() check/has_subclass() check in subquery_planner/

3.
0003: line 1241-1244
0006: line ?

In comments of expand_partitioned_rtentry:
+ * Note: even though only the unpruned partitions will be added to the
+ * resulting plan, this still locks *all* partitions via find_all_inheritors
+ * in order to avoid partitions being locked in a different order than other
+ * places in the backend that may lock partitions.

This comments is no more needed if 0006 patch is applied because
find_all_inheritors is removed in the 0006 patch.

4.
0003: line 1541-1544

+	 * Add the RelOptInfo.  Even though we may not really scan this relation
+	 * for reasons such as contradictory quals, we still need need to create
+	 * one, because for every RTE in the query's range table, there must be an
+	 * accompanying RelOptInfo.

s/need need/need/

5.
0003: line 1620-1621

+ * After creating the RelOptInfo for the given child RT index, it goes on to
+ * initialize some of its fields base on the parent RelOptInfo.

s/fields base on/fields based on/

6.
parsenodes.h
906 * inh is true for relation references that should be expanded to include
907 * inheritance children, if the rel has any. This *must* be false for
908 * RTEs other than RTE_RELATION entries.

I think inh can become true now even if RTEKind equals RTE_SUBQUERY, so latter
sentence need to be modified.

7.
0005: line 109-115
+		/*
+		 * If partition is excluded by constraints, remove it from
+		 * live_parts, too.
+		 */
+		if (IS_DUMMY_REL(childrel))
+			parentrel->live_parts = bms_del_member(parentrel->live_parts, i);
+

When I read this comment, I imagined that relation_excluded_by_constraints()
would be called before this code. childrel is marked dummy if
build_append_child_rel found self-contradictory quals, so comments can be
modified more correctly like another comments in your patch as below.

In 0003: line 267-271
+		 * Child relation may have be marked dummy if build_append_child_rel
+		 * found self-contradictory quals.
+		 */
+		if (IS_DUMMY_REL(childrel))
+			continue;

8.
0003: line 705-711
+		 * Query planning may have added some columns to the top-level tlist,
+		 * which happens when there are row marks applied to inheritance
+		 * parent relations (additional junk columns needed for applying row
+		 * marks are added after expanding inheritance.)
+		 */
+		if (list_length(tlist) < list_length(root->processed_tlist))
+			tlist = root->processed_tlist;

In grouping_planner():

if (planned_rel == NULL)
{
...
root->processed_tlist = tlist;
}
else
tlist = root->processed_tlist;
...
if (current_rel == NULL)
current_rel = query_planner(root, tlist,
standard_qp_callback, &qp_extra);
...
/*
* Query planning may have added some columns to the top-level tlist,
* which happens when there are row marks applied to inheritance
* parent relations (additional junk columns needed for applying row
* marks are added after expanding inheritance.)
*/
if (list_length(tlist) < list_length(root->processed_tlist))
tlist = root->processed_tlist;

Are there any case tlist points to an address different from
root->processed_tlist after calling query_planner? Junk columns are possibly
added to root->processed_tlist after expanding inheritance, but that adding
process don't change the root->processed_tlist's pointer address.
I checked "Assert(tlist == root->processed_tlist)" after calling query_planner
passes "make check".

9.
0003: line 1722-1763
In build_append_child_rel():

+	/*
+	 * In addition to the quals inherited from the parent, we might
+	 * have securityQuals associated with this particular child node.
+	 * (Currently this can only happen in appendrels originating from
+	 * UNION ALL; inheritance child tables don't have their own
+	 * securityQuals.)	Pull any such securityQuals up into the
...
+		foreach(lc, childRTE->securityQuals)
+		{
...
+		}
+		Assert(security_level <= root->qual_security_level);
+	}

This foreach loop loops only once in the current regression tests. I checked
"Assert(childRTE->securityQuals->length == 1)" passes "make check".
I think there are no need to change codes, I state this fact only for sharing.

--
Yoshikazu Imai

Imai-san,

Thanks for the review again.

On 2018/12/05 11:29, Imai, Yoshikazu wrote:

On Tue, Nov 20, 2018 at 10:24 PM, Amit Langote wrote:

Attached v8 patches.

Thanks for the patch. I took a look 0003, 0005, 0006 of v8 patch.

1.
0003: line 267-268
+		 * Child relation may have be marked dummy if build_append_child_rel
+		 * found self-contradictory quals.

/s/have be marked/have been marked/

2.
0003: around line 1077
In append.c(or prepunion.c)
228 * Check that there's at least one descendant, else treat as no-child
229 * case. This could happen despite above has_subclass() check, if table
230 * once had a child but no longer does.

has_subclass() check is moved to subquery_planner from above this code,
so the comments need to be modified like below.

s/above has_subclass() check/has_subclass() check in subquery_planner/

3.
0003: line 1241-1244
0006: line ?

In comments of expand_partitioned_rtentry:
+ * Note: even though only the unpruned partitions will be added to the
+ * resulting plan, this still locks *all* partitions via find_all_inheritors
+ * in order to avoid partitions being locked in a different order than other
+ * places in the backend that may lock partitions.

This comments is no more needed if 0006 patch is applied because
find_all_inheritors is removed in the 0006 patch.

4.
0003: line 1541-1544

+	 * Add the RelOptInfo.  Even though we may not really scan this relation
+	 * for reasons such as contradictory quals, we still need need to create
+	 * one, because for every RTE in the query's range table, there must be an
+	 * accompanying RelOptInfo.

s/need need/need/

5.
0003: line 1620-1621

+ * After creating the RelOptInfo for the given child RT index, it goes on to
+ * initialize some of its fields base on the parent RelOptInfo.

s/fields base on/fields based on/

Fixed all of 1-5.

6.
parsenodes.h
906 * inh is true for relation references that should be expanded to include
907 * inheritance children, if the rel has any. This *must* be false for
908 * RTEs other than RTE_RELATION entries.

I think inh can become true now even if RTEKind equals RTE_SUBQUERY, so latter
sentence need to be modified.

Seems like an existing comment bug. Why don't you send a patch as you
discovered it? :)

7.
0005: line 109-115
+		/*
+		 * If partition is excluded by constraints, remove it from
+		 * live_parts, too.
+		 */
+		if (IS_DUMMY_REL(childrel))
+			parentrel->live_parts = bms_del_member(parentrel->live_parts, i);
+

When I read this comment, I imagined that relation_excluded_by_constraints()
would be called before this code. childrel is marked dummy if
build_append_child_rel found self-contradictory quals, so comments can be
modified more correctly like another comments in your patch as below.

I realized that bms_del_member statement is unnecessary. I've revised the
comment describing live_parts to say that it contains indexes into
part_rels array of the non-NULL RelOptInfos contained in it, that is,
RelOptInfos of un-pruned partitions (rest of the entries are NULL.)
Un-pruned partitions may become dummy due to contradictory constraints or
constraint exclusion using normal CHECK constraints later and whether it's
dummy is checked properly by functions that iterate over live_parts.

8.
0003: line 705-711
+		 * Query planning may have added some columns to the top-level tlist,
+		 * which happens when there are row marks applied to inheritance
+		 * parent relations (additional junk columns needed for applying row
+		 * marks are added after expanding inheritance.)
+		 */
+		if (list_length(tlist) < list_length(root->processed_tlist))
+			tlist = root->processed_tlist;

In grouping_planner():

if (planned_rel == NULL)
{
...
root->processed_tlist = tlist;
}
else
tlist = root->processed_tlist;
...
if (current_rel == NULL)
current_rel = query_planner(root, tlist,
standard_qp_callback, &qp_extra);
...
/*
* Query planning may have added some columns to the top-level tlist,
* which happens when there are row marks applied to inheritance
* parent relations (additional junk columns needed for applying row
* marks are added after expanding inheritance.)
*/
if (list_length(tlist) < list_length(root->processed_tlist))
tlist = root->processed_tlist;

Are there any case tlist points to an address different from
root->processed_tlist after calling query_planner? Junk columns are possibly
added to root->processed_tlist after expanding inheritance, but that adding
process don't change the root->processed_tlist's pointer address.
I checked "Assert(tlist == root->processed_tlist)" after calling query_planner
passes "make check".

You're right. I think that may not have been true in some version of the
patch that I didn't share on the list, but it is with the latest patch.
I've removed that block of code and adjusted nearby comments to mention
that targetlist may change during query_planner.

9.
0003: line 1722-1763
In build_append_child_rel():

+	/*
+	 * In addition to the quals inherited from the parent, we might
+	 * have securityQuals associated with this particular child node.
+	 * (Currently this can only happen in appendrels originating from
+	 * UNION ALL; inheritance child tables don't have their own
+	 * securityQuals.)	Pull any such securityQuals up into the
...
+		foreach(lc, childRTE->securityQuals)
+		{
...
+		}
+		Assert(security_level <= root->qual_security_level);
+	}

This foreach loop loops only once in the current regression tests. I checked
"Assert(childRTE->securityQuals->length == 1)" passes "make check".
I think there are no need to change codes, I state this fact only for sharing.

Thanks for the information. There aren't any changes to the code itself
due to this patch, just moved from one place to another.

Attached updated patches. I have a few other changes in mind to make to
0001 such that the range table in each child's version of Query contains
only that child table in place of the original target relation, instead of
*all* child tables which is the current behavior. The current behavior
makes range_table_mutator a big bottleneck when the number of un-pruned
target children is large. But I'm saving it for the next week so that I
can prepare for the PGConf.ASIA that's starting on Monday next week. See
you there. :)

Thanks,
Amit

Hi, Amit

Thanks for the quick modification.

On Wed, Dec 5, 2018 at 8:26 PM, Amit Langote wrote:

1.

...

5.
0003: line 1620-1621

+ * After creating the RelOptInfo for the given child RT index, it goes on to
+ * initialize some of its fields base on the parent RelOptInfo.

s/fields base on/fields based on/

Fixed all of 1-5.

Thanks for fixing.

6.
parsenodes.h
906 * inh is true for relation references that should be expanded to include
907 * inheritance children, if the rel has any. This *must* be false for
908 * RTEs other than RTE_RELATION entries.

I think inh can become true now even if RTEKind equals RTE_SUBQUERY, so latter
sentence need to be modified.

Seems like an existing comment bug. Why don't you send a patch as you
discovered it? :)

Thanks, I am pleased with your proposal. I'll post it as a small fix of the comment.

7.
0005: line 109-115

...

Un-pruned partitions may become dummy due to contradictory constraints or
constraint exclusion using normal CHECK constraints later and whether it's
dummy is checked properly by functions that iterate over live_parts.

Ah, I understand partitions are eliminated contradictory constraints or
constraint exclusion, both using constraints.

Attached updated patches. I have a few other changes in mind to make to
0001 such that the range table in each child's version of Query contains
only that child table in place of the original target relation, instead of
*all* child tables which is the current behavior. The current behavior
makes range_table_mutator a big bottleneck when the number of un-pruned
target children is large. But I'm saving it for the next week so that I

OK. I will continue the review of 0001 before/after your supplying of next
patch with keeping those in mind.

can prepare for the PGConf.ASIA that's starting on Monday next week. See
you there. :)

Yeah, see you there. :)

--
Yoshikazu Imai

Hi, Amit,

On Fri, Dec 7, 2018 at 0:57 AM, Imai, Yoshikazu wrote:

OK. I will continue the review of 0001 before/after your supplying of
next patch with keeping those in mind.

Here's the continuation of the review. Almost all of below comments are
little fixes.

---
0001: line 76-77
In commit message:
exclusion for target child relation, which is no longer
is no longer needed. Constraint exclusion runs during query_planner

s/which is no longer is no longer needed/which is no longer needed/

---
0001: line 464
+	if (IS_DUMMY_REL(find_base_rel(root, resultRelation )))

s/resultRelation )))/resultRelation)))/
(There is an extra space.)

---
0001: line 395-398
+	 * Reset inh_target_child_roots to not be same as parent root's so that
+	 * the subroots for this child's own children (if any) don't end up in
+	 * root parent's list.  We'll eventually merge all entries into one list,
+	 * but that's now now.

s/that's now now/that's not now/

---
0001: line 794
+	 * are put into  a list that will be controlled by a single ModifyTable

s/are put into a list/are put into a list/
(There are two spaces between "into" and "a".)

---
0001: line 241-242, 253-254, 291-294 (In set_append_rel_size())

+		if (appinfo->parent_relid == root->parse->resultRelation)
+			subroot = adjust_inherit_target_child(root, childrel, appinfo);

+			add_child_rel_equivalences(subroot, appinfo, rel, childrel,
+									   root != subroot);

+		if (subroot != root)
+		{
+			root->inh_target_child_roots =
+							lappend(root->inh_target_child_roots, subroot);

A boolean value of "appinfo->parent_relid == root->parse->resultRelation" is
same with "subroot != root"(because of line 241-242), so we can use bool
variable here like
child_is_target = (appinfo->parent_relid == root->parse->resultRelation).
The name of child_is_target is brought from arguments of
add_child_rel_equivalences() in your patch.

I attached a little diff as "v9-0001-delta.diff".

---
0001: line 313-431

adjust_inherit_target_child() is in allpaths.c in your patch, but it has the
code like below ones which are in master's(not patch applied) planner.c or
planmain.c, so could it be possible in planner.c(or planmain.c)?
This point is less important, but I was just wondering whether
adjust_inherit_target_child() should be in allpaths.c, planner.c or planmain.c.

+	/* Translate the original query's expressions to this child. */
+	subroot = makeNode(PlannerInfo);
+	memcpy(subroot, root, sizeof(PlannerInfo));

+	root->parse->targetList = root->unexpanded_tlist;
+	subroot->parse = (Query *) adjust_appendrel_attrs(root,
+													  (Node *) root->parse,
+													  1, &appinfo);

+	tlist = preprocess_targetlist(subroot);
+	subroot->processed_tlist = tlist;
+	build_base_rel_tlists(subroot, tlist);

---
0001: line 57-70

In commit message:
This removes some existing code in inheritance_planner that dealt
with any subquery RTEs in the query. The rationale of that code
was that the subquery RTEs may change during each iteration of
planning (that is, for different children), so different iterations
better use different copies of those RTEs.
...
Since with the new code
we perform planning just once, I think we don't need this special
handling.

0001: line 772-782
- * controlled by a single ModifyTable node. All the instances share the
- * same rangetable, but each instance must have its own set of subquery
- * RTEs within the finished rangetable because (1) they are likely to get
- * scribbled on during planning, and (2) it's not inconceivable that
- * subqueries could get planned differently in different cases. We need
- * not create duplicate copies of other RTE kinds, in particular not the
- * target relations, because they don't have either of those issues. Not
- * having to duplicate the target relations is important because doing so
- * (1) would result in a rangetable of length O(N^2) for N targets, with
- * at least O(N^3) work expended here; and (2) would greatly complicate
- * management of the rowMarks list.

I used considerable time to confirm there are no needs copying subquery RTEs in
the new code, but so far I couldn't. If copied RTEs are only used when planning,
it might not need to copy RTEs in the new code because we perform planning just
once, so I tried to detect when copied RTEs are used or overwritten, but I gave
up.

Of course, there are comments about this,

- * same rangetable, but each instance must have its own set of subquery
- * RTEs within the finished rangetable because (1) they are likely to get
- * scribbled on during planning, and (2) it's not inconceivable that

so copied RTEs might be used when planning, but I couldn't find the actual codes.
I also checked commits[1, 2] related to these codes. I'll check these for more
time but it would be better there are other's review and I also want a help here.

---
Maybe I checked all the way of the v9 patch excluding the codes about
EquivalenceClass codes(0001: line 567-638).
I'll consider whether there are any performance degration case, but I have
no idea for now. Do you have any points you concerns? If there, I'll check it.

[1]: https://github.com/postgres/postgres/commit/b3aaf9081a1a95c245fd605dcf02c91b3a5c3a29
[2]: https://github.com/postgres/postgres/commit/c03ad5602f529787968fa3201b35c119bbc6d782

--
Yoshikazu Imai

Thank you Imai-san.

On 2018/12/25 16:47, Imai, Yoshikazu wrote:

Here's the continuation of the review. Almost all of below comments are
little fixes.

---
0001: line 76-77
In commit message:
exclusion for target child relation, which is no longer
is no longer needed. Constraint exclusion runs during query_planner

s/which is no longer is no longer needed/which is no longer needed/

---
0001: line 464
+	if (IS_DUMMY_REL(find_base_rel(root, resultRelation )))

s/resultRelation )))/resultRelation)))/
(There is an extra space.)

---
0001: line 395-398
+	 * Reset inh_target_child_roots to not be same as parent root's so that
+	 * the subroots for this child's own children (if any) don't end up in
+	 * root parent's list.  We'll eventually merge all entries into one list,
+	 * but that's now now.

s/that's now now/that's not now/

---
0001: line 794
+	 * are put into  a list that will be controlled by a single ModifyTable

s/are put into a list/are put into a list/
(There are two spaces between "into" and "a".)

All fixed in my local repository.

---
0001: line 241-242, 253-254, 291-294 (In set_append_rel_size())

+		if (appinfo->parent_relid == root->parse->resultRelation)
+			subroot = adjust_inherit_target_child(root, childrel, appinfo);

+			add_child_rel_equivalences(subroot, appinfo, rel, childrel,
+									   root != subroot);

+		if (subroot != root)
+		{
+			root->inh_target_child_roots =
+							lappend(root->inh_target_child_roots, subroot);

A boolean value of "appinfo->parent_relid == root->parse->resultRelation" is
same with "subroot != root"(because of line 241-242), so we can use bool
variable here like
child_is_target = (appinfo->parent_relid == root->parse->resultRelation).
The name of child_is_target is brought from arguments of
add_child_rel_equivalences() in your patch.

I attached a little diff as "v9-0001-delta.diff".

Sounds like a good idea for clarifying the code, so done.

---
0001: line 313-431

adjust_inherit_target_child() is in allpaths.c in your patch, but it has the
code like below ones which are in master's(not patch applied) planner.c or
planmain.c, so could it be possible in planner.c(or planmain.c)?
This point is less important, but I was just wondering whether
adjust_inherit_target_child() should be in allpaths.c, planner.c or planmain.c.

+	/* Translate the original query's expressions to this child. */
+	subroot = makeNode(PlannerInfo);
+	memcpy(subroot, root, sizeof(PlannerInfo));

+	root->parse->targetList = root->unexpanded_tlist;
+	subroot->parse = (Query *) adjust_appendrel_attrs(root,
+													  (Node *) root->parse,
+													  1, &appinfo);

+	tlist = preprocess_targetlist(subroot);
+	subroot->processed_tlist = tlist;
+	build_base_rel_tlists(subroot, tlist);

I'm thinking of changing where adjust_inherit_target_child gets called
from. In the current patch, it's in the middle of set_rel_size which I'm
starting to think is a not a good place for it. Maybe, I'll place the
call call near to where inheritance is expanded.

---
0001: line 57-70

In commit message:
This removes some existing code in inheritance_planner that dealt
with any subquery RTEs in the query. The rationale of that code
was that the subquery RTEs may change during each iteration of
planning (that is, for different children), so different iterations
better use different copies of those RTEs.
...
Since with the new code
we perform planning just once, I think we don't need this special
handling.

0001: line 772-782
- * controlled by a single ModifyTable node. All the instances share the
- * same rangetable, but each instance must have its own set of subquery
- * RTEs within the finished rangetable because (1) they are likely to get
- * scribbled on during planning, and (2) it's not inconceivable that
- * subqueries could get planned differently in different cases. We need
- * not create duplicate copies of other RTE kinds, in particular not the
- * target relations, because they don't have either of those issues. Not
- * having to duplicate the target relations is important because doing so
- * (1) would result in a rangetable of length O(N^2) for N targets, with
- * at least O(N^3) work expended here; and (2) would greatly complicate
- * management of the rowMarks list.

I used considerable time to confirm there are no needs copying subquery RTEs in
the new code, but so far I couldn't. If copied RTEs are only used when planning,
it might not need to copy RTEs in the new code because we perform planning just
once, so I tried to detect when copied RTEs are used or overwritten, but I gave
up.

Of course, there are comments about this,

- * same rangetable, but each instance must have its own set of subquery
- * RTEs within the finished rangetable because (1) they are likely to get
- * scribbled on during planning, and (2) it's not inconceivable that

so copied RTEs might be used when planning, but I couldn't find the actual codes.
I also checked commits[1, 2] related to these codes. I'll check these for more
time but it would be better there are other's review and I also want a help here.

[1]

https://github.com/postgres/postgres/commit/b3aaf9081a1a95c245fd605dcf02c91b3a5c3a29

[2]

https://github.com/postgres/postgres/commit/c03ad5602f529787968fa3201b35c119bbc6d782

Thank you very much for spending time on that. I'd really like someone
else to consider this as well.

Here's the summary of what I'm proposing to change with respect to the
above: because we perform scan-level planning only once for any given
relation including for sub-queries with the patch applied, we no longer
need to make copies of sub-query RTEs that are currently needed due to
repeated planning, with one copy per child target relation. Since there
are no new copies, there is no need to process various nodes to change the
RT index being used to refer to the sub-query. I have removed the code
that does to copying of subquery RTEs and the code that does translation
due new RT index being assigned every time a new copy was being made.

---
Maybe I checked all the way of the v9 patch excluding the codes about
EquivalenceClass codes(0001: line 567-638).
I'll consider whether there are any performance degration case, but I have
no idea for now. Do you have any points you concerns? If there, I'll check it.

I will send an updated patch hopefully before my new year vacation that
starts on Friday this week.

Thanks,
Amit

Hi,

On 2018/12/26 13:28, Amit Langote wrote:

On 2018/12/25 16:47, Imai, Yoshikazu wrote:

adjust_inherit_target_child() is in allpaths.c in your patch, but it has the
code like below ones which are in master's(not patch applied) planner.c or
planmain.c, so could it be possible in planner.c(or planmain.c)?
This point is less important, but I was just wondering whether
adjust_inherit_target_child() should be in allpaths.c, planner.c or planmain.c.

+	/* Translate the original query's expressions to this child. */
+	subroot = makeNode(PlannerInfo);
+	memcpy(subroot, root, sizeof(PlannerInfo));

+	root->parse->targetList = root->unexpanded_tlist;
+	subroot->parse = (Query *) adjust_appendrel_attrs(root,
+													  (Node *) root->parse,
+													  1, &appinfo);

+	tlist = preprocess_targetlist(subroot);
+	subroot->processed_tlist = tlist;
+	build_base_rel_tlists(subroot, tlist);

I'm thinking of changing where adjust_inherit_target_child gets called
from. In the current patch, it's in the middle of set_rel_size which I'm
starting to think is a not a good place for it. Maybe, I'll place the
call call near to where inheritance is expanded.

So what I did here is that I added a new step to query_planner() itself
that adds the PlannerInfos for inheritance child target relations and
hence moved the function adjust_inherit_target_child() to planmain.c and
renamed it to add_inherit_target_child_root(). I've removed its
RelOptInfo argument and decided to modify the child RelOptInfo's
targetlist in allpaths.c. So, add_inherit_target_child_root() only builds
the child PlannerInfo now. Child PlannerInfo's are now stored in an array
of simple_rel_array_size elements, instead of a list. It's filled with
NULLs except for the slots corresponding to child target relations.

In allpaths.c, I've added set_inherit_target_rel_sizes and
set_inherit_target_rel_pathlists, which look like set_append_rel_size and
set_append_rel_pathlist, respectively. The reason to add separate
functions for the target relation case is that we don't want to combine
the outputs of children to form an "append relation" in that case. So,
this patch no longer modifies set_append_rel_size for handling the case
where the parent relation is query's target relation.

I will send an updated patch hopefully before my new year vacation that
starts on Friday this week.

Here's the v10 of the patch. I didn't get chance to do more changes than
those described above and address Imai-san's review comments yesterday.

Have a wonderful new year! I'll be back on January 7 to reply on this thread.

Thanks,
Amit

Hi,

I don't think I can't help with code review, but I loaded our largest
customer's schema into pg12dev and tested with this patch. It's working well -
thanks for your work.

Details follow.

We have tooo many tables+indices so this vastly improves planning speed. Our
large customers have ~300 parent tables and total ~20k child tables with total
~80k indices. Our largest tables heirarchies have ~1200 child tables, which
may have as many as 6-8 indices each. And 5-10 of the largest heirarchies are
unioned together in a view.

Running pg11.1, explaining query for the largest view with condition eliminates
all but today's tables can take several minutes with a cold cache, due to not
only stat()ing every file in every table in a partition heirarchy, before
pruning, but also actually open()ing all their indices.

Running 11dev with your v10 patch applied, this takes 2244ms with empty buffer
cache after postmaster restarted on a totally untuned instance (and a new
backend, with no cached opened files).

I was curious why it took even 2sec, and why it did so many opens() (but not
20k of them that PG11 does):

[pryzbyj@database postgresql]$ cut -d'(' -f1 /tmp/strace-12dev-explain |sort |uniq -c |sort -nr
2544 lseek
1263 open
...

It turns out 1050 open()s are due to historic data which is no longer being
loaded and therefor never converted to relkind=p (but hasn't exceeded the
retention interval so not yet DROPped, either).

Cheers,
Justin

On Fri, 4 Jan 2019 at 04:39, Justin Pryzby <pryzby@telsasoft.com> wrote:

Running 11dev with your v10 patch applied, this takes 2244ms with empty buffer
cache after postmaster restarted on a totally untuned instance (and a new
backend, with no cached opened files).

I was curious why it took even 2sec, and why it did so many opens() (but not
20k of them that PG11 does):

It would be pretty hard to know that without seeing the query plan.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Thanks Justin for reporting the results of your testing.

On 2019/01/07 17:40, David Rowley wrote:

On Fri, 4 Jan 2019 at 04:39, Justin Pryzby <pryzby@telsasoft.com> wrote:

Running 11dev with your v10 patch applied, this takes 2244ms with empty buffer
cache after postmaster restarted on a totally untuned instance (and a new
backend, with no cached opened files).

I was curious why it took even 2sec, and why it did so many opens() (but not
20k of them that PG11 does):

It would be pretty hard to know that without seeing the query plan.

Yeah, I too would be curious to see if the resulting plan really needs to
do those open()s. If all the open()s being seen here are accounted for by
un-pruned partitions (across the UNION ALL) contained in the plan and
their indexes, then the patch has done enough to help. If the open()s can
be traced to pruned partitions, then there's something wrong with the patch.

Thanks,
Amit

On Mon, Jan 07, 2019 at 09:40:50PM +1300, David Rowley wrote:

On Fri, 4 Jan 2019 at 04:39, Justin Pryzby <pryzby@telsasoft.com> wrote:

Running 11dev with your v10 patch applied, this takes 2244ms with empty buffer
cache after postmaster restarted on a totally untuned instance (and a new
backend, with no cached opened files).

I was curious why it took even 2sec, and why it did so many opens() (but not
20k of them that PG11 does):

It would be pretty hard to know that without seeing the query plan.

The issue was this:

It turns out 1050 open()s are due to historic data which is no longer being
loaded and therefor never converted to relkind=p (but hasn't exceeded the
retention interval so not yet DROPped, either).

So there's no evidence of any issue with the patch.

Justin

On 2019/01/07 23:13, Justin Pryzby wrote:

The issue was this:

It turns out 1050 open()s are due to historic data which is no longer being
loaded and therefor never converted to relkind=p (but hasn't exceeded the
retention interval so not yet DROPped, either).

So there's no evidence of any issue with the patch.

Ah, so by this you had meant that the historic data is still a old-style
(9.6-style) inheritance hierarchy, which gets folded under the UNION ALL
whose other children are new-style partitioned tables.

Thanks,
Amit

On 2018/12/27 20:25, Amit Langote wrote:

Here's the v10 of the patch. I didn't get chance to do more changes than
those described above and address Imai-san's review comments yesterday.

Have a wonderful new year! I'll be back on January 7 to reply on this thread.

Rebased due to changed copyright year in prepunion.c. Also realized that
the new files added by patch 0004 still had 2018 in them.

Thanks,
Amit

On Tue, 8 Jan 2019 at 19:30, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:

Rebased due to changed copyright year in prepunion.c. Also realized that
the new files added by patch 0004 still had 2018 in them.

I've made a pass over 0001. There's probably enough for you to look at
while I look at 0002 and the others.

0001

1. In your doc changes, just below a paragraph that you removed,
there's a paragraph starting "Both of these behaviors are likely to be
changed in a future release". This needs to be fixed since you've
removed the first of the two reasons.

2. This part should be left alone.

-    technique similar to partition pruning.  While it is primarily used
-    for partitioning implemented using the legacy inheritance method, it can be
-    used for other purposes, including with declarative partitioning.
+    technique similar to partition pruning.  It is primarily used
+    for partitioning implemented using the legacy inheritance method.

Looking at set_inherit_target_rel_sizes(), constraint exclusion still
is applied to partitions, it's just applied after pruning, according
to:

if (did_pruning && !bms_is_member(appinfo->child_relid, live_children))
{
/* This partition was pruned; skip it. */
set_dummy_rel_pathlist(childrel);
continue;
}

if (relation_excluded_by_constraints(root, childrel, childRTE))

3. add_child_rel_equivalences(). You're replacing parent EMs with
their child equivalent, but only when the eclass has no volatile
functions. Is this really what you want? I think this would misbehave
if we ever allowed: UPDATE ... SET .. ORDER BY, of which there's a
legitimate use case of wanting to reduce the chances of deadlocks
caused by non-deterministic UPDATE order. Or if you think skipping
the volatile eclasses is fine today, then I think the comment you've
added to add_child_rel_equivalences should mention that.

4. Do you think it's okay that add_child_rel_equivalences() does not
update the ec_relids when removing the member?

UPDATE: I see you're likely leaving this alone since you're only doing
a shallow copy of the eclasses in
adjust_inherited_target_child_root(). It seems like a pretty bad idea
to do a shallow copy there.

5. What's CE?

+ /* CE failed, so finish copying/modifying join quals. */

6. Typo:

+ * ass dummy. We must do this in this phase so that the rel's

ass -> as

7. There's no accumulation going on here:

+ /*
+ * Accumulate size information from each live child.
+ */
+ Assert(childrel->rows > 0);

8. Any point in this? We're about to loop again anyway...

+ /*
+ * If child is dummy, ignore it.
+ */
+ if (IS_DUMMY_REL(childrel))
+ continue;
+ }

9. It's a bit confusing to mention SELECT in this comment. The Assert
ensures it's an UPDATE or DELETE.

+ /*
+ * For UPDATE/DELETE queries, the top parent can only ever be a table.
+ * As a contrast, it could be a UNION ALL subquery in the case of SELECT.
+ */
+ Assert(root->parse->commandType == CMD_UPDATE ||
+    root->parse->commandType == CMD_DELETE);

10. I'd say the subroot assignment can go after the IS_DUMMY_REL
check. Keeping that loop as tight as possible for pruned rels seems
like a good idea.

+ subroot = root->inh_target_child_roots[appinfo->child_relid];
+ Assert(subroot->parse->resultRelation > 0);
+ childrel = find_base_rel(root, appinfo->child_relid);
+
+ /* Ignore excluded/pruned children. */
+ if (IS_DUMMY_REL(childrel))
+ continue;

11. I don't think you should reuse the childrel variable here:

+ childrel->reloptkind = RELOPT_BASEREL;
+
+ Assert(subroot->join_rel_list == NIL);
+ Assert(subroot->join_rel_hash == NULL);
+
+ /* Perform join planning and save the resulting RelOptInfo. */
+ childrel = make_rel_from_joinlist(subroot, translated_joinlist);
+
+ /*
+ * Remember this child target rel.  inheritance_planner will perform
+ * the remaining steps of planning for each child relation separately.
+ * Specifically, it will call grouping_planner on every
+ * RelOptInfo contained in the inh_target_child_rels list, each of
+ * which represents the source of tuples to be modified for a given
+ * target child rel.
+ */
+ root->inh_target_child_joinrels =
+ lappend(root->inh_target_child_joinrels, childrel);

12. The following comment makes less sense now that you've modified
the previous paragraph:

+ * Fortunately, the UPDATE/DELETE target can never be the nullable side of an
+ * outer join, so it's OK to generate the plan this way.

This text used to refer to:

but target inheritance has to be expanded at
* the top. The reason is that for UPDATE, each target relation needs a
* different targetlist matching its own column set. Fortunately,
* the UPDATE/DELETE target can never be the nullable side of an outer join,
* so it's OK to generate the plan this way.

you no longer describe plan as being expanded from the top rather than
at the bottom, which IMO is what "this way" refers to.

13. "tree is" -> "tree are" (references is plural)

+ * references in the join tree to the original target relation that's the
+ * root parent of the inheritance tree is replaced by each of its

14. Any reason to move this line from its original location?

Assert(parse->commandType != CMD_INSERT);
+ parent_rte = planner_rt_fetch(top_parentRTindex, root);

Previously it was assigned just before it was needed and there's a
fast path out after where you moved it to and where it was.

15. relation_excluded_by_constraints(), the switch
(constraint_exclusion), you could consider turning that into

if (constraint_exclusion == CONSTRAINT_EXCLUSION_OFF)
return false;
/*
* When constraint_exclusion is set to 'partition' we only handle
* OTHER_MEMBER_RELs.
*/
else if (constraint_exclusion == CONSTRAINT_EXCLUSION_PARTITION &&
rel->reloptkind != RELOPT_OTHER_MEMBER_REL)
return false;

When I wrote that code I was trying my best to make the complex rules
as simple as possible by separating them out. The rules have become
quite simple after your change, so it probably does not warrant having
the switch.

16. I think the following comment needs to explain how large this
array is and what indexes it. The current comment would have you
think there are only enough elements to store PlannerInfos for child
rels and leaves you guessing about what they're indexed by.

+ /*
+ * PlannerInfos corresponding to each inheritance child targets.
+ * Content of each PlannerInfo is same as the parent PlannerInfo, except
+ * for the parse tree which is a translated copy of the parent's parse
+ * tree.
+ */
+ struct PlannerInfo **inh_target_child_roots;

17. I'm getting an assert failure in add_paths_to_append_rel() for
Assert(parallel_workers > 0) during the partition_join tests.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hi,

I ran the performance tests for no prepared query and for prepared query with
plan_cache_mode='auto' and plan_cache_mode='force_custom_plan'. I also changed
number of partitions as 256 or 4096. I ran the tests on master and v9-patched.

[settings]
plan_cache_mode = 'auto' or 'force_custom_plan'
max_parallel_workers = 0
max_parallel_workers_per_gather = 0
max_locks_per_transaction = 4096

[partitioning table definitions(with 4096 partitions)]
create table rt (a int, b int, c int) partition by range (a);

\o /dev/null
select 'create table rt' || x::text || ' partition of rt for values from (' ||
(x)::text || ') to (' || (x+1)::text || ');' from generate_series(1, 4096) x;
\gexec
\o

[pgbench(with 4096 partitions)]
no prepared: pgbench -n -f select4096.sql -T 60
prepared: pgbench -n -f select4096.sql -T 60 -M prepared

[select4096.sql]
\set a random(1, 4096)
select a from rt where a = :a;

[results]
master:
part_num no-prepared auto force_custom_plan (1-auto/force_custom_plan)
256 604 571 576 0.01
4096 17.5 17.5 15.1 -0.16

patched:
part_num no-prepared auto force_custom_plan
256 8614 9446 9384 -0.006
4096 7158 7165 7864 0.089

There are almost no difference between auto and force_custom_plan with 256
partitions, but there are difference between auto and force_custom_plan with
4096 partitions. While auto is faster than force_custom_plan on master,
force_custom_plan is faster than auto on patched.

I wonder why force_custom_plan is faster than auto after applied the patch.

When we use PREPARE-EXECUTE, a generic plan is created and used if its cost is
cheaper than creating and using a custom plan with plan_cache_mode='auto',
while a custom plan is always created and used with plan_cache_mode='force_custom_plan'.
So one can think the difference in above results is because of creating or
using a generic plan.

I checked how many times a generic plan is created during executing pgbench and
found a generic plan is created only once and custom plans are created at other
times with plan_cache_mode='auto'. I also checked the time of creating a
generic plan, but it didn't take so much(250ms or so with 4096 partitions). So
the time of creating a generic plan does not affect the performance.

Currently, a generic plan is created at sixth time of executing EXECUTE query.
I changed it to more later (ex. at 400,000th time of executing EXECUTE query on
master with 4096 partitions, because 7000TPS x 60sec=420,0000 transactions are
run while executing pgbench.), then there are almost no difference between auto
and force_custom_plan. I think that creation of a generic plan affects the time
of executing queries which are ordered after creating generic plan.

If my assumption is right, we can expect some additional process is occurred at
executing queries ordered after creating a generic plan, which results in auto is
slower than force_custom_plan because of additional process. But looking at
above results, on master with 4096 partitions, auto is faster than force_custom_plan.
So now I am confused.

Do you have any ideas what does affect the performance?

--
Yoshikazu Imai

From 3b86331dd5a2368adc39c9fef92f3dd09d817a08 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Wed, 7 Nov 2018 16:51:31 +0900
Subject: [PATCH v11 4/6] Move inheritance expansion code into its own file

I wonder if it would make sense to commit 0004 ahead of the rest? To
avoid carrying this huge one over and over ...

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Jan 9, 2019 at 11:41 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

From 3b86331dd5a2368adc39c9fef92f3dd09d817a08 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Wed, 7 Nov 2018 16:51:31 +0900
Subject: [PATCH v11 4/6] Move inheritance expansion code into its own file

I wonder if it would make sense to commit 0004 ahead of the rest? To
avoid carrying this huge one over and over ...

Maybe a good idea. I will rearrange the patches that way tomorrow.

Thanks,
Amit

Hi Amit,

On Mon, Jan 7, 2019 at 6:30 PM, Amit Langote wrote:

On 2018/12/27 20:25, Amit Langote wrote:

Here's the v10 of the patch. I didn't get chance to do more changes
than those described above and address Imai-san's review comments

yesterday.

Have a wonderful new year! I'll be back on January 7 to reply on this

thread.

Rebased due to changed copyright year in prepunion.c. Also realized that
the new files added by patch 0004 still had 2018 in them.

Thank you for new patches.

I also have some comments on 0001, set_inherit_target_rel_sizes().

In set_inherit_target_rel_sizes():

Some codes are placed not the same order as set_append_rel_size().

0001: at line 325-326,
+	ListCell   *l;
+	bool		has_live_children;

In set_append_rel_size(), "has_live_children" is above of the "ListCell *l";

0001: at line 582-603
+		if (IS_DUMMY_REL(childrel))
+			continue;
+
...
+		Assert(childrel->rows > 0);
+
+		/* We have at least one live child. */
+		has_live_children = true;

In set_append_rel_size(), 
+		/* We have at least one live child. */
+		has_live_children = true;
is directly under of
+		if (IS_DUMMY_REL(childrel))
+			continue;

0001: at line 606-622,
+	if (!has_live_children)
+	{
+		/*
+		 * All children were excluded by constraints, so mark the relation
+		 * ass dummy.  We must do this in this phase so that the rel's
+		 * dummy-ness is visible when we generate paths for other rels.
+		 */
+		set_dummy_rel_pathlist(rel);
+	}
+	else
+		/*
+		 * Set a non-zero value here to cope with the caller's requirement
+		 * that non-dummy relations are actually not empty.  We don't try to
+		 * be accurate here, because we're not going to create a path that
+		 * combines the children outputs.
+		 */
+		rel->rows = 1;

In set_append_rel_size(), a condition of if clause is not !has_live_children but
has_live_children.

I also noticed there isn't else block while there is if block.

--
Yoshikazu Imai

On 2019/01/10 0:16, Amit Langote wrote:

On Wed, Jan 9, 2019 at 11:41 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

From 3b86331dd5a2368adc39c9fef92f3dd09d817a08 Mon Sep 17 00:00:00 2001
From: amit <amitlangote09@gmail.com>
Date: Wed, 7 Nov 2018 16:51:31 +0900
Subject: [PATCH v11 4/6] Move inheritance expansion code into its own file

I wonder if it would make sense to commit 0004 ahead of the rest? To
avoid carrying this huge one over and over ...

Maybe a good idea. I will rearrange the patches that way tomorrow.

Here's v12, which is more or less same as v11 but with the order of
patches switched so that the code movement patch is first. Note that the
attached v12-0001 contains no functional changes (but there's tiny note in
the commit message mentioning the addition of a tiny function which is
just old code).

In the v13 that I will try to post tomorrow, I will have hopefully
addressed David's and Imai-san's review comments. Thank you both!

Regards,
Amit

On Thu, 10 Jan 2019 at 21:41, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

In the v13 that I will try to post tomorrow, I will have hopefully
addressed David's and Imai-san's review comments. Thank you both!

I'd been looking at v11's 0002 and started on 0003 too. I'll include
my notes so far if you're about to send a v13.

v11 0002

18. There's a missing case in the following code. I understand that
makeNode() will 0 the member here, but that does not stop the various
other initialisations that set the default value for the field. Below
there's a missing case where parent != NULL && parent->rtekind !=
RTE_RELATION. You might be better just always zeroing the field below
"rel->partitioned_child_rels = NIL;"

+
+ /*
+ * For inheritance child relations, we also set inh_root_parent.
+ * Note that 'parent' might itself be a child (a sub-partitioned
+ * partition), in which case we simply use its value of
+ * inh_root_parent.
+ */
+ if (parent->rtekind == RTE_RELATION)
+ rel->inh_root_parent = parent->inh_root_parent > 0 ?
+ parent->inh_root_parent :
+ parent->relid;
  }
  else
+ {
  rel->top_parent_relids = NULL;
+ rel->inh_root_parent = 0;
+ }

19. Seems strange to have a patch that adds a new field that is
unused. I also don't quite understand yet why top_parent_relids can't
be used instead. I think I recall being confused about that before, so
maybe it's worth writing a comment to mention why it cannot be used.

v11 0003

20. This code looks wrong:

+ /*
+ * expand_inherited_tables may have proved that the relation is empty, so
+ * check if it's so.
+ */
+ else if (rte->inh && !IS_DUMMY_REL(rel))

Likely you'll want:

else if rte->inh)
{
if (IS_DUMMY_REL(rel))
return;
// other stuff
}

otherwise, you'll end up in the else clause when you shouldn't be.

21. is -> was

+ * The child rel's RelOptInfo is created during
+ * expand_inherited_tables().
  */
  childrel = find_base_rel(root, childRTindex);

since you're talking about something that already happened.

I'll continue looking at v12.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2019-Jan-10, Amit Langote wrote:

Here's v12, which is more or less same as v11 but with the order of
patches switched so that the code movement patch is first. Note that the
attached v12-0001 contains no functional changes (but there's tiny note in
the commit message mentioning the addition of a tiny function which is
just old code).

Pushed 0001 with some minor tweaks, thanks.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, 11 Jan 2019 at 06:56, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

Pushed 0001 with some minor tweaks, thanks.

Thanks for pushing. I had looked at 0001 last night and there wasn't
much to report, just:

v12 0001

1. I see you've moved translate_col_privs() out of prepunion.c into
appendinfo.c. This required making it an external function, but it's
only use is in inherit.c, so would it not be better to put it there
and keep it static?

2. The following two lines I think need to swap their order.

+#include "utils/rel.h"
+#include "utils/lsyscache.h"

Both are pretty minor details but thought I'd post them anyway.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2019/01/11 2:56, Alvaro Herrera wrote:

On 2019-Jan-10, Amit Langote wrote:

Here's v12, which is more or less same as v11 but with the order of
patches switched so that the code movement patch is first. Note that the
attached v12-0001 contains no functional changes (but there's tiny note in
the commit message mentioning the addition of a tiny function which is
just old code).

Pushed 0001 with some minor tweaks, thanks.

Thank you for the tweaks and committing.

Regards,
Amit

Sorry, I hadn't read this email before sending my earlier "thank you for
committing" email.

On 2019/01/11 6:47, David Rowley wrote:

On Fri, 11 Jan 2019 at 06:56, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

Pushed 0001 with some minor tweaks, thanks.

Thanks for pushing. I had looked at 0001 last night and there wasn't
much to report, just:

v12 0001

1. I see you've moved translate_col_privs() out of prepunion.c into
appendinfo.c. This required making it an external function, but it's
only use is in inherit.c, so would it not be better to put it there
and keep it static?

Actually, I *was* a bit puzzled where to put it. I tend to agree with you
now that it might be define it locally within inherit.c as it might not be
needed elsewhere. Let's hear what Alvaro thinks. I'm attaching a patch
anyway.

2. The following two lines I think need to swap their order.

+#include "utils/rel.h"
+#include "utils/lsyscache.h"

Oops, fixed.

Both are pretty minor details but thought I'd post them anyway.

Thank you for reporting.

Attached find the patch.

Regards,
Amit

On Thu, 10 Jan 2019 at 21:41, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

Here's v12, which is more or less same as v11 but with the order of
patches switched so that the code movement patch is first. Note that the
attached v12-0001 contains no functional changes (but there's tiny note in
the commit message mentioning the addition of a tiny function which is
just old code).

A few more comments based on reading v12.

v12 0002:

1. Missing braces around the else clause. (Should be consistent with
the "if" above)

+ if (!has_live_children)
+ {
+ /*
+ * All children were excluded by constraints, so mark the relation
+ * ass dummy.  We must do this in this phase so that the rel's
+ * dummy-ness is visible when we generate paths for other rels.
+ */
+ set_dummy_rel_pathlist(rel);
+ }
+ else
+ /*
+ * Set a non-zero value here to cope with the caller's requirement
+ * that non-dummy relations are actually not empty.  We don't try to
+ * be accurate here, because we're not going to create a path that
+ * combines the children outputs.
+ */
+ rel->rows = 1;

v12 0004:

2. I wonder if there's a better way, instead of doing this:

+ if (child_rel1 == NULL)
+ child_rel1 = build_dummy_partition_rel(root, rel1, cnt_parts);
+ if (child_rel2 == NULL)
+ child_rel2 = build_dummy_partition_rel(root, rel2, cnt_parts);

maybe add some logic in populate_joinrel_with_paths() to allow NULL
rels to mean dummy rels. There's a bit of a problem there as the
joinrel has already been created by that time, but perhaps
populate_joinrel_with_paths is a better place to decide if the dummy
rel needs to be built or not.

3. I wonder if there's a better way to handle what
build_dummy_partition_rel() does. I see the child's relid to the
parent's relid and makes up a fake AppendRelInfo and puts it in the
parent's slot. What's going to happen when the parent is not the
top-level parent? It'll already have a AppendRelInfo set.

I had thought something like the following could break this, but of
course, it does not since we build the dummy rel for the pruned
sub_parent2, so we don't hit the NULL relation case when doing the
next level. i.e we only make dummies for the top-level, never dummys
of joinrels.

Does that not mean that the if (parent->top_parent_relids) will always
be false in build_dummy_partition_rel() and it'll only ever get
rtekind == RTE_RELATION?

drop table if exists parent;
create table parent (id int, a int, b text, c float) partition by range (a);
create table sub_parent1 (b text, c float, a int, id int) partition by
range (a);
create table sub_parent2 (c float, b text, id int, a int) partition by
range (a);
alter table parent attach partition sub_parent1 for values from (0) to (10);
alter table parent attach partition sub_parent2 for values from (10) to (20);

create table child11 (id int, b text, c float, a int);
create table child12 (id int, b text, c float, a int);
create table child21 (id int, b text, c float, a int);
create table child22 (id int, b text, c float, a int);

alter table sub_parent1 attach partition child11 for values from (0) to (5);
alter table sub_parent1 attach partition child12 for values from (5) to (10);
alter table sub_parent2 attach partition child21 for values from (10) to (15);
alter table sub_parent2 attach partition child22 for values from (15) to (20);

insert into parent values(0,1,'test',100.0);

select * from parent p1 inner join parent p2 on p1.a=p2.a where p1.id < 10;

4. How are dummy rels handled in grouping_planner()?

I see you have this:

- if (IS_DUMMY_REL(planned_rel))
+ if (!parent_rte->inh || IS_DUMMY_REL(planned_rel))
  {
  grouping_planner(root, false, planned_rel, 0.0);
  return;

With the above example I tried to see how it was handled by doing:

postgres=# update parent set c = c where a = 333;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.

I didn't look into what's causing the crash.

5. Wondering why you removed the if (childOID != parentOID) from:

- if (childOID != parentOID && RELATION_IS_OTHER_TEMP(newrelation))
- {
- heap_close(newrelation, lockmode);
- continue;
- }

Isn't that releasing the only lock we hold on the rel defined in the query?

I tested with:

-- session 1
create temp table a1(a int);
create temp table a2(a int) inherits(a1);

-- session 2
select oid::regclass from pg_class where relname = 'a1';
oid
--------------
pg_temp_3.a1
(1 row)

explain select * from pg_temp_3.a1;
WARNING: you don't own a lock of type AccessShareLock
QUERY PLAN
------------------------------------------
Result (cost=0.00..0.00 rows=0 width=4)
One-Time Filter: false
(2 rows)

6. expand_planner_arrays() zeros a portion of the append_rel_array
even if it just palloc0'd the array. While it's not a bug, it is
repeat work. It should be okay to move the Memset() up to the
repalloc().

7. I see get_relation_info() grew an extra parameter. Would it now be
better just to pass rte instead of doing;

get_relation_info(root, rte->relid, rte->inh, rte->updatedCols,
rel);

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Hi David,

On Thu, Jan 10, 2019 at 4:02 PM, David Rowley wrote:

3. I wonder if there's a better way to handle what
build_dummy_partition_rel() does. I see the child's relid to the
parent's relid and makes up a fake AppendRelInfo and puts it in the
parent's slot. What's going to happen when the parent is not the
top-level parent? It'll already have a AppendRelInfo set.

...

select * from parent p1 inner join parent p2 on p1.a=p2.a where p1.id < 10;

I think there is a mistake in the select SQL.
"p1.id < 10" doesn't prune any partition because tables are partitioned by
column "a" in your definition. Isn't it?

Does that not mean that the if (parent->top_parent_relids) will always
be false in build_dummy_partition_rel() and it'll only ever get
rtekind == RTE_RELATION?

At least, I checked if (parent->top_parent_relids) can be true if I execute
below SQL.

select * from parent p1 inner join parent p2 on p1.a=p2.a where p1.id < 15;

I couldn't check other points you mentioned, but I also think
build_dummy_partition_rel() needs more consideration because I felt it has
complicated logic when I was checking around here.

Amit,
I also realized there are some mistakes in the comments around this function.

+ * build_dummy_partition_rel
+ *		Build a RelOptInfo and AppendRelInfo for a pruned partition 
s/and AppendRelInfo/and an AppendRelInfo/

+	 * Now we'll need a (no-op) AppendRelInfo for parent, because we're
+	 * setting the dummy partition's relid to be same as the parent's.
s/a \(no-op\) AppendRelInfo/an \(no-op\) AppendRelInfo/

--
Yoshikazu Imai

On 2019/01/11 11:07, Amit Langote wrote:

On 2019/01/11 6:47, David Rowley wrote:

On Fri, 11 Jan 2019 at 06:56, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

Pushed 0001 with some minor tweaks, thanks.

Thanks for pushing. I had looked at 0001 last night and there wasn't
much to report, just:

v12 0001

1. I see you've moved translate_col_privs() out of prepunion.c into
appendinfo.c. This required making it an external function, but it's
only use is in inherit.c, so would it not be better to put it there
and keep it static?

Actually, I *was* a bit puzzled where to put it. I tend to agree with you
now that it might be define it locally within inherit.c as it might not be
needed elsewhere. Let's hear what Alvaro thinks. I'm attaching a patch
anyway.

2. The following two lines I think need to swap their order.

+#include "utils/rel.h"
+#include "utils/lsyscache.h"

Oops, fixed.

Both are pretty minor details but thought I'd post them anyway.

Thank you for reporting.

Attached find the patch.

Looking around a bit more, I started thinking even build_child_join_sjinfo
doesn't belong in appendinfo.c (just to be clear, it was defined in
prepunion.c before this commit), so maybe we should move it to joinrels.c
and instead export adjust_child_relids that's required by it from
appendinfo.c. There's already adjust_child_relids_multilevel in
appendinfo.h, so having adjust_child_relids next to it isn't too bad. At
least not as bad as appendinfo.c exporting build_child_join_sjinfo for
joinrels.c to use.

I have updated the patch.

Thanks,
Amit

On Thu, Jan 10, 2019 at 6:10 PM, Imai, Yoshikazu wrote:

Does that not mean that the if (parent->top_parent_relids) will always
be false in build_dummy_partition_rel() and it'll only ever get
rtekind == RTE_RELATION?

At least, I checked if (parent->top_parent_relids) can be true if I
execute below SQL.

select * from parent p1 inner join parent p2 on p1.a=p2.a where p1.id < 15;

Sorry, I also made mistake. I was executed:
select * from parent p1 inner join parent p2 on p1.a=p2.a where p1.a < 15;

--
Yoshikazu Imai

Show quoted text

-----Original Message-----
From: Imai, Yoshikazu [mailto:imai.yoshikazu@jp.fujitsu.com]
Sent: Friday, January 11, 2019 3:10 PM
To: 'David Rowley' <david.rowley@2ndquadrant.com>; Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp>
Cc: Amit Langote <amitlangote09@gmail.com>; Alvaro Herrera
<alvherre@2ndquadrant.com>; Pg Hackers <pgsql-hackers@postgresql.org>
Subject: RE: speeding up planning with partitions

Hi David,

On Thu, Jan 10, 2019 at 4:02 PM, David Rowley wrote:

3. I wonder if there's a better way to handle what
build_dummy_partition_rel() does. I see the child's relid to the
parent's relid and makes up a fake AppendRelInfo and puts it in the
parent's slot. What's going to happen when the parent is not the
top-level parent? It'll already have a AppendRelInfo set.

...

select * from parent p1 inner join parent p2 on p1.a=p2.a where p1.id
< 10;

I think there is a mistake in the select SQL.
"p1.id < 10" doesn't prune any partition because tables are partitioned
by column "a" in your definition. Isn't it?

Does that not mean that the if (parent->top_parent_relids) will always
be false in build_dummy_partition_rel() and it'll only ever get
rtekind == RTE_RELATION?

At least, I checked if (parent->top_parent_relids) can be true if I
execute below SQL.

select * from parent p1 inner join parent p2 on p1.a=p2.a where p1.id
< 15;

I couldn't check other points you mentioned, but I also think
build_dummy_partition_rel() needs more consideration because I felt it
has complicated logic when I was checking around here.

Amit,
I also realized there are some mistakes in the comments around this
function.

+ * build_dummy_partition_rel
+ *		Build a RelOptInfo and AppendRelInfo for a pruned
partition
s/and AppendRelInfo/and an AppendRelInfo/

+	 * Now we'll need a (no-op) AppendRelInfo for parent, because
we're
+	 * setting the dummy partition's relid to be same as the parent's.
s/a \(no-op\) AppendRelInfo/an \(no-op\) AppendRelInfo/

--
Yoshikazu Imai

Thanks for reviewing, David, Imai-san. Replying to all reviews (up to and
including David's comments earlier today) with this one email so that I
can attach the finished patch here.

On 2019/01/09 9:09, David Rowley wrote:

On Tue, 8 Jan 2019 at 19:30, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:

Rebased due to changed copyright year in prepunion.c. Also realized that
the new files added by patch 0004 still had 2018 in them.

I've made a pass over 0001. There's probably enough for you to look at
while I look at 0002 and the others.

0001

1. In your doc changes, just below a paragraph that you removed,
there's a paragraph starting "Both of these behaviors are likely to be
changed in a future release". This needs to be fixed since you've
removed the first of the two reasons.

OK, I've fixed the sentence and moved it to the previous paragraph.

2. This part should be left alone.

-    technique similar to partition pruning.  While it is primarily used
-    for partitioning implemented using the legacy inheritance method, it can be
-    used for other purposes, including with declarative partitioning.
+    technique similar to partition pruning.  It is primarily used
+    for partitioning implemented using the legacy inheritance method.

Looking at set_inherit_target_rel_sizes(), constraint exclusion still
is applied to partitions, it's just applied after pruning, according
to:

OK, I've restored the sentence.

3. add_child_rel_equivalences(). You're replacing parent EMs with
their child equivalent, but only when the eclass has no volatile
functions. Is this really what you want? I think this would misbehave
if we ever allowed: UPDATE ... SET .. ORDER BY, of which there's a
legitimate use case of wanting to reduce the chances of deadlocks
caused by non-deterministic UPDATE order. Or if you think skipping
the volatile eclasses is fine today, then I think the comment you've
added to add_child_rel_equivalences should mention that.

To be honest, I hadn't considered this aspect before.

I think it would be OK to create a new copy of the EC even if it's a
volatile one as we're creating an entirely new one for the child's
planning. Maybe, this will also ensure that someone who will work in the
future on implementing UPDATE SET ORDER BY, they won't have to fiddle with
this code.

4. Do you think it's okay that add_child_rel_equivalences() does not
update the ec_relids when removing the member?

That's an oversight. Fixed by making add_child_rel_equivalences do this:

+            cur_ec->ec_relids = bms_difference(cur_ec->ec_relids,
+                                               parent_rel->relids);
+            cur_ec->ec_relids = bms_add_members(cur_ec->ec_relids,
+                                                child_rel->relids);

UPDATE: I see you're likely leaving this alone since you're only doing
a shallow copy of the eclasses in
adjust_inherited_target_child_root(). It seems like a pretty bad idea
to do a shallow copy there.

So, you're talking about this code:

/*
* Child root should get its own copy of ECs, because they'll be modified
* to replace parent EC expressions by child expressions in
* add_child_rel_equivalences.
*/
subroot->eq_classes = NIL;
foreach(lc, root->eq_classes)
{
EquivalenceClass *ec = lfirst(lc);
EquivalenceClass *new_ec = makeNode(EquivalenceClass);

memcpy(new_ec, ec, sizeof(EquivalenceClass));
new_ec->ec_members = list_copy(ec->ec_members);
subroot->eq_classes = lappend(subroot->eq_classes, new_ec);
}

Can you say what you think is wrong with this way of making a copy of the ECs?

5. What's CE?

+ /* CE failed, so finish copying/modifying join quals. */

Constraint exclusion. It seems I needed to fix comments around here.

6. Typo:

+ * ass dummy. We must do this in this phase so that the rel's

ass -> as

Oops! Fixed.

7. There's no accumulation going on here:

+ /*
+ * Accumulate size information from each live child.
+ */
+ Assert(childrel->rows > 0);

Removed the comment.

8. Any point in this? We're about to loop again anyway...

+ /*
+ * If child is dummy, ignore it.
+ */
+ if (IS_DUMMY_REL(childrel))
+ continue;
+ }

Removed this code.

9. It's a bit confusing to mention SELECT in this comment. The Assert
ensures it's an UPDATE or DELETE.

+ /*
+ * For UPDATE/DELETE queries, the top parent can only ever be a table.
+ * As a contrast, it could be a UNION ALL subquery in the case of SELECT.
+ */
+ Assert(root->parse->commandType == CMD_UPDATE ||
+    root->parse->commandType == CMD_DELETE);

I guess we don't need the 2nd sentence. Removed.

10. I'd say the subroot assignment can go after the IS_DUMMY_REL
check. Keeping that loop as tight as possible for pruned rels seems
like a good idea.

+ subroot = root->inh_target_child_roots[appinfo->child_relid];
+ Assert(subroot->parse->resultRelation > 0);
+ childrel = find_base_rel(root, appinfo->child_relid);
+
+ /* Ignore excluded/pruned children. */
+ if (IS_DUMMY_REL(childrel))
+ continue;

Agreed, done.

11. I don't think you should reuse the childrel variable here:

+ childrel->reloptkind = RELOPT_BASEREL;
+
+ Assert(subroot->join_rel_list == NIL);
+ Assert(subroot->join_rel_hash == NULL);
+
+ /* Perform join planning and save the resulting RelOptInfo. */
+ childrel = make_rel_from_joinlist(subroot, translated_joinlist);
+
+ /*
+ * Remember this child target rel.  inheritance_planner will perform
+ * the remaining steps of planning for each child relation separately.
+ * Specifically, it will call grouping_planner on every
+ * RelOptInfo contained in the inh_target_child_rels list, each of
+ * which represents the source of tuples to be modified for a given
+ * target child rel.
+ */
+ root->inh_target_child_joinrels =
+ lappend(root->inh_target_child_joinrels, childrel);

OK, I've added a new variable called childjoinrel.

12. The following comment makes less sense now that you've modified
the previous paragraph:

+ * Fortunately, the UPDATE/DELETE target can never be the nullable side of an
+ * outer join, so it's OK to generate the plan this way.

This text used to refer to:

but target inheritance has to be expanded at
* the top. The reason is that for UPDATE, each target relation needs a
* different targetlist matching its own column set. Fortunately,
* the UPDATE/DELETE target can never be the nullable side of an outer join,
* so it's OK to generate the plan this way.

you no longer describe plan as being expanded from the top rather than
at the bottom, which IMO is what "this way" refers to.

To be honest, I never quite understood what that line really means, which
is why I kept it around. What I do know though is that, even with the
patched, we still generate subpaths for each child whose targetlist
patches the child, so I think the part about "this way" that prompted
someone to write that line still remains. Does that make sense to you?

13. "tree is" -> "tree are" (references is plural)

+ * references in the join tree to the original target relation that's the
+ * root parent of the inheritance tree is replaced by each of its

Fixed.

14. Any reason to move this line from its original location?

Assert(parse->commandType != CMD_INSERT);
+ parent_rte = planner_rt_fetch(top_parentRTindex, root);

Previously it was assigned just before it was needed and there's a
fast path out after where you moved it to and where it was.

Next patch in the series needs it moved, but no reason for this patch to
move it. Put it back where it was.

15. relation_excluded_by_constraints(), the switch
(constraint_exclusion), you could consider turning that into

if (constraint_exclusion == CONSTRAINT_EXCLUSION_OFF)
return false;
/*
* When constraint_exclusion is set to 'partition' we only handle
* OTHER_MEMBER_RELs.
*/
else if (constraint_exclusion == CONSTRAINT_EXCLUSION_PARTITION &&
rel->reloptkind != RELOPT_OTHER_MEMBER_REL)
return false;

When I wrote that code I was trying my best to make the complex rules
as simple as possible by separating them out. The rules have become
quite simple after your change, so it probably does not warrant having
the switch.

OK, done.

16. I think the following comment needs to explain how large this
array is and what indexes it. The current comment would have you
think there are only enough elements to store PlannerInfos for child
rels and leaves you guessing about what they're indexed by.

+ /*
+ * PlannerInfos corresponding to each inheritance child targets.
+ * Content of each PlannerInfo is same as the parent PlannerInfo, except
+ * for the parse tree which is a translated copy of the parent's parse
+ * tree.
+ */
+ struct PlannerInfo **inh_target_child_roots;

I've added those details to the comment.

17. I'm getting an assert failure in add_paths_to_append_rel() for
Assert(parallel_workers > 0) during the partition_join tests.

I guess that was due to not using the correct root in
inherit_target_rel_pathlists. Fixed.

On 2019/01/10 18:12, David Rowley wrote:>

I'd been looking at v11's 0002 and started on 0003 too. I'll include
my notes so far if you're about to send a v13.

v11 0002

18. There's a missing case in the following code. I understand that
makeNode() will 0 the member here, but that does not stop the various
other initialisations that set the default value for the field. Below
there's a missing case where parent != NULL && parent->rtekind !=
RTE_RELATION. You might be better just always zeroing the field below
"rel->partitioned_child_rels = NIL;"

+
+ /*
+ * For inheritance child relations, we also set inh_root_parent.
+ * Note that 'parent' might itself be a child (a sub-partitioned
+ * partition), in which case we simply use its value of
+ * inh_root_parent.
+ */
+ if (parent->rtekind == RTE_RELATION)
+ rel->inh_root_parent = parent->inh_root_parent > 0 ?
+ parent->inh_root_parent :
+ parent->relid;
}
else
+ {
rel->top_parent_relids = NULL;
+ rel->inh_root_parent = 0;
+ }

Okay, done. Actually, also did the same for top_parent_relids.

19. Seems strange to have a patch that adds a new field that is
unused. I also don't quite understand yet why top_parent_relids can't
be used instead. I think I recall being confused about that before, so
maybe it's worth writing a comment to mention why it cannot be used.

See if this updated comment makes it any clearer:

/*
* For inheritance children, this is the RT index of inheritance table
* mentioned in the query from which this relation originated.
* top_parent_relids cannot be used for this, because if the inheritance
* root table is itself under UNION ALL, top_parent_relids contains the
* RT index of UNION ALL parent subquery.
*/

This is its own patch, because it was thought it could be useful to
another thread which has since stalled:

/messages/by-id/be766794-eb16-b798-52ec-1f786b26b61b@lab.ntt.co.jp

v11 0003

20. This code looks wrong:

+ /*
+ * expand_inherited_tables may have proved that the relation is empty, so
+ * check if it's so.
+ */
+ else if (rte->inh && !IS_DUMMY_REL(rel))

Likely you'll want:

else if rte->inh)
{
if (IS_DUMMY_REL(rel))
return;
// other stuff
}

otherwise, you'll end up in the else clause when you shouldn't be.

OK, done that way.

21. is -> was

+ * The child rel's RelOptInfo is created during
+ * expand_inherited_tables().
*/
childrel = find_base_rel(root, childRTindex);

since you're talking about something that already happened.

OK, fixed.

On 2019/01/11 13:01, David Rowley wrote:>

A few more comments based on reading v12.

v12 0002:

1. Missing braces around the else clause. (Should be consistent with
the "if" above)

+ if (!has_live_children)
+ {
+ /*
+ * All children were excluded by constraints, so mark the relation
+ * ass dummy.  We must do this in this phase so that the rel's
+ * dummy-ness is visible when we generate paths for other rels.
+ */
+ set_dummy_rel_pathlist(rel);
+ }
+ else
+ /*
+ * Set a non-zero value here to cope with the caller's requirement
+ * that non-dummy relations are actually not empty.  We don't try to
+ * be accurate here, because we're not going to create a path that
+ * combines the children outputs.
+ */
+ rel->rows = 1;

Agreed, done.

v12 0004:

2. I wonder if there's a better way, instead of doing this:

+ if (child_rel1 == NULL)
+ child_rel1 = build_dummy_partition_rel(root, rel1, cnt_parts);
+ if (child_rel2 == NULL)
+ child_rel2 = build_dummy_partition_rel(root, rel2, cnt_parts);

maybe add some logic in populate_joinrel_with_paths() to allow NULL
rels to mean dummy rels. There's a bit of a problem there as the
joinrel has already been created by that time, but perhaps
populate_joinrel_with_paths is a better place to decide if the dummy
rel needs to be built or not.

Hmm, I'd thought about that, but concluded that I shouldn't mix that work
with this refactoring project. We can try to hack the join planning code
later, but until then build_dummy_partition_rel can keep things working.
Once we have a working solution that works without having to create this
dummy RelOptInfo, we can remove build_dummy_partition_rel.

3. I wonder if there's a better way to handle what
build_dummy_partition_rel() does. I see the child's relid to the
parent's relid and makes up a fake AppendRelInfo and puts it in the
parent's slot. What's going to happen when the parent is not the
top-level parent? It'll already have a AppendRelInfo set.

Yeah, the parent's AppendRelInfo would already be present and it won't be
replaced:

if (root->append_rel_array[parent->relid] == NULL)
{
AppendRelInfo *appinfo = make_append_rel_info(parent, parentrte,
parent->tupdesc,
parentrte->relid,
parent->reltype,
parent->relid);

root->append_rel_array[parent->relid] = appinfo;
}

Now you'll say why the discrepancy? For the top-level parent's dummy
children, appinfo generated is actually no-op, because it's generated
using the above code. But for an intermediate parent's dummy parent's
there already exists a "real" appinfo. It doesn't make much difference as
far as I can tell, because the appinfo is not used for anything significant.

I had thought something like the following could break this, but of
course, it does not since we build the dummy rel for the pruned
sub_parent2, so we don't hit the NULL relation case when doing the
next level. i.e we only make dummies for the top-level, never dummys
of joinrels.

Does that not mean that the if (parent->top_parent_relids) will always
be false in build_dummy_partition_rel() and it'll only ever get
rtekind == RTE_RELATION?

Well, parentrte->rtekind should always be RTE_RELATION in
build_dummy_partition_rel, because partitionwise join is considered only
for partitioned tables and joinrels resulting from joining partitioned tables.

parent->top_parent_relids might be non-NULL if it's an intermediate parent.

4. How are dummy rels handled in grouping_planner()?

I see you have this:

- if (IS_DUMMY_REL(planned_rel))
+ if (!parent_rte->inh || IS_DUMMY_REL(planned_rel))
{
grouping_planner(root, false, planned_rel, 0.0);
return;

With the above example I tried to see how it was handled by doing:

postgres=# update parent set c = c where a = 333;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.

I didn't look into what's causing the crash.

I tried your example, but it didn't crash for me:

explain update parent set c = c where a = 333;
QUERY PLAN
────────────────────────────────────────────────────
Update on parent (cost=0.00..0.00 rows=0 width=0)
-> Result (cost=0.00..0.00 rows=0 width=54)
One-Time Filter: false
(3 rows)

5. Wondering why you removed the if (childOID != parentOID) from:

- if (childOID != parentOID && RELATION_IS_OTHER_TEMP(newrelation))
- {
- heap_close(newrelation, lockmode);
- continue;
- }

Isn't that releasing the only lock we hold on the rel defined in the query?

I think I mistakenly removed it. Added it back.

6. expand_planner_arrays() zeros a portion of the append_rel_array
even if it just palloc0'd the array. While it's not a bug, it is
repeat work. It should be okay to move the Memset() up to the
repalloc().

Done. Also moved other MemSets to their respective repallocs.

7. I see get_relation_info() grew an extra parameter. Would it now be
better just to pass rte instead of doing;

get_relation_info(root, rte->relid, rte->inh, rte->updatedCols,
rel);

OK, done.

On 2019/01/10 15:37, Imai, Yoshikazu wrote:>

I also have some comments on 0001, set_inherit_target_rel_sizes().

In set_inherit_target_rel_sizes():

Some codes are placed not the same order as set_append_rel_size().

0001: at line 325-326,
+	ListCell   *l;
+	bool		has_live_children;

In set_append_rel_size(), "has_live_children" is above of the "ListCell *l";

OK, moved to look like set_append_rel_size.

0001: at line 582-603
+		if (IS_DUMMY_REL(childrel))
+			continue;
+
...
+		Assert(childrel->rows > 0);
+
+		/* We have at least one live child. */
+		has_live_children = true;

In set_append_rel_size(),
+		/* We have at least one live child. */
+		has_live_children = true;
is directly under of
+		if (IS_DUMMY_REL(childrel))
+			continue;

OK, done.

0001: at line 606-622,
+	if (!has_live_children)
+	{
+		/*
+		 * All children were excluded by constraints, so mark the relation
+		 * ass dummy.  We must do this in this phase so that the rel's
+		 * dummy-ness is visible when we generate paths for other rels.
+		 */
+		set_dummy_rel_pathlist(rel);
+	}
+	else
+		/*
+		 * Set a non-zero value here to cope with the caller's requirement
+		 * that non-dummy relations are actually not empty.  We don't try to
+		 * be accurate here, because we're not going to create a path that
+		 * combines the children outputs.
+		 */
+		rel->rows = 1;

In set_append_rel_size(), a condition of if clause is not
!has_live_children but
has_live_children.

I also noticed there isn't else block while there is if block.

Things that set_inherit_target_rel_sizes and set_append_rel_size need to
do, respectively, are different, so the code looks different. Anyway,
I've switched the above if else blocks' code so that it looks like this:

if (has_live_children)
rel->rows = 1;
else
set_dummy_rel_pathlist(rel);

Here are the updated patches. I've also attached the patch I posted
earlier today here as 0001-Some-fixups-for-b60c397599-v2.patch.

Thanks you again.

Regards,
Amit

On Sat, 12 Jan 2019 at 02:00, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

That's an oversight. Fixed by making add_child_rel_equivalences do this:

+            cur_ec->ec_relids = bms_difference(cur_ec->ec_relids,
+                                               parent_rel->relids);
+            cur_ec->ec_relids = bms_add_members(cur_ec->ec_relids,
+                                                child_rel->relids);

UPDATE: I see you're likely leaving this alone since you're only doing
a shallow copy of the eclasses in
adjust_inherited_target_child_root(). It seems like a pretty bad idea
to do a shallow copy there.

So, you're talking about this code:

/*
* Child root should get its own copy of ECs, because they'll be modified
* to replace parent EC expressions by child expressions in
* add_child_rel_equivalences.
*/
subroot->eq_classes = NIL;
foreach(lc, root->eq_classes)
{
EquivalenceClass *ec = lfirst(lc);
EquivalenceClass *new_ec = makeNode(EquivalenceClass);

memcpy(new_ec, ec, sizeof(EquivalenceClass));
new_ec->ec_members = list_copy(ec->ec_members);
subroot->eq_classes = lappend(subroot->eq_classes, new_ec);
}

Can you say what you think is wrong with this way of making a copy of the ECs?

If you shallow copy with memcpy(new_ec, ec,
sizeof(EquivalenceClass));, then fields such as ec_relids will just
point to the same memory as the parent PlannerInfo's
EquivalenceClasses, so when you do your adjustment (as above) on the
child eclasses, you'll modify memory belonging to the parent. I'd
assume you'd not want to do that since you need to keep the parent
intact at least to make copies for other child rels. You'd have
gotten away with it before since you performed a list_copy() and only
were deleting the parent's EMs with list_delete_cell() which was
modifying the copy, not the original list. Since you were missing the
alteration to ec_relids, then you might not have seen issues with the
shallow copy, but now that you are changing that field, are you not
modifying the ec_relids field that still set in the parent's
PlannerInfo? In this instance, you've sidestepped that issue by using
bms_difference() which creates a copy of the Bitmapset and modifies
that. I think you'd probably see issues if you tried to use
bms_del_members(). I think not doing the deep copy is going to give
other people making changes in this are a hard time in the future.

12. The following comment makes less sense now that you've modified
the previous paragraph:

+ * Fortunately, the UPDATE/DELETE target can never be the nullable side of an
+ * outer join, so it's OK to generate the plan this way.

This text used to refer to:

but target inheritance has to be expanded at
* the top. The reason is that for UPDATE, each target relation needs a
* different targetlist matching its own column set. Fortunately,
* the UPDATE/DELETE target can never be the nullable side of an outer join,
* so it's OK to generate the plan this way.

you no longer describe plan as being expanded from the top rather than
at the bottom, which IMO is what "this way" refers to.

To be honest, I never quite understood what that line really means, which
is why I kept it around. What I do know though is that, even with the
patched, we still generate subpaths for each child whose targetlist
patches the child, so I think the part about "this way" that prompted
someone to write that line still remains. Does that make sense to you?

Not quite. I thought that "OK to generate the plan this way" is
talking about expanding the children at the top of the plan, e.g.

explain (costs off) update listp set b = b + 1 from t where listp.a = t.a;
QUERY PLAN
--------------------------------------
Update on listp
Update on listp1
Update on listp2
-> Merge Join
Merge Cond: (listp1.a = t.a)
-> Sort
Sort Key: listp1.a
-> Seq Scan on listp1
-> Sort
Sort Key: t.a
-> Seq Scan on t
-> Merge Join
Merge Cond: (listp2.a = t.a)
-> Sort
Sort Key: listp2.a
-> Seq Scan on listp2
-> Sort
Sort Key: t.a
-> Seq Scan on t

i.e each non-pruned partition gets joined to the other tables
(expanded from the top of the plan tree). The other tables appear once
for each child target relation.

instead of:

postgres=# explain (costs off) select * from listp inner join t on
listp.a = t.a;
QUERY PLAN
--------------------------------------
Merge Join
Merge Cond: (t.a = listp1.a)
-> Sort
Sort Key: t.a
-> Seq Scan on t
-> Sort
Sort Key: listp1.a
-> Append
-> Seq Scan on listp1
-> Seq Scan on listp2

where the children are expanded from the bottom (or at the leaf side
of the plan tree), since we always plant our trees up-side-down, in
computer science.

In my view, since you removed the part about where the children are
expanded then it does not quite make sense anymore.

Of course, I might not be reading the comment as it was intended.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sat, 12 Jan 2019 at 21:49, David Rowley <david.rowley@2ndquadrant.com> wrote:

Can you say what you think is wrong with this way of making a copy of the ECs?

If you shallow copy with memcpy(new_ec, ec,
sizeof(EquivalenceClass));, then fields such as ec_relids will just
point to the same memory as the parent PlannerInfo's
EquivalenceClasses, so when you do your adjustment (as above) on the
child eclasses, you'll modify memory belonging to the parent. I'd
assume you'd not want to do that since you need to keep the parent
intact at least to make copies for other child rels. You'd have
gotten away with it before since you performed a list_copy() and only
were deleting the parent's EMs with list_delete_cell() which was
modifying the copy, not the original list. Since you were missing the
alteration to ec_relids, then you might not have seen issues with the
shallow copy, but now that you are changing that field, are you not
modifying the ec_relids field that still set in the parent's
PlannerInfo? In this instance, you've sidestepped that issue by using
bms_difference() which creates a copy of the Bitmapset and modifies
that. I think you'd probably see issues if you tried to use
bms_del_members(). I think not doing the deep copy is going to give
other people making changes in this are a hard time in the future.

Setting to waiting on author pending clarification about shallow vs
deep copying of ECs.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Thanks for the comments.

On 2019/01/12 17:49, David Rowley wrote:

On Sat, 12 Jan 2019 at 02:00, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

That's an oversight. Fixed by making add_child_rel_equivalences do this:

+            cur_ec->ec_relids = bms_difference(cur_ec->ec_relids,
+                                               parent_rel->relids);
+            cur_ec->ec_relids = bms_add_members(cur_ec->ec_relids,
+                                                child_rel->relids);

UPDATE: I see you're likely leaving this alone since you're only doing
a shallow copy of the eclasses in
adjust_inherited_target_child_root(). It seems like a pretty bad idea
to do a shallow copy there.

So, you're talking about this code:

/*
* Child root should get its own copy of ECs, because they'll be modified
* to replace parent EC expressions by child expressions in
* add_child_rel_equivalences.
*/
subroot->eq_classes = NIL;
foreach(lc, root->eq_classes)
{
EquivalenceClass *ec = lfirst(lc);
EquivalenceClass *new_ec = makeNode(EquivalenceClass);

memcpy(new_ec, ec, sizeof(EquivalenceClass));
new_ec->ec_members = list_copy(ec->ec_members);
subroot->eq_classes = lappend(subroot->eq_classes, new_ec);
}

Can you say what you think is wrong with this way of making a copy of the ECs?

If you shallow copy with memcpy(new_ec, ec,
sizeof(EquivalenceClass));, then fields such as ec_relids will just
point to the same memory as the parent PlannerInfo's
EquivalenceClasses, so when you do your adjustment (as above) on the
child eclasses, you'll modify memory belonging to the parent. I'd
assume you'd not want to do that since you need to keep the parent
intact at least to make copies for other child rels. You'd have
gotten away with it before since you performed a list_copy() and only
were deleting the parent's EMs with list_delete_cell() which was
modifying the copy, not the original list. Since you were missing the
alteration to ec_relids, then you might not have seen issues with the
shallow copy, but now that you are changing that field, are you not
modifying the ec_relids field that still set in the parent's
PlannerInfo?

Yep. add_eq_member, when adding the child member to the child's copy of
EC, will end up modifying ec_relids that it shares with the parent's copy,
because it uses bms_add_member which modifies the input bitmapset in-place.

In this instance, you've sidestepped that issue by using
bms_difference() which creates a copy of the Bitmapset and modifies
that. I think you'd probably see issues if you tried to use
bms_del_members(). I think not doing the deep copy is going to give
other people making changes in this are a hard time in the future.

I thought of inventing a _copyEquivalenceClass but noticed this in
_copyPathKey:

/* EquivalenceClasses are never moved, so just shallow-copy the pointer */
COPY_SCALAR_FIELD(pk_eclass);

I think that won't be a problem in our case, because this comment seems to
be talking about identical copies of a given EC. In our case, we're
talking about making copies that are essentially different because of
parent-to-child translation. However, providing a function in copyfuncs.c
to copy ECs would make it sound like making identical copies of ECs is OK
(which it apparently isn't), so I added the copying code in the place
where the patch wants to use it. The new code takes care to make copies
of all the fields that might change, not just ec_members.

Also, realizing another problem with where the copying code is placed now
in the patch (adjust_inherited_target_child_root), I have moved it to
adjust_inherit_target_rel_sizes so that translation from parent-to-child
of EC members works correctly. Concretely, the problem is that a level 2
partition's EC members won't be created with the patch's current way of
initializing eq_classes in the partition subroots -- all initially get the
copy of root parent's EC members, whereas translation can only occur
between a partition and its immediate parent. Without EC members, a level
2 leaf partition will not be able to merge join. So, for example:

explain update p set a = foo.a+1 from foo where p.a = foo.a;
QUERY PLAN
──────────────────────────────────────────────────────────────────────────
Update on p (cost=0.00..98556.15 rows=6535012 width=16)
Update on p11
Update on p2
-> Nested Loop (cost=0.00..97614.88 rows=6502500 width=16)
-> Seq Scan on p11 (cost=0.00..35.50 rows=2550 width=10)
-> Materialize (cost=0.00..48.25 rows=2550 width=10)
-> Seq Scan on foo (cost=0.00..35.50 rows=2550 width=10)
-> Merge Join (cost=359.57..941.28 rows=32512 width=16)
Merge Cond: (p2.a = foo.a)
-> Sort (cost=179.78..186.16 rows=2550 width=10)
Sort Key: p2.a
-> Seq Scan on p2 (cost=0.00..35.50 rows=2550 width=10)
-> Sort (cost=179.78..186.16 rows=2550 width=10)
Sort Key: foo.a
-> Seq Scan on foo (cost=0.00..35.50 rows=2550 width=10)

vs.

explain update p set a = foo.a+1 from foo where p.a = foo.a;
QUERY PLAN
──────────────────────────────────────────────────────────────────────────
Update on p (cost=359.57..1882.55 rows=65024 width=16)
Update on p11
Update on p2
-> Merge Join (cost=359.57..941.28 rows=32512 width=16)
Merge Cond: (p11.a = foo.a)
-> Sort (cost=179.78..186.16 rows=2550 width=10)
Sort Key: p11.a
-> Seq Scan on p11 (cost=0.00..35.50 rows=2550 width=10)
-> Sort (cost=179.78..186.16 rows=2550 width=10)
Sort Key: foo.a
-> Seq Scan on foo (cost=0.00..35.50 rows=2550 width=10)
-> Merge Join (cost=359.57..941.28 rows=32512 width=16)
Merge Cond: (p2.a = foo.a)
-> Sort (cost=179.78..186.16 rows=2550 width=10)
Sort Key: p2.a
-> Seq Scan on p2 (cost=0.00..35.50 rows=2550 width=10)
-> Sort (cost=179.78..186.16 rows=2550 width=10)
Sort Key: foo.a
-> Seq Scan on foo (cost=0.00..35.50 rows=2550 width=10)
(19 rows)

after fixing. Note that p11 is a partition of p1 which is partition of p.
p2 is partition of p.

12. The following comment makes less sense now that you've modified
the previous paragraph:

+ * Fortunately, the UPDATE/DELETE target can never be the nullable side of an
+ * outer join, so it's OK to generate the plan this way.

This text used to refer to:

but target inheritance has to be expanded at
* the top. The reason is that for UPDATE, each target relation needs a
* different targetlist matching its own column set. Fortunately,
* the UPDATE/DELETE target can never be the nullable side of an outer join,
* so it's OK to generate the plan this way.

you no longer describe plan as being expanded from the top rather than
at the bottom, which IMO is what "this way" refers to.

To be honest, I never quite understood what that line really means, which
is why I kept it around. What I do know though is that, even with the
patched, we still generate subpaths for each child whose targetlist
patches the child, so I think the part about "this way" that prompted
someone to write that line still remains. Does that make sense to you?

Not quite. I thought that "OK to generate the plan this way" is
talking about expanding the children at the top of the plan, e.g.

explain (costs off) update listp set b = b + 1 from t where listp.a = t.a;
QUERY PLAN
--------------------------------------
Update on listp
Update on listp1
Update on listp2
-> Merge Join
Merge Cond: (listp1.a = t.a)
-> Sort
Sort Key: listp1.a
-> Seq Scan on listp1
-> Sort
Sort Key: t.a
-> Seq Scan on t
-> Merge Join
Merge Cond: (listp2.a = t.a)
-> Sort
Sort Key: listp2.a
-> Seq Scan on listp2
-> Sort
Sort Key: t.a
-> Seq Scan on t

i.e each non-pruned partition gets joined to the other tables
(expanded from the top of the plan tree). The other tables appear once
for each child target relation.

instead of:

postgres=# explain (costs off) select * from listp inner join t on
listp.a = t.a;
QUERY PLAN
--------------------------------------
Merge Join
Merge Cond: (t.a = listp1.a)
-> Sort
Sort Key: t.a
-> Seq Scan on t
-> Sort
Sort Key: listp1.a
-> Append
-> Seq Scan on listp1
-> Seq Scan on listp2

where the children are expanded from the bottom (or at the leaf side
of the plan tree), since we always plant our trees up-side-down, in
computer science.

In my view, since you removed the part about where the children are
expanded then it does not quite make sense anymore.

Thanks for explaining. I think my previous reply was confusing -- I
wanted to say that we *do* still expand the children at the top, in the
sense that the resulting plan shape hasn't changed due to the patch. I
suspect that it's the resulting plan shape that "generating plan this way"
refers to, which hasn't changed with the patch. It's true that the patch
optimizes away repeated query_planner calls but only because it makes
other arrangements to produce the same output as before.

Attached updated patches with mainly fixes around EC copying as described
above and other cosmetic fixes like comment updates. I've also moved
around the code a bit, putting the new functions
add_inherit_target_child_roots, etc. in inherit.c instead of planmain.c.

Thanks,
Amit

On 2019-Jan-11, Amit Langote wrote:

Looking around a bit more, I started thinking even build_child_join_sjinfo
doesn't belong in appendinfo.c (just to be clear, it was defined in
prepunion.c before this commit), so maybe we should move it to joinrels.c
and instead export adjust_child_relids that's required by it from
appendinfo.c. There's already adjust_child_relids_multilevel in
appendinfo.h, so having adjust_child_relids next to it isn't too bad. At
least not as bad as appendinfo.c exporting build_child_join_sjinfo for
joinrels.c to use.

OK, pushed, thanks. I may have caused merge conflicts with the rest of
the series, because I reordered some functions -- sorry about that.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2019/01/17 4:32, Alvaro Herrera wrote:

On 2019-Jan-11, Amit Langote wrote:

Looking around a bit more, I started thinking even build_child_join_sjinfo
doesn't belong in appendinfo.c (just to be clear, it was defined in
prepunion.c before this commit), so maybe we should move it to joinrels.c
and instead export adjust_child_relids that's required by it from
appendinfo.c. There's already adjust_child_relids_multilevel in
appendinfo.h, so having adjust_child_relids next to it isn't too bad. At
least not as bad as appendinfo.c exporting build_child_join_sjinfo for
joinrels.c to use.

OK, pushed, thanks.

Thank you.

I may have caused merge conflicts with the rest of
the series, because I reordered some functions -- sorry about that.

No problem.

I have rebased the patches. Other than rebasing, I've squashed the patch
to add inh_root_parent field to RelOptInfo which contained no other code
changes to use that field into the patch that contains changes to start
using it. Updated the commit message of lazy-partition-initialization
patch to be accurate (it contained old details that have changed since I
first wrote that commit message).

Thanks,
Amit

Thank you Imai-san for testing. Sorry it totally slipped my mind to reply
to this email.

On 2019/01/09 11:08, Imai, Yoshikazu wrote:

I wonder why force_custom_plan is faster than auto after applied the patch.

When we use PREPARE-EXECUTE, a generic plan is created and used if its cost is
cheaper than creating and using a custom plan with plan_cache_mode='auto',
while a custom plan is always created and used with plan_cache_mode='force_custom_plan'.
So one can think the difference in above results is because of creating or
using a generic plan.

I checked how many times a generic plan is created during executing pgbench and
found a generic plan is created only once and custom plans are created at other
times with plan_cache_mode='auto'. I also checked the time of creating a
generic plan, but it didn't take so much(250ms or so with 4096 partitions). So
the time of creating a generic plan does not affect the performance.

Currently, a generic plan is created at sixth time of executing EXECUTE query.
I changed it to more later (ex. at 400,000th time of executing EXECUTE query on
master with 4096 partitions, because 7000TPS x 60sec=420,0000 transactions are
run while executing pgbench.), then there are almost no difference between auto
and force_custom_plan. I think that creation of a generic plan affects the time
of executing queries which are ordered after creating generic plan.

If my assumption is right, we can expect some additional process is occurred at
executing queries ordered after creating a generic plan, which results in auto is
slower than force_custom_plan because of additional process. But looking at
above results, on master with 4096 partitions, auto is faster than force_custom_plan.
So now I am confused.

Do you have any ideas what does affect the performance?

Are you saying that, when using auto mode, all executions of the query
starting from 7th are slower than the first 5 executions? That is, the
latency of creating and using a custom plan increases *after* a generic
plan is created and discarded on the 6th execution of the query? If so,
that is inexplicable to me.

Thanks,
Amit

Imai-san,

From: Amit Langote [mailto:Langote_Amit_f8@lab.ntt.co.jp]

On 2019/01/09 11:08, Imai, Yoshikazu wrote:

I wonder why force_custom_plan is faster than auto after applied the patch.

When we use PREPARE-EXECUTE, a generic plan is created and used if its

cost is

cheaper than creating and using a custom plan with plan_cache_mode='auto',
while a custom plan is always created and used with

plan_cache_mode='force_custom_plan'.

So one can think the difference in above results is because of creating

or

using a generic plan.

I checked how many times a generic plan is created during executing pgbench

and

found a generic plan is created only once and custom plans are created

at other

times with plan_cache_mode='auto'. I also checked the time of creating

a

generic plan, but it didn't take so much(250ms or so with 4096 partitions).

So

the time of creating a generic plan does not affect the performance.

Currently, a generic plan is created at sixth time of executing EXECUTE

query.

I changed it to more later (ex. at 400,000th time of executing EXECUTE

query on

master with 4096 partitions, because 7000TPS x 60sec=420,0000

transactions are

run while executing pgbench.), then there are almost no difference between

auto

and force_custom_plan. I think that creation of a generic plan affects

the time

of executing queries which are ordered after creating generic plan.

If my assumption is right, we can expect some additional process is

occurred at

executing queries ordered after creating a generic plan, which results

in auto is

slower than force_custom_plan because of additional process. But looking

at

above results, on master with 4096 partitions, auto is faster than

force_custom_plan.

So now I am confused.

Do you have any ideas what does affect the performance?

Are you saying that, when using auto mode, all executions of the query
starting from 7th are slower than the first 5 executions? That is, the
latency of creating and using a custom plan increases *after* a generic
plan is created and discarded on the 6th execution of the query? If so,
that is inexplicable to me.

Isn't CheckCachedPlan() (and AcquireExecutorLocks() therein) called in every EXECUTE after 6th one due to some unknow issue?
Does choose_custom_plan() always return true after 6th EXECUTE?

Regards
Takayuki Tsunakawa

Tsunakawa-san,

On 2019/01/18 14:12, Tsunakawa, Takayuki wrote:

From: Amit Langote [mailto:Langote_Amit_f8@lab.ntt.co.jp]

Are you saying that, when using auto mode, all executions of the query
starting from 7th are slower than the first 5 executions? That is, the
latency of creating and using a custom plan increases *after* a generic
plan is created and discarded on the 6th execution of the query? If so,
that is inexplicable to me.

Isn't CheckCachedPlan() (and AcquireExecutorLocks() therein) called in every EXECUTE after 6th one due to some unknow issue?

CheckCachedPlan is only called if choose_custom_plan() returns false
resulting in generic plan being created/reused. With plan_cache_mode =
auto, I see it always returns true, because a custom plan containing a
single partition to scan is way cheaper than the generic plan.

Does choose_custom_plan() always return true after 6th EXECUTE?

Yes.

Thanks,
Amit

Hi Amit-san,

On Wed, Jan 17, 2019 at 10:25 AM, Amit Langote wrote:

Thank you Imai-san for testing. Sorry it totally slipped my mind to reply to this email.

Thanks for replying and sorry for my late reply. I've been undertaking on-the-job training last week.

Are you saying that, when using auto mode, all executions of the query
starting from 7th are slower than the first 5 executions? That is, the
latency of creating and using a custom plan increases *after* a generic
plan is created and discarded on the 6th execution of the query? If so,
that is inexplicable to me.

Yes. And it's also inexplicable to me.

I'll check if this fact is really correct by majoring the time of the
first 5 queries before generic plan is created and the other queries
after generic plan is created.

Yoshikazu Imai

Tsunakawa-san

On Thu, Jan 18, 2019 at 5:29 AM, Amit Langote wrote:

On 2019/01/18 14:12, Tsunakawa, Takayuki wrote:

...

Isn't CheckCachedPlan() (and AcquireExecutorLocks() therein) called

in every EXECUTE after 6th one due to some unknow issue?

CheckCachedPlan is only called if choose_custom_plan() returns false
resulting in generic plan being created/reused. With plan_cache_mode
= auto, I see it always returns true, because a custom plan containing
a single partition to scan is way cheaper than the generic plan.

Does choose_custom_plan() always return true after 6th EXECUTE?

Yes.

Yes.
I also checked choose_custom_plan() always returns true excluding 6th EXECUTE
and CheckCachedPlan() is only called at 6th EXECUTE.

Yoshikazu Imai

On Mon, 21 Jan 2019 at 13:45, Imai, Yoshikazu
<imai.yoshikazu@jp.fujitsu.com> wrote:

On Wed, Jan 17, 2019 at 10:25 AM, Amit Langote wrote:

Are you saying that, when using auto mode, all executions of the query
starting from 7th are slower than the first 5 executions? That is, the
latency of creating and using a custom plan increases *after* a generic
plan is created and discarded on the 6th execution of the query? If so,
that is inexplicable to me.

Yes. And it's also inexplicable to me.

I'll check if this fact is really correct by majoring the time of the
first 5 queries before generic plan is created and the other queries
after generic plan is created.

It would be interesting to see the profiles of having the generic plan
being built on the 6th execution vs the 400,000th execution.

I'd thought maybe one difference would be the relcache hash table
having been expanded greatly after the generic plan was created, but I
see even the generic plan is selecting a random partition, so the
cache would have ended up with that many items eventually anyway, and
since we're talking in the odds of 7.8k TPS with 4k partitions, it
would have only have taken about 2-3 seconds out of the 60 second run
to hit most or all of those partitions anyway. So I doubt it could be
that.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Imai-san,

On 2019/01/21 9:45, Imai, Yoshikazu wrote:

I'll check if this fact is really correct by majoring the time of the
first 5 queries before generic plan is created and the other queries
after generic plan is created.

That would really help. If you are able to recreate that behavior
consistently, that might something to try to fix at some point.

Thanks,
Amit

On 2019/01/17 11:17, Amit Langote wrote:

I have rebased the patches. Other than rebasing, I've squashed the patch
to add inh_root_parent field to RelOptInfo which contained no other code
changes to use that field into the patch that contains changes to start
using it. Updated the commit message of lazy-partition-initialization
patch to be accurate (it contained old details that have changed since I
first wrote that commit message).

Rebased again due to 8d8dcead1295.

Thanks,
Amit

Hi,

On 1/21/19 4:01 AM, Amit Langote wrote:

Rebased again due to 8d8dcead1295.

Passes check-world on 8d8dcead1295.

I ran a work load on the series, measuring the time to load the data
plus the run-time to complete the work load.

Load Run
master 3m39s 144s
1778 3m12s 36s
NP [*] 2m20s 11s

[*] Non partitioned case.

Best regards,
Jesper

I measured the latency of queries executed before and after creating generic plan with master + v15-patch.

In this test, table is partitioned into 4k partitions.
I executed 400,0001 queries by pgbench.
I changed the timing of creating generic plan at 1st, 10,001st, 20,001st, 50,001st, ..., 390,001st by changing the source code.
I run the test with setting both plan_cache_mode = auto and plan_cache_mode = force_custom_plan.

The results is below.
The value in before columns is showing the average latency of queries before creating generic plan in microsec.
The value in after columns is showing the average latency of queries after creating generic plan in microsec.

[auto]
time of creating generic plan | before[usec] | after[usec]
1st 531 142
10,001st 144 141
20,001st 141 144
50,001st 133 140
200,001st 131 143
390,001st 130 138

[force_custom_plan]
time of creating generic plan | before[usec] | after[usec]
1st
10,001st 144 129
20,001st 137 131
50,001st 133 134
200,001st 131 133
390,001st 132 131

* A generic plan is actually not created with plan_cache_mode = force_custom_plan.

Looking at the results of force_custom_plan, the latency of first 10,000 transactions is 144 microsec, and the latency of first 50,000 transactions is 133 microsec.
I think that is because in the early transactions, relcache hash table is not hot (as David mentioned?).

Comparing the latencies in after column between auto and force_custom_plan, auto ones are higher about 8% than force_custom_plan ones.
That is, it seems creating generic plan affects the latency of queries executed after creating generic plan.

On Mon, Jan 21, 2019 at 1:32 AM, David Rowley wrote:

It would be interesting to see the profiles of having the generic plan
being built on the 6th execution vs the 400,000th execution.

I'd thought maybe one difference would be the relcache hash table
having been expanded greatly after the generic plan was created

Does it mean that in the executing queries after the generic plan was created, the time of searching entry in the relcache hash table becomes slow and it increases the latency?

but I
see even the generic plan is selecting a random partition, so the
cache would have ended up with that many items eventually anyway, and
since we're talking in the odds of 7.8k TPS with 4k partitions, it
would have only have taken about 2-3 seconds out of the 60 second run
to hit most or all of those partitions anyway.

And does it mean even if we executes a lot of custom plan without creating generic plan, cache would have been ended up to the same size of which is after creating generic plan?

Anyway, I'll check the relcache size.
Since I don't know how to get profile at the just time of building generic plan, I'll use MemoryContextStats(MemoryContext*) function to check the relcache size at before/after building generic plan and at after executing a lot of custom plans.

Yoshikazu Imai

Imai-san,

If the time for EXECUTE differs cleary before and after the creation of the generic plan, why don't you try to count the function calls for each EXECUTE? Although I haven't tried it, but you can probably do it with SystemTap, like this:

probe process("your_postgres_path").function("*").call {
/* accumulate the call count in an associative array */
}

Then, sort the functions by their call counts. You may find some notable difference between the 5th and 7th EXECUTE.

Regards
Takayuki Tsunakawa

Tsunakawa-san

Thanks for giving the information.
I didn't use it yet, but I just used perf to clarify the difference of before and after the creation of the generic plan, and I noticed that usage of hash_seq_search() is increased about 3% in EXECUTE queries after the creation of the generic plan.

What I understand so far is about 10,000 while loops at total (4098+4098+some extra) is needed in hash_seq_search() in EXECUTE query after the creation of the generic plan.
10,000 while loops takes about 10 microsec (of course, we can't estimate correct time), and the difference of the latency between 5th and 7th EXECUTE is about 8 microsec, I currently think this causes the difference.

I don't know this problem relates to Amit-san's patch, but I'll continue to investigate it.

Yoshikazu Imai

On Tue, 22 Jan 2019 at 20:01, Imai, Yoshikazu
<imai.yoshikazu@jp.fujitsu.com> wrote:

I didn't use it yet, but I just used perf to clarify the difference of before and after the creation of the generic plan, and I noticed that usage of hash_seq_search() is increased about 3% in EXECUTE queries after the creation of the generic plan.

What I understand so far is about 10,000 while loops at total (4098+4098+some extra) is needed in hash_seq_search() in EXECUTE query after the creation of the generic plan.
10,000 while loops takes about 10 microsec (of course, we can't estimate correct time), and the difference of the latency between 5th and 7th EXECUTE is about 8 microsec, I currently think this causes the difference.

I don't know this problem relates to Amit-san's patch, but I'll continue to investigate it.

I had another thought... when you're making a custom plan you're only
grabbing locks on partitions that were not pruned (just 1 partition in
your case), but when making the generic plan, locks will be acquired
on all partitions (all 4000 of them). This likely means that when
building the generic plan for the first time that the
LockMethodLocalHash table is expanded to fit all those locks, and
since we never shrink those down again, it'll remain that size for the
rest of your run. I imagine the reason for the slowdown is that
during LockReleaseAll(), a sequential scan is performed over the
entire hash table. I see from looking at the hash_seq_search() code
that the value of max_bucket is pretty critical to how it'll perform.
The while ((curElem = segp[segment_ndx]) == NULL) loop will need to
run fewer times with a lower max_bucket.

I just did a quick and crude test by throwing the following line into
the end of hash_seq_init():

elog(NOTICE, "%s %u", hashp->tabname, hashp->hctl->max_bucket);

With a 5000 partition table I see:

postgres=# set plan_cache_mode = 'auto';
postgres=# prepare q1 (int) as select * from listp where a = $1;
postgres=# explain analyze execute q1(1);
NOTICE: Portal hash 15
NOTICE: LOCALLOCK hash 15
NOTICE: LOCALLOCK hash 15
...
<skip forward to the 6th execution>

postgres=# explain analyze execute q1(1);
NOTICE: LOCALLOCK hash 5002
NOTICE: Portal hash 15
NOTICE: LOCALLOCK hash 5002
NOTICE: LOCALLOCK hash 5002
QUERY PLAN
--------------------------------------------------------------------------------------------------------
Append (cost=0.00..41.94 rows=13 width=4) (actual time=0.005..0.005
rows=0 loops=1)
-> Seq Scan on listp1 (cost=0.00..41.88 rows=13 width=4) (actual
time=0.005..0.005 rows=0 loops=1)
Filter: (a = 1)
Planning Time: 440.822 ms
Execution Time: 0.017 ms
(5 rows)

I've not gone as far to try to recreate the performance drop you've
mentioned but I believe there's a very high chance that this is the
cause, and if so it's not for Amit to do anything with on this patch.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2019/01/22 09:47, David Rowley wrote:

On Tue, 22 Jan 2019 at 20:01, Imai, Yoshikazu
<imai.yoshikazu@jp.fujitsu.com> wrote:

I didn't use it yet, but I just used perf to clarify the difference of before and after the creation of the generic plan, and I noticed that usage of hash_seq_search() is increased about 3% in EXECUTE queries after the creation of the generic plan.

What I understand so far is about 10,000 while loops at total (4098+4098+some extra) is needed in hash_seq_search() in EXECUTE query after the creation of the generic plan.
10,000 while loops takes about 10 microsec (of course, we can't estimate correct time), and the difference of the latency between 5th and 7th EXECUTE is about 8 microsec, I currently think this causes the difference.

I don't know this problem relates to Amit-san's patch, but I'll continue to investigate it.

I had another thought... when you're making a custom plan you're only
grabbing locks on partitions that were not pruned (just 1 partition in
your case), but when making the generic plan, locks will be acquired
on all partitions (all 4000 of them). This likely means that when
building the generic plan for the first time that the
LockMethodLocalHash table is expanded to fit all those locks, and
since we never shrink those down again, it'll remain that size for the
rest of your run. I imagine the reason for the slowdown is that
during LockReleaseAll(), a sequential scan is performed over the > entire hash table. I see from looking at the hash_seq_search() code
that the value of max_bucket is pretty critical to how it'll perform.
The while ((curElem = segp[segment_ndx]) == NULL) loop will need to
run fewer times with a lower max_bucket.

Really thanks to giving the detail explanation.
Yes, 10,000 while loops I mentioned is exactly scanning entire hash
table in hash_seq_search() which is called in LockReleaseAll().

And I also checked expand_table() is called during building the generic
plan which increases max_bucket.

I just did a quick and crude test by throwing the following line into
the end of hash_seq_init():

elog(NOTICE, "%s %u", hashp->tabname, hashp->hctl->max_bucket);

With a 5000 partition table I see:

postgres=# set plan_cache_mode = 'auto';
postgres=# prepare q1 (int) as select * from listp where a = $1;
postgres=# explain analyze execute q1(1);
NOTICE: Portal hash 15
NOTICE: LOCALLOCK hash 15
NOTICE: LOCALLOCK hash 15
...
<skip forward to the 6th execution>

postgres=# explain analyze execute q1(1);
NOTICE: LOCALLOCK hash 5002
NOTICE: Portal hash 15
NOTICE: LOCALLOCK hash 5002
NOTICE: LOCALLOCK hash 5002
QUERY PLAN
--------------------------------------------------------------------------------------------------------
Append (cost=0.00..41.94 rows=13 width=4) (actual time=0.005..0.005
rows=0 loops=1)
-> Seq Scan on listp1 (cost=0.00..41.88 rows=13 width=4) (actual
time=0.005..0.005 rows=0 loops=1)
Filter: (a = 1)
Planning Time: 440.822 ms
Execution Time: 0.017 ms
(5 rows)

I've not gone as far to try to recreate the performance drop you've
mentioned but I believe there's a very high chance that this is the
cause, and if so it's not for Amit to do anything with on this patch.

Since expand_table() is called if we need locks more than max_bucket, I
think the performance would also drop after executing the query like
"select * from listp where a >= 1 and a <= 5000", but I've not tried it
neither.

cause, and if so it's not for Amit to do anything with on this patch.

+1

One concern I have is force_custom_plan is slower than auto in master,
which is opposite result in patched. I am investigating about it, but
its causes would be also in another place from Amit's patch.

Thanks
--
Yoshikazu Imai

On 2019/01/22 18:47, David Rowley wrote:

On Tue, 22 Jan 2019 at 20:01, Imai, Yoshikazu

What I understand so far is about 10,000 while loops at total (4098+4098+some extra) is needed in hash_seq_search() in EXECUTE query after the creation of the generic plan.
10,000 while loops takes about 10 microsec (of course, we can't estimate correct time), and the difference of the latency between 5th and 7th EXECUTE is about 8 microsec, I currently think this causes the difference.

I don't know this problem relates to Amit-san's patch, but I'll continue to investigate it.

I had another thought... when you're making a custom plan you're only
grabbing locks on partitions that were not pruned (just 1 partition in
your case), but when making the generic plan, locks will be acquired
on all partitions (all 4000 of them). This likely means that when
building the generic plan for the first time that the
LockMethodLocalHash table is expanded to fit all those locks, and
since we never shrink those down again, it'll remain that size for the
rest of your run. I imagine the reason for the slowdown is that
during LockReleaseAll(), a sequential scan is performed over the
entire hash table. I see from looking at the hash_seq_search() code
that the value of max_bucket is pretty critical to how it'll perform.
The while ((curElem = segp[segment_ndx]) == NULL) loop will need to
run fewer times with a lower max_bucket.

I too think that that might be behind that slight drop in performance.
So, it's good to know what one of the performance bottlenecks is when
dealing with large number of relations in queries.

Thanks,
Amit

On 2019/01/21 18:01, Amit Langote wrote:

On 2019/01/17 11:17, Amit Langote wrote:

I have rebased the patches. Other than rebasing, I've squashed the patch
to add inh_root_parent field to RelOptInfo which contained no other code
changes to use that field into the patch that contains changes to start
using it. Updated the commit message of lazy-partition-initialization
patch to be accurate (it contained old details that have changed since I
first wrote that commit message).

Rebased again due to 8d8dcead1295.

Rebased due to the heap_open/close() -> table_open/close() change.

Thanks,
Amit

On Wed, Jan 23, 2019 at 1:35 AM, Amit Langote wrote:

Rebased due to the heap_open/close() -> table_open/close() change.

Maybe there are not many things I can point out through reviewing the patch, so I ran the performance test against v17 patches instead of reviewing codes.
There are already a lot of tests about partition pruning case and we confirmed performance improves in those cases. In this time, I tested about accessing all partitions case.

I tested with master, master + 0001, master + 0001 + 0002, ..., master + 0001 + 0002 + 0003 + 0004.
I ran pgbench 3 times in each test case and below results are average of those.

[postgresql.conf]
max_parallel_workers = 0
max_parallel_workers_per_gather = 0

[partition table definitions(8192 partitions case)]
create table rt (a int, b int, c int) partition by range (a)
create table rt_1 partition of rt for values from (1) to (2);
...
create table rt_8192 partition of rt for values from (8191) to (8192);

[pgbench commands]
pgbench -n -f update.sql -T 30 postgres

[update.sql(updating partkey case)]
update rt set a = 1;

[update.sql(updating non-partkey case)]
update rt set b = 1;

[results]
updating partkey case:

part-num master 0001 0002 0003 0004
1 8215.34 7924.99 7931.15 8407.40 8475.65
2 7137.49 7026.45 7128.84 7583.08 7593.73
4 5880.54 5896.47 6014.82 6405.33 6398.71
8 4222.96 4446.40 4518.54 4802.43 4785.82
16 2634.91 2891.51 2946.99 3085.81 3087.91
32 935.12 1125.28 1169.17 1199.44 1202.04
64 352.37 405.27 417.09 425.78 424.53
128 236.26 310.01 307.70 315.29 312.81
256 65.36 86.84 87.67 84.39 89.27
512 18.34 24.84 23.55 23.91 23.91
1024 4.83 6.93 6.51 6.45 6.49

updating non-partkey case:

part-num master 0001 0002 0003 0004
1 8862.58 8421.49 8575.35 9843.71 10065.30
2 7715.05 7575.78 7654.28 8800.84 8720.60
4 6249.95 6321.32 6470.26 7278.14 7280.10
8 4514.82 4730.48 4823.37 5382.93 5341.10
16 2815.21 3123.27 3162.51 3422.36 3393.94
32 968.45 1702.47 1722.38 1809.89 1799.88
64 364.17 420.48 432.87 440.20 435.31
128 119.94 148.77 150.47 152.18 143.35
256 45.09 46.35 46.93 48.30 45.85
512 8.74 10.59 10.23 10.27 10.13
1024 2.28 2.60 2.56 2.57 2.51

Looking at the results, if we only apply 0001 or 0001 + 0002 and if number of partition is few like 1 or 2, performance degrades compare to master(A maximum reduction is about 5%, which is 8863->8421).
In all other cases, performance improves compare to master.

--
Yoshikazu Imai

On Thu, Jan 24, 2019 at 6:10 AM, Imai, Yoshikazu wrote:

updating partkey case:

part-num master 0001 0002 0003 0004
1 8215.34 7924.99 7931.15 8407.40 8475.65
2 7137.49 7026.45 7128.84 7583.08 7593.73
4 5880.54 5896.47 6014.82 6405.33 6398.71
8 4222.96 4446.40 4518.54 4802.43 4785.82
16 2634.91 2891.51 2946.99 3085.81 3087.91
32 935.12 1125.28 1169.17 1199.44 1202.04
64 352.37 405.27 417.09 425.78 424.53
128 236.26 310.01 307.70 315.29 312.81
256 65.36 86.84 87.67 84.39 89.27
512 18.34 24.84 23.55 23.91 23.91
1024 4.83 6.93 6.51 6.45 6.49

I also tested with non-partitioned table case.

updating partkey case:

part-num master 0001 0002 0003 0004
0 10956.7 10370.5 10472.6 10571.0 10581.5
1 8215.34 7924.99 7931.15 8407.40 8475.65
...
1024 4.83 6.93 6.51 6.45 6.49

In my performance results, it seems update performance degrades in non-partitioned case with v17-patch applied.
But it seems this degrades did not happen at v2-patch.

On Thu, Aug 30, 2018 at 1:45 AM, Amit, Langote wrote:

UPDATE:

nparts master 0001 0002 0003
====== ====== ==== ==== ====
0 2856 2893 2862 2816

Does this degradation only occur in my tests? Or if this result is correct, what may causes the degradation?

--
Yoshikazu Imai

On Sat, 12 Jan 2019 at 02:00, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

On 2019/01/09 9:09, David Rowley wrote:

postgres=# update parent set c = c where a = 333;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.

I didn't look into what's causing the crash.

I tried your example, but it didn't crash for me:

explain update parent set c = c where a = 333;
QUERY PLAN
────────────────────────────────────────────────────
Update on parent (cost=0.00..0.00 rows=0 width=0)
-> Result (cost=0.00..0.00 rows=0 width=54)
One-Time Filter: false
(3 rows)

I had a closer look. The crash is due to
set_inherit_target_rel_sizes() forgetting to set has_live_children to
false. This results in the relation not properly being set to a dummy
rel and the code then making a modify table node without any subnodes.
That crashes due to getTargetResultRelInfo() returning NULL due to
rootResultRelInfo and resultRelInfo both being NULL.

The attached fixes it. If you were not seeing the crash then
has_live_children must have been zero/false by chance during your
test.

A simple case of:

create table listp (a int, b int) partition by list(a);
create table listp1 partition of listp for values in(1);
update listp set b = b + 1 where a = 42;

was crashing for me.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Imai-san,

Thanks for testing.

On 2019/01/24 15:09, Imai, Yoshikazu wrote:

[pgbench commands]
pgbench -n -f update.sql -T 30 postgres

[update.sql(updating partkey case)]
update rt set a = 1;

[update.sql(updating non-partkey case)]
update rt set b = 1;

[results]
updating partkey case:

part-num master 0001 0002 0003 0004
1 8215.34 7924.99 7931.15 8407.40 8475.65
2 7137.49 7026.45 7128.84 7583.08 7593.73
4 5880.54 5896.47 6014.82 6405.33 6398.71
8 4222.96 4446.40 4518.54 4802.43 4785.82
16 2634.91 2891.51 2946.99 3085.81 3087.91
32 935.12 1125.28 1169.17 1199.44 1202.04
64 352.37 405.27 417.09 425.78 424.53
128 236.26 310.01 307.70 315.29 312.81
256 65.36 86.84 87.67 84.39 89.27
512 18.34 24.84 23.55 23.91 23.91
1024 4.83 6.93 6.51 6.45 6.49

updating non-partkey case:

part-num master 0001 0002 0003 0004
1 8862.58 8421.49 8575.35 9843.71 10065.30
2 7715.05 7575.78 7654.28 8800.84 8720.60
4 6249.95 6321.32 6470.26 7278.14 7280.10
8 4514.82 4730.48 4823.37 5382.93 5341.10
16 2815.21 3123.27 3162.51 3422.36 3393.94
32 968.45 1702.47 1722.38 1809.89 1799.88
64 364.17 420.48 432.87 440.20 435.31
128 119.94 148.77 150.47 152.18 143.35
256 45.09 46.35 46.93 48.30 45.85
512 8.74 10.59 10.23 10.27 10.13
1024 2.28 2.60 2.56 2.57 2.51

Looking at the results, if we only apply 0001 or 0001 + 0002 and if number of partition is few like 1 or 2, performance degrades compare to master(A maximum reduction is about 5%, which is 8863->8421).
In all other cases, performance improves compare to master.

Just to be clear, these are cases where pruning *doesn't* occur, though
we'll still need to at least figure out why the degradation occurs for
small number of partitions.

Thanks,
Amit

Hi David,

On 2019/01/28 13:18, David Rowley wrote:

On Sat, 12 Jan 2019 at 02:00, Amit Langote wrote:

On 2019/01/09 9:09, David Rowley wrote:

postgres=# update parent set c = c where a = 333;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.

I didn't look into what's causing the crash.

I tried your example, but it didn't crash for me:

explain update parent set c = c where a = 333;
QUERY PLAN
────────────────────────────────────────────────────
Update on parent (cost=0.00..0.00 rows=0 width=0)
-> Result (cost=0.00..0.00 rows=0 width=54)
One-Time Filter: false
(3 rows)

I had a closer look. The crash is due to
set_inherit_target_rel_sizes() forgetting to set has_live_children to
false. This results in the relation not properly being set to a dummy
rel and the code then making a modify table node without any subnodes.
That crashes due to getTargetResultRelInfo() returning NULL due to
rootResultRelInfo and resultRelInfo both being NULL.

Oops, you're right.

The attached fixes it. If you were not seeing the crash then
has_live_children must have been zero/false by chance during your
test.

Thanks for the fix, I'll incorporate it in the next version I'll post by
tomorrow.

Regards,
Amit

On Mon, 28 Jan 2019 at 21:21, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

Thanks for the fix, I'll incorporate it in the next version I'll post by
tomorrow.

I just started reading 0001 again. I made a few notes:

1. Should bms_del_members() instead of bms_difference() if you don't
mind modifying in place, which it sounds like you don't, going by the
comment.

/*
* Now fix up EC's relids set. It's OK to modify EC like this,
* because caller must have made a copy of the original EC.
* For example, see adjust_inherited_target_child_root.
*/
cur_ec->ec_relids = bms_difference(cur_ec->ec_relids,
parent_rel->relids);
cur_ec->ec_relids = bms_add_members(cur_ec->ec_relids,
child_rel->relids);

2. In the comment:

/*
* Now fix up EC's relids set. It's OK to modify EC like this,
* because caller must have made a copy of the original EC.
* For example, see adjust_inherited_target_child_root.
*/

You mention that the caller must have made a copy, but the header
comment mentions nothing about that to warn callers.

3. Would it be better to do the following in make_rel_from_joinlist()?

/*
* Check that we got at least one usable path. In the case of an
* inherited update/delete operation, no path has been created for
* the query's actual target relation yet.
*/
if (!root->inherited_update &&
(!final_rel ||
!final_rel->cheapest_total_path ||
final_rel->cheapest_total_path->param_info != NULL))
elog(ERROR, "failed to construct the join relation");

That way the test is also performed for each partition's join problem
and you don't need that weird special case to disable the check.

4. Do you think it would be nicer if
inheritance_make_rel_from_joinlist returned rel?

inheritance_make_rel_from_joinlist(root, joinlist);

/*
* Return the RelOptInfo of original target relation, although this
* doesn't really contain the final path. inheritance_planner
* from where we got here will generate the final path, but it will
* do so by iterative over child subroots, not through this
* RelOptInfo.
*/
rel = find_base_rel(root, root->parse->resultRelation);

5. "iterative over child subroots" -> "iterating over the child subroots".

6. In set_inherit_target_rel_sizes() the
!bms_is_member(appinfo->child_relid, live_children) check could be
moved much earlier, likely just after the if (appinfo->parent_relid !=
parentRTindex) check.

I understand you're copying set_append_rel_size() here, but I don't
quite understand the reason why that function is doing it that way.
Seems like wasted effort building the quals for pruned partitions.

7. In set_inherit_target_rel_sizes() I still don't really like the way
you're adjusting the EquivalenceClasses. Would it not be better to
invent a function similar to adjust_appendrel_attrs(), or just use
that function?

8. Still a typo in this comment:

+ * ass dummy. We must do this in this phase so that the rel's

9. "parent's" -> "the parent's"

/* Also propagate this child's own children into parent's list. */

10. Too many "of":

* For each child of of the query's result relation, this translates the

11. Badly formed multiline comment.

/* Save the just translated targetlist as unexpanded_tlist in the child's
* subroot, so that this child's own children can use it. Must use copy

12. DEELETE -> DELETE

/*
* The following fields are set during query planning portion of an
* inherited UPDATE/DEELETE operation.
*/

13. Surely this is not true?

* non-NULL. Content of each PlannerInfo is same as the parent
* PlannerInfo, except for the parse tree which is a translated copy of
* the parent's parse tree.

Are you not modifying the EquivalenceClasses?

14. This comment plus the variable name is confusing me:

/*
* RelOptInfos corresponding to each child target rel. For leaf children,
* it's the RelOptInfo representing the output of make_rel_from_joinlist()
* called with the parent rel in the original join tree replaced by a
* given leaf child. For non-leaf children, it's the baserel RelOptInfo
* itself, left as a placeholder.
*/
List *inh_target_child_joinrels;

The variable name mentions joinrels, but the comment target rels. A
join rel can't be the target of an UPDATE/DELETE.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2019/01/29 11:23, David Rowley wrote:

On Mon, 28 Jan 2019 at 21:21, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

Thanks for the fix, I'll incorporate it in the next version I'll post by
tomorrow.

I just started reading 0001 again. I made a few notes:

Thanks.

1. Should bms_del_members() instead of bms_difference() if you don't
mind modifying in place, which it sounds like you don't, going by the
comment.

/*
* Now fix up EC's relids set. It's OK to modify EC like this,
* because caller must have made a copy of the original EC.
* For example, see adjust_inherited_target_child_root.
*/
cur_ec->ec_relids = bms_difference(cur_ec->ec_relids,
parent_rel->relids);
cur_ec->ec_relids = bms_add_members(cur_ec->ec_relids,
child_rel->relids);

Fixed.

2. In the comment:

/*
* Now fix up EC's relids set. It's OK to modify EC like this,
* because caller must have made a copy of the original EC.
* For example, see adjust_inherited_target_child_root.
*/

You mention that the caller must have made a copy, but the header
comment mentions nothing about that to warn callers.

OK, added a sentence.

3. Would it be better to do the following in make_rel_from_joinlist()?

/*
* Check that we got at least one usable path. In the case of an
* inherited update/delete operation, no path has been created for
* the query's actual target relation yet.
*/
if (!root->inherited_update &&
(!final_rel ||
!final_rel->cheapest_total_path ||
final_rel->cheapest_total_path->param_info != NULL))
elog(ERROR, "failed to construct the join relation");

That way the test is also performed for each partition's join problem
and you don't need that weird special case to disable the check.

Moving it inside make_rel_from_joinlist() seems a bit awkward as there are
many return statements, with those appearing earlier in the function being
being for fast-path cases.

How about in the direct callers of make_rel_from_joinlist() viz.
make_one_rel() and inheritance_make_rel_from_joinlist()?

4. Do you think it would be nicer if
inheritance_make_rel_from_joinlist returned rel?

inheritance_make_rel_from_joinlist(root, joinlist);

/*
* Return the RelOptInfo of original target relation, although this
* doesn't really contain the final path. inheritance_planner
* from where we got here will generate the final path, but it will
* do so by iterative over child subroots, not through this
* RelOptInfo.
*/
rel = find_base_rel(root, root->parse->resultRelation);

OK, done. It now returns RelOptInfo root->parse->resultRelation.

5. "iterative over child subroots" -> "iterating over the child subroots".

Sentence no longer exists after above adjustments.

6. In set_inherit_target_rel_sizes() the
!bms_is_member(appinfo->child_relid, live_children) check could be
moved much earlier, likely just after the if (appinfo->parent_relid !=
parentRTindex) check.

I understand you're copying set_append_rel_size() here, but I don't
quite understand the reason why that function is doing it that way.
Seems like wasted effort building the quals for pruned partitions.

Yeah, fixed that in both functions. Actually, as you might be aware, the
next patch obviates the need for this stanza at all, because pruned
partitions aren't added to root->append_rel_list.

7. In set_inherit_target_rel_sizes() I still don't really like the way
you're adjusting the EquivalenceClasses. Would it not be better to
invent a function similar to adjust_appendrel_attrs(), or just use
that function?

OK, I added a function copy_eq_classes_for_child_root() that simply makes
a copy of eq_classes from the source root (deep-copying where applicable)
and returns the list.

8. Still a typo in this comment:

+ * ass dummy. We must do this in this phase so that the rel's

Sorry, fixed.

9. "parent's" -> "the parent's"

/* Also propagate this child's own children into parent's list. */

10. Too many "of":

* For each child of of the query's result relation, this translates the

11. Badly formed multiline comment.

/* Save the just translated targetlist as unexpanded_tlist in the child's
* subroot, so that this child's own children can use it. Must use copy

12. DEELETE -> DELETE

/*
* The following fields are set during query planning portion of an
* inherited UPDATE/DEELETE operation.
*/

13. Surely this is not true?

* non-NULL. Content of each PlannerInfo is same as the parent
* PlannerInfo, except for the parse tree which is a translated copy of
* the parent's parse tree.

Are you not modifying the EquivalenceClasses?

Yep, fixed. Also fixed items 9-12.

14. This comment plus the variable name is confusing me:

/*
* RelOptInfos corresponding to each child target rel. For leaf children,
* it's the RelOptInfo representing the output of make_rel_from_joinlist()
* called with the parent rel in the original join tree replaced by a
* given leaf child. For non-leaf children, it's the baserel RelOptInfo
* itself, left as a placeholder.
*/
List *inh_target_child_joinrels;

The variable name mentions joinrels, but the comment target rels. A
join rel can't be the target of an UPDATE/DELETE.

How about inh_target_child_path_rels? The final ModifyTable's child
"subpaths" have to hang off of some RelOptInfo until we've done the final
grouping_planner() steps. Those RelOptInfos are stored here.

Attached updated patches.

Thanks,
Amit

PS: replies might be slow until Feb 5 (attending FOSDEM for the first time!)

Imai-san,

On 2019/01/28 10:44, Imai, Yoshikazu wrote:

On Thu, Jan 24, 2019 at 6:10 AM, Imai, Yoshikazu wrote:

updating partkey case:

part-num master 0001 0002 0003 0004
1 8215.34 7924.99 7931.15 8407.40 8475.65
2 7137.49 7026.45 7128.84 7583.08 7593.73
4 5880.54 5896.47 6014.82 6405.33 6398.71
8 4222.96 4446.40 4518.54 4802.43 4785.82
16 2634.91 2891.51 2946.99 3085.81 3087.91
32 935.12 1125.28 1169.17 1199.44 1202.04
64 352.37 405.27 417.09 425.78 424.53
128 236.26 310.01 307.70 315.29 312.81
256 65.36 86.84 87.67 84.39 89.27
512 18.34 24.84 23.55 23.91 23.91
1024 4.83 6.93 6.51 6.45 6.49

I also tested with non-partitioned table case.

updating partkey case:

part-num master 0001 0002 0003 0004
0 10956.7 10370.5 10472.6 10571.0 10581.5
1 8215.34 7924.99 7931.15 8407.40 8475.65
...
1024 4.83 6.93 6.51 6.45 6.49

In my performance results, it seems update performance degrades in non-partitioned case with v17-patch applied.
But it seems this degrades did not happen at v2-patch.

On Thu, Aug 30, 2018 at 1:45 AM, Amit, Langote wrote:

UPDATE:

nparts master 0001 0002 0003
====== ====== ==== ==== ====
0 2856 2893 2862 2816

Does this degradation only occur in my tests? Or if this result is correct, what may causes the degradation?

I re-ran tests with v18 using the following setup [1]I changed the compile flags in build scripts to drop -DCATCACHE_FORCE_RELEASE, which would cause many syscache misses in my test runs:

create table ht (a int, b int) partition by hash (b);
create table ht_0 partition of ht for values with (modulus N, remainder 0);
...
$ cat update-noprune.sql
update ht set a = 0;

pgbench -n -T 60 -f update-noprune,sql

TPS:

nparts master 0001 0002 0003 0004
====== ====== ==== ==== ==== ====
0 4408 4335 4423 4379 4314
1 3883 3873 3679 3856 4007
2 3495 3476 3477 3500 3627

I can see some degradation for small number of partitions, but maybe it's
just noise? At least, I don't yet have a systematic explanation for that
happening.

Thanks,
Amit

[1]: I changed the compile flags in build scripts to drop -DCATCACHE_FORCE_RELEASE, which would cause many syscache misses in my test runs
-DCATCACHE_FORCE_RELEASE, which would cause many syscache misses in my
test runs

On Tue, 29 Jan 2019 at 22:32, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

On 2019/01/29 11:23, David Rowley wrote:

7. In set_inherit_target_rel_sizes() I still don't really like the way
you're adjusting the EquivalenceClasses. Would it not be better to
invent a function similar to adjust_appendrel_attrs(), or just use
that function?

OK, I added a function copy_eq_classes_for_child_root() that simply makes
a copy of eq_classes from the source root (deep-copying where applicable)
and returns the list.

hmm, but you've added a case for SpecialJoinInfo, is there a good
reason not just to do the translation by just adding an
EquivalenceClass case to adjust_appendrel_attrs_mutator() then just
get rid of the new "replace" flag in add_child_rel_equivalences()?
That way you'd also remove the churn in the couple of places you've
had to modify the existing calls to add_child_rel_equivalences().

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services