Re: Optimise default partition scanning while adding new partition

On 2017/09/15 0:59, Robert Haas wrote:
> On Thu, Sep 14, 2017 at 4:03 AM, Jeevan Ladhe
> <jeevan.ladhe@enterprisedb.com> wrote:
>> Thanks Amit for reviewing.
>>> Patch looks fine to me.  By the way, why don't we just say "Can we skip
>>> scanning part_rel?" in the comment before the newly added call to
>>> PartConstraintImpliedByRelConstraint()?  We don't need to repeat the
>>> explanation of what it does at the every place we call it.
>>
>> I have changed the comment.
>> PFA.
> 
> I'm probably missing something stupid, but why does this happen?
> 
>  ALTER TABLE list_parted2 ATTACH PARTITION part_7 FOR VALUES IN (7);
> -INFO:  partition constraint for table "list_parted2_def" is implied
> by existing constraints
>  ERROR:  partition constraint is violated by some row
> 
> Based on the regression test changes made up higher in the file, I'd
> expect that line to be replaced by two lines, one for
> list_parted2_def_p1 and another for list_parted2_def_p2, because at
> this point, list_parted2_def is a partitioned table with those two
> children, and they seem to have appropriate constraints.
> 
> list_parted2_def_p1 has
>  Check constraints:
>      "check_a" CHECK (a = ANY (ARRAY[21, 22]))
> 
> list_parted2_def_p2 has
>  Check constraints:
>      "check_a" CHECK (a = ANY (ARRAY[31, 32]))
> 
> Well, if a is 21 or 22 for the first, or 31 or 32 for the second, then
> it's not 7.  Or so I would think.

ISTM, Jeevan's patch modifies check_default_allows_bound(), which only
DefineRelation() calls as things stand today.  IOW, it doesn't handle the
ATTACH PARTITION case, so you're not seeing the lines for
list_parted2_def_p1 and list_parted2_def_p2, because ATExecAttachPartition
doesn't yet know how to skip the validation scan for default partition's
children.

I wonder if we should call check_default_allows_bound() from
ATExecAttachPartition(), too, instead of validating updated default
partition constraint using ValidatePartitionConstraints()?  That is, call
the latter only to validate the partition constraint of the table being
attached and call check_default_allows_bound() to validate the updated
default partition constraint.  That way, INFO/ERROR messages related to
default partition constraint are consistent across the board.  I hacked
that up in the attached 0001.  The patch also modifies the INFO message
that check_default_allows_bound() emits when the scan is skipped to make
it apparent that a default partition is involved.  (Sorry, this should've
been a review comment I should've posted before the default partition
patch was committed.)

Then apply 0002, which is Jeevan's patch.  With 0001 in place, Robert's
complaint is taken care of.

Then comes 0003, which is my patch to teach ValidatePartitionConstraints()
to skip child table scans using their existing constraint.

Thanks,
Amit