On Thu, 10 Jan 2019 at 21:41, Amit Langote
<langote_amit...@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
