Re: Adding support for Default partition in partitioning

Thanks Ashutosh,

On Thu, Jun 8, 2017 at 4:04 PM, Ashutosh Bapat <
ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:

> On Thu, Jun 8, 2017 at 2:54 PM, Ashutosh Bapat
> <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
> > On Wed, Jun 7, 2017 at 2:08 AM, Jeevan Ladhe
> > <jeevan(dot)ladhe(at)enterprisedb(dot)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.
>

I will review your refactoring patch as well test above cases.

Regards,
Jeevan Ladhe