Re: speeding up planning with partitions - Mailing list pgsql-hackers

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