Re: Declarative partitioning - another take

On 2016/11/11 6:51, Robert Haas wrote:
> On Wed, Nov 9, 2016 at 9:58 PM, Amit Langote
>  wrote:
>>> With all patches applied, "make check" fails with a bunch of diffs
>>> that look like this:
>>>
>>>   Check constraints:
>>> -     "pt1chk2" CHECK (c2 <> ''::text)
>>>       "pt1chk3" CHECK (c2 <> ''::text)
>>
>> Hm, I can't seem to reproduce this one.  Is it perhaps possible that you
>> applied the patches on top of some other WIP patches or something?
> 
> Nope.  I just checked and this passes with only 0001 and 0002 applied,
> but when I add 0003 and 0004 then it starts failing.

Sorry, it definitely wasn't an error on your part.

> It appears that
> the problem starts at this point in the foreign_data test:
> 
> ALTER TABLE pt1 DROP CONSTRAINT pt1chk2 CASCADE;
> 
> After that command, in the expected output, pt1chk2 stops showing up
> in the output of \d+ pt1, but continues to appear in the output of \d+
> ft2.  With your patch, however, it stops showing up for ft2 also.  If
> that's not also happening for you, it might be due to an uninitialized
> variable someplace.

Thanks for the context.  I think I found the culprit variable in
MergeConstraintsIntoExisting() and fixed it.  As you correctly guessed,
the uninitialized variable caused (in your environment) even non-partition
child relations to be treated partitions and hence forced any merged
constraints to be non-local in all cases, not just in case of partitions.
Which meant the command you quoted would even drop the ft2's (a child)
constraint because its conislocal is wrongly false.

> 
> +        /* Force inheritance recursion, if partitioned table. */
> 
> Doesn't match code (any more).

Fixed.

>>> I think "partitioning key" is a bit awkward and actually prefer
>>> "partiton key".  But "partition method" sounds funny so I would go
>>> with "partitioning method".
>>
>> OK, "partition key" and "partitioning method" it is then.  Source code
>> comments, error messages, variables call the latter (partitioning)
>> "strategy" though which hopefully is fine.
>
> Oh, I like "partitioning strategy".  Can we standardize on that?

OK, done.

>>> I would be in favor of committing the initial patch set without that,
>>> and then considering the possibility of adding it later.  If we
>>> include it in the initial patch set we are stuck with it.
>>
>> OK, I have removed the syntactic ability to specify INCLUSIVE/EXCLUSIVE
>> with each of the range bounds.
>>
>> I haven't changed any code (such as comparison functions) that manipulates
>> instances of PartitionRangeBound which has a flag called inclusive.  I
>> didn't remove the flag, but is instead just set to (is_lower ? true :
>> false) when initializing from the parse node. Perhaps, there is some scope
>> for further simplifying that code, which you probably alluded to when you
>> proposed that we do this.
>
> Yes, you need to rip out all of the logic that supports it.  Having
> the logic to support it but not the syntax is bad because then that
> code can't be properly tested.

Agreed, done.


Attached updated patches.

Thanks,
Amit