Re: [HACKERS] Adding support for Default partition in partitioning

On Thu, Jun 8, 2017 at 2:54 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> On Wed, Jun 7, 2017 at 2:08 AM, Jeevan Ladhe
> <jeevan.ladhe@enterprisedb.com> wrote:
>
>>
>>>
>>> The code in check_default_allows_bound() to check whether the default
>>> partition
>>> has any rows that would fit new partition looks quite similar to the code
>>> in
>>> ATExecAttachPartition() checking whether all rows in the table being
>>> attached
>>> as a partition fit the partition bounds. One thing that
>>> check_default_allows_bound() misses is, if there's already a constraint on
>>> the
>>> default partition refutes the partition constraint on the new partition,
>>> we can
>>> skip the scan of the default partition since it can not have rows that
>>> would
>>> fit the new partition. ATExecAttachPartition() has code to deal with a
>>> similar
>>> case i.e. the table being attached has a constraint which implies the
>>> partition
>>> constraint. There may be more cases which check_default_allows_bound()
>>> does not
>>> handle but ATExecAttachPartition() handles. So, I am wondering whether
>>> it's
>>> better to somehow take out the common code into a function and use it. We
>>> will
>>> have to deal with a difference through. The first one would throw an error
>>> when
>>> finding a row that satisfies partition constraints whereas the second one
>>> would
>>> throw an error when it doesn't find such a row. But this difference can be
>>> handled through a flag or by negating the constraint. This would also take
>>> care
>>> of Amit Langote's complaint about foreign partitions. There's also another
>>> difference that the ATExecAttachPartition() queues the table for scan and
>>> the
>>> actual scan takes place in ATRewriteTable(), but there is not such queue
>>> while
>>> creating a table as a partition. But we should check if we can reuse the
>>> code to
>>> scan the heap for checking a constraint.
>>>
>>> In case of ATTACH PARTITION, probably we should schedule scan of default
>>> partition in the alter table's work queue like what
>>> ATExecAttachPartition() is
>>> doing for the table being attached. That would fit in the way alter table
>>> works.
>>
>
> I tried refactoring existing code so that it can be used for default
> partitioning as well. The code to validate the partition constraints
> against the table being attached in ATExecAttachPartition() is
> extracted out into a set of functions. For default partition we reuse
> those functions to check whether it contains any row that would fit
> the partition being attached. While creating a new partition, the
> function to skip validation is reused but the scan portion is
> duplicated from ATRewriteTable since we are not in ALTER TABLE
> context. The names of the functions, their declaration will require
> some thought.
>
> There's one test failing because for ATTACH partition the error comes
> from ATRewriteTable instead of check_default_allows_bounds(). May be
> we want to use same message in both places or some make ATRewriteTable
> give a different message while validating default partition.
>
> Please review the patch and let me know if the changes look good.

From the discussion on thread [1], that having a NOT NULL constraint
embedded within an expression may cause a scan to be skipped when it
shouldn't be. For default partitions such a case may arise. If an
existing partition accepts NULL and we try to attach a default
partition, it would get a NOT NULL partition constraint but it will be
buried within an expression like !(key = any(array[1, 2, 3]) OR key is
null) where the existing partition/s accept values 1, 2, 3 and null.
We need to check whether the refactored code handles this case
correctly. v19 patch does not have this problem since it doesn't try
to skip the scan based on the constraints of the table being attached.
Please try following cases 1. a default partition accepting nulls
exists and we attach a partition to accept NULL values 2. a NULL
accepting partition exists and we try to attach a table as default
partition. In both the cases default partition should be checked for
rows with NULL partition keys. In both the cases, if the default
partition table has a NOT NULL constraint we should be able to skip
the scan and should scan the table when such a constraint does not
exist.