Re: selecting from partitions and constraint exclusion

Hi David,

Thanks for checking.

On 2019/03/20 19:41, David Rowley wrote:
> On Wed, 20 Mar 2019 at 17:37, Amit Langote
> <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> That's because get_relation_constraints() no longer (as of PG 11) includes
>> the partition constraint for SELECT queries. But that's based on an
>> assumption that partitions are always accessed via parent, so partition
>> pruning would make loading the partition constraint unnecessary. That's
>> not always true, as shown in the above example.
>>
>> Should we fix that? I'm attaching a patch here.
>
> Perhaps we should. The constraint_exclusion documents [1] just mention:
>
>> Controls the query planner's use of table constraints to optimize queries.
>
> and I'm pretty sure you could class the partition constraint as a
> table constraint.

Yes.

> As for the patch:
>
> + if ((root->parse->commandType == CMD_SELECT && !IS_OTHER_REL(rel)) ||
>
> Shouldn't this really be checking rel->reloptkind == RELOPT_BASEREL
> instead of !IS_OTHER_REL(rel) ?

Hmm, thought I'd use the macro if we have one, but I'll change as you
suggest if that's what makes the code easier to follow. As you might
know, we can only get "simple" relations here.

> For the comments:
>
> + * For selects, we only need those if the partition is directly mentioned
> + * in the query, that is not via parent. In case of the latter, partition
> + * pruning, which uses the parent table's partition bound descriptor,
> + * ensures that we only consider partitions whose partition constraint
> + * satisfy the query quals (or, the two don't contradict each other), so
> + * loading them is pointless.
> + *
> + * For updates and deletes, we always need those for performing partition
> + * pruning using constraint exclusion, but, only if pruning is enabled.
>
> You mention "the latter", normally you'd only do that if there was a
> former, but in this case there's not.

I was trying to go for "accessing partition directly" as the former and
"accessing it via the parent" as the latter, but maybe the sentence as
written cannot be read that way.

> How about just making it:
>
> /*
> * Append partition predicates, if any.
> *
> * For selects, partition pruning uses the parent table's partition bound
> * descriptor, so there's no need to include the partition constraint for
> * this case. However, if the partition is referenced directly in the query
> * then no partition pruning will occur, so we'll include it in the case.
> */
> if ((root->parse->commandType != CMD_SELECT && enable_partition_pruning) ||
> (root->parse->commandType == CMD_SELECT && rel->reloptkind ==
> RELOPT_BASEREL))

OK, I will use this text.

> For the tests, it seems excessive to create some new tables for this.
> Won't the tables in the previous test work just fine?

OK, I have revised the tests to use existing tables.

I'll add this to July fest to avoid forgetting about this.

Thanks,
Amit