Re: Declarative partitioning - another take

I have not looked at the latest set of patches, but in the version
that I have we create one composite type for every partition. This
means that if there are thousand partitions, there will be thousand
identical entries in pg_type. Since all the partitions share the same
definition (by syntax), it doesn't make sense to add so many identical
entries. Moreover, in set_append_rel_size(), while translating the
expressions from parent to child, we add a ConvertRowtypeExpr instead
of whole-row reference if reltype of the parent and child do not match
(adjust_appendrel_attrs_mutator())                  if (appinfo->parent_reltype != appinfo->child_reltype)
    {                       ConvertRowtypeExpr *r = makeNode(ConvertRowtypeExpr);
 

I guess, we should set reltype of child to that of parent for
declarative partitions.

On Fri, Nov 11, 2016 at 4:00 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2016/11/11 6:51, Robert Haas wrote:
>> On Wed, Nov 9, 2016 at 9:58 PM, Amit Langote
>> <Langote_Amit_f8@lab.ntt.co.jp> 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



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company