On 19 December 2017 at 17:36, David Rowley <david.row...@2ndquadrant.com> wrote:
> I'm sorry to say this is another micro review per code I'm stumbling
> over when looking at the run-time partition pruning stuff.
Again, another micro review. I apologise for the slow trickle of
review. Again, these are just things I'm noticing while reading
through while thinking of the run-time pruning patch.
1. The following Assert appears to be testing for the presence of
cosmic rays :-)
/*
* Determine set of partitions using provided keys, which proceeds in a
* manner determined by the partitioning method.
*/
if (keys->n_eqkeys == partkey->partnatts)
{
Assert(keys->n_eqkeys == partkey->partnatts);
Perhaps it's misplaced during a rewrite? Should be safe enough to
remove it, I'd say.
2. The following code in classify_partition_bounding_keys() misses
looking under the RelabelType for rightarg:
leftop = (Expr *) get_leftop(clause);
if (IsA(leftop, RelabelType))
leftop = ((RelabelType *) leftop)->arg;
rightop = (Expr *) get_rightop(clause);
if (EXPR_MATCHES_PARTKEY(leftop, partattno, partexpr))
constexpr = rightop;
else if (EXPR_MATCHES_PARTKEY(rightop, partattno, partexpr))
constexpr = leftop;
This breaks the following case:
create table thisthat (a varchar not null) partition by list (a);
create table this partition of thisthat for values in('this');
create table that partition of thisthat for values in('that');
explain select * from thisthat where 'this' = a; -- does not work
QUERY PLAN
------------------------------------------------------------
Append (cost=0.00..54.00 rows=14 width=32)
-> Seq Scan on that (cost=0.00..27.00 rows=7 width=32)
Filter: ('this'::text = (a)::text)
-> Seq Scan on this (cost=0.00..27.00 rows=7 width=32)
Filter: ('this'::text = (a)::text)
(5 rows)
explain select * from thisthat where a = 'this'; -- works as we look
through the RelabelType on left arg.
QUERY PLAN
------------------------------------------------------------
Append (cost=0.00..27.00 rows=7 width=32)
-> Seq Scan on this (cost=0.00..27.00 rows=7 width=32)
Filter: ((a)::text = 'this'::text)
3. The follow code assumes there will be a commutator for the operator:
if (constexpr == rightop)
pc->op = opclause;
else
{
OpExpr *commuted;
commuted = (OpExpr *) copyObject(opclause);
commuted->opno = get_commutator(opclause->opno);
commuted->opfuncid = get_opcode(commuted->opno);
commuted->args = list_make2(rightop, leftop);
pc->op = commuted;
}
I had to hunt for it, but it appears that you're pre-filtering clauses
with the Consts on the left and no valid commutator in
match_clauses_to_partkey. I think it's likely worth putting a comment
to mention that reversed clauses with no commutator should have been
filtered out beforehand. I'd say it's also worthy of an Assert().
4. The spelling of "arbitrary" is incorrect in:
* partattno == 0 refers to arbirtary expressions, which get the
5. I've noticed that partition pruning varies slightly from constraint
exclusion in the following case:
create table ta (a int not null) partition by list (a);
create table ta1 partition of ta for values in(1,2);
create table ta2 partition of ta for values in(3,4);
explain select * from ta where a <> 1 and a <> 2; -- partition ta1 is
not eliminated.
QUERY PLAN
-------------------------------------------------------------
Append (cost=0.00..96.50 rows=5050 width=4)
-> Seq Scan on ta1 (cost=0.00..48.25 rows=2525 width=4)
Filter: ((a <> 1) AND (a <> 2))
-> Seq Scan on ta2 (cost=0.00..48.25 rows=2525 width=4)
Filter: ((a <> 1) AND (a <> 2))
(5 rows)
alter table ta1 add constraint ta1_chk check (a in(1,2)); -- add a
check constraint to see if can be removed.
explain select * from ta where a <> 1 and a <> 2; -- it can.
QUERY PLAN
-------------------------------------------------------------
Append (cost=0.00..48.25 rows=2525 width=4)
-> Seq Scan on ta2 (cost=0.00..48.25 rows=2525 width=4)
Filter: ((a <> 1) AND (a <> 2))
(3 rows)
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
