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
                   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...@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...@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:
>> 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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to