|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|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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).
>>> 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?
>>> 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.
Attached updated patches.
|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|