Thanks Ashutosh, On Thu, Jun 8, 2017 at 4:04 PM, Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote:
> On Thu, Jun 8, 2017 at 2:54 PM, Ashutosh Bapat > <ashutosh.ba...@enterprisedb.com> wrote: > > On Wed, Jun 7, 2017 at 2:08 AM, Jeevan Ladhe > > <jeevan.la...@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. > I will review your refactoring patch as well test above cases. Regards, Jeevan Ladhe