Re: default partition and concurrent attach partition

Hi Alvaro,

On Fri, Sep 4, 2020 at 6:28 AM Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>
> Also, I should have pointed out that ExecInsert doesn't actually check
> the partitin constraint except in very specific cases; what it does is
> expect that the partition routing code got it right. So the comment
> you're adding about that is wrong, and it did misled me into changing
> your code in a way that actually caused a bug -- hence my proposed
> rewording.

Thanks for taking a look.

/*
* If this partition is the default one, we must check its partition
- * constraint, because it may have changed due to partitions being
- * added to the parent concurrently. We do the check here instead of
- * in ExecInsert(), because we would not want to miss checking the
- * constraint of any nonleaf partitions as they never make it to
- * ExecInsert().
+ * constraint now, which may have changed due to partitions being
+ * added to the parent concurrently.

I am fine with your rewording but let me explain how I ended up
writing what I wrote:

At first I had pondered tweaking the following code in ExecInsert() to
fix this bug:

/*
* Also check the tuple against the partition constraint, if there is
* one; except that if we got here via tuple-routing, we don't need to
* if there's no BR trigger defined on the partition.
*/
if (resultRelInfo->ri_PartitionCheck &&
(resultRelInfo->ri_PartitionRoot == NULL ||
(resultRelInfo->ri_TrigDesc &&
resultRelInfo->ri_TrigDesc->trig_insert_before_row)))
ExecPartitionCheck(resultRelInfo, slot, estate, true);

I gave up because I concluded that there isn't enough information at
this place in code to determine if the partition is a default
partition, so I moved my attention to ExecFindPartition() where we
have access to the parent's PartitionDesc which is enough to do so.
In the process of modifying ExecFindPartition() to fix the bug I
realized that default partitions can be partitioned and
sub-partitioned partitions never reach ExecInsert(). So, beside the
earlier mentioned inconvenience of fixing this bug in ExecInsert(),
there is also the problem that such a fix would have missed addressing
sub-partitioned default partitions. I thought someone beside me would
also wonder why ExecInsert() is not touched in this fix, hence the
comment.

FWIW, I still prefer "minimally valid ResultRelInfo" to "fake
ResultRelInfo", because those aren't really fake, are they? They are
as valid as any other ResultRelInfo as far I can see. I said
"minimally valid" because a fully-valid partition ResultRelInfo is one
made by ExecInitPartitionInfo(), which I avoided to use here thinking
that would be overkill.

--
Amit Langote
EnterpriseDB: http://www.enterprisedb.com