Re: Declarative partitioning - another take

From: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Amit Langote <amitlangote09(at)gmail(dot)com>, Rajkumar Raghuwanshi <rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com>, Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Declarative partitioning - another take
Date: 2016-11-11 10:30:37
Message-ID: 8d7c35e3-1c85-33d0-4440-0a75bf9d31cd@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On 2016/11/11 6:51, Robert Haas wrote:
> On Wed, Nov 9, 2016 at 9:58 PM, Amit Langote
> <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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

Attachment Content-Type Size
0001-Catalog-and-DDL-for-partitioned-tables-13.patch text/x-diff 114.3 KB
0002-psql-and-pg_dump-support-for-partitioned-tables-13.patch text/x-diff 23.9 KB
0003-Catalog-and-DDL-for-partitions-13.patch text/x-diff 202.5 KB
0004-psql-and-pg_dump-support-for-partitions-13.patch text/x-diff 22.1 KB
0005-Teach-a-few-places-to-use-partition-check-quals-13.patch text/x-diff 30.9 KB
0006-Introduce-a-PartitionTreeNode-data-structure-13.patch text/x-diff 8.0 KB
0007-Tuple-routing-for-partitioned-tables-13.patch text/x-diff 42.6 KB
0008-Update-DDL-Partitioning-chapter-to-reflect-new-devel-13.patch text/x-diff 24.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Petr Jelinek 2016-11-11 10:41:49 Re: Logical Replication WIP
Previous Message Ashutosh Bapat 2016-11-11 10:22:39 Re: Push down more full joins in postgres_fdw