Re: Commit 4dba331cb3 broke ATTACH PARTITION behaviour.

On 2018/03/30 17:31, Kyotaro HORIGUCHI wrote:
> At Thu, 29 Mar 2018 13:04:06 -0300, Alvaro Herrera wrote:
>> Rushabh Lathia wrote:
>>> On Thu, Mar 29, 2018 at 7:46 PM, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
>>> wrote:
>>
>>>> Hmm, offhand I don't quite see why this error fails to be thrown.
>>>
>>> ATTACH PARTITION should throw an error, because partition table "foo"
>>> already have two partition with key values (1, 2,3 4). And table "foo_d"
>>> which we are attaching here has :
>>
>> Oh, I understand how it's supposed to work. I was just saying I don't
>> understand how this bug occurs. Is it because we fail to determine the
>> correct partition constraint for the default partition in time for its
>> verification scan?
>
> The reason is that CommandCounterIncrement added in
> StorePartitionBound reveals the just added default partition to
> get_default_oid_from_partdesc too early. The revealed partition
> has immature constraint and it overrites the right constraint
> generated just above.
>
> ATExecAttachPartition checks for default partition oid twice but
> the second is just needless before the commit and harms after it.

Yes. What happens as of the commit mentioned in $subject is that the
partition constraint that's set as tab->partition_constraint during the
first call to ValidatePartitionConstraints (which is the correct one) is
overwritten by a wrong one during the 2nd call, which wouldn't happen
before the commit. In the wrongly occurring 2nd call, we'd end up setting
tab->partition_constraint to the negation of the clause expression that
would've been set by the first call (in this case). Thus
tab->partition_constraint ends up returning true for all the values it
contains.

I noticed that there were no tests covering this case causing 4dba331cb3
to not notice this failure in the first place. I updated your patch to
add a few tests. Also, I revised the comment changed by your patch a bit.

Thanks,
Amit