Re: speeding up planning with partitions

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Amit Langote <amitlangote09(at)gmail(dot)com>
Cc: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>, David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>, Imai Yoshikazu <yoshikazu_i443(at)live(dot)jp>, "jesper(dot)pedersen(at)redhat(dot)com" <jesper(dot)pedersen(at)redhat(dot)com>, "Imai, Yoshikazu" <imai(dot)yoshikazu(at)jp(dot)fujitsu(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Justin Pryzby <pryzby(at)telsasoft(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: speeding up planning with partitions
Date: 2019-04-29 21:12:29
Message-ID: 7323.1556572349@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Amit Langote <amitlangote09(at)gmail(dot)com> writes:
> Here is the patch. I've also included the patch to update the text in
> ddl.sgml regarding constraint exclusion and partition pruning.

I thought this was a bit messy. In particular, IMV the reason to
have a split between get_relation_constraints and its only caller
relation_excluded_by_constraints is to create a policy vs mechanism
separation: relation_excluded_by_constraints figures out what kinds
of constraints we need to look at, while get_relation_constraints does
the gruntwork of digging them out of the catalog data. Somebody had
already ignored this principle to the extent of putting this
very-much-policy test into get_relation_constraints:

if (enable_partition_pruning && root->parse->commandType != CMD_SELECT)

but the way to improve that is to add another flag parameter to convey
the policy choice, not to move the whole chunk of mechanism out to the
caller.

It also struck me while looking at the code that we were being
unnecessarily stupid about non-inheritable constraints: rather than
just throwing up our hands for traditional inheritance situations,
we can still apply constraint exclusion, as long as we consider only
constraints that aren't marked ccnoinherit. (attnotnull constraints
have to be considered as if they were ccnoinherit, for ordinary
tables but not partitioned ones.)

So, I propose the attached revised patch.

I'm not sure how much of this, if anything, we should back-patch to
v11. It definitely doesn't seem like we should back-patch the
improvement just explained. I tried diking out that change, as
in the v11 variant attached, and found that this still causes quite a
few other changes in v11's expected results, most of them not for the
better. So I'm thinking that we'd better conclude that v11's ship
has sailed. Its behavior is in some ways weird, but I am not sure
that anyone will appreciate our changing it on the fourth minor
release.

It's somewhat interesting that we get these other changes in v11
but not HEAD. I think the reason is that we reimplemented so much
of inheritance_planner in 428b260f8; that is, it seems the weird
decisions we find in relation_excluded_by_constraints are mostly
there to band-aid over the old weird behavior of inheritance_planner.

Anyway, my current thought is to apply this to HEAD and do nothing
in v11. I include the v11 patch just for amusement. (I did not
check v11's behavior outside the core regression tests; it might
possibly have additional test diffs in contrib.)

regards, tom lane

Attachment Content-Type Size
constraint-exclusion-partition-constraint-2.patch text/x-diff 15.5 KB
constraint-exclusion-partition-constraint-2-v11.patch text/x-diff 19.3 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message David Rowley 2019-04-29 21:29:44 Re: "long" type is not appropriate for counting tuples
Previous Message Fabien COELHO 2019-04-29 20:03:56 Re: [PATCH v5] Show detailed table persistence in \dt+