From: | Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | Stephen Frost <sfrost(at)snowman(dot)net> |
Cc: | Mark Dilger <hornschnorter(at)gmail(dot)com>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Boolean partitions syntax |
Date: | 2018-01-26 01:44:08 |
Message-ID: | 4cb808d7-1056-ca24-2549-fcace1022d83@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi Stephen.
On 2018/01/26 10:16, Stephen Frost wrote:
> * Amit Langote (Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp) wrote:
> Still compiles and passes regression tests, which is good.
Thanks for looking at this.
>>> I extended your test a bit to check whether partitions over booleans are useful.
>>> Note specifically the 'explain' output, which does not seem to restrict the scan
>>> to just the relevant partitions. You could easily argue that this is beyond the scope
>>> of your patch (and therefore not your problem), but I doubt it makes much sense
>>> to have boolean partitions without planner support for skipping partitions like is
>>> done for tables partitioned over other data types.
>>
>> Yeah. Actually, I'm aware that the planner doesn't work this. While
>> constraint exclusion (planner's current method of skipping partitions)
>> does not work with IS TRUE/FALSE/UNKNOWN clauses, the new partition
>> pruning patch [1] addresses that. In fact, I started this thread prompted
>> by some discussion about Boolean partitions on that thread [2].
>>
>> That said, someone might argue that we should also fix constraint
>> exclusion (the current method of partition pruning) so that partition
>> skipping works correctly for Boolean partitions.
>
> For my 2c, at least, I don't think we need to fix constraint exclusion
> to work for this case and hopefully we'll get the partition pruning
> patch in but I'm not sure that we really need to wait for that either.
> Worst case, we can simply document that the planner won't actually
> exclude boolean-based partitions in this release and then fix it in the
> future.
Yeah, I meant this just as a tiny syntax extension patch.
> Looking over this patch, it seems to be in pretty good shape to me
> except that I'm not sure why you went with the approach of naming the
> function 'NoCast'. There's a number of other functions just above
> makeBoolAConst() that don't include a TypeCast and it seems like having
> makeBoolConst() and makeBoolConstCast() would be more in-line with the
> existing code (see makeStringConst() and makeStringConstCast() for
> example, but also makeIntConst(), makeFloatConst(), et al). That would
> require updating the existing callers that really want a TypeCast result
> even though they're calling makeBoolAConst(), but that seems like a good
> improvement to be making.
Agreed, done.
> I could see an argument that we should have two patches (one to rename
> the existing function, another to add support for boolean) but that's
> really up to whatever committer picks this up. For my 2c, I don't think
> it's really necessary to split it into two patches.
OK, I kept the function name change part with the main patch.
Attached updated patch.
Thanks,
Amit
Attachment | Content-Type | Size |
---|---|---|
v3-0001-Allow-Boolean-values-in-partition-FOR-VALUES-clau.patch | text/plain | 6.2 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tsunakawa, Takayuki | 2018-01-26 01:54:04 | RE: [HACKERS][PATCH] Applying PMDK to WAL operations for persistent memory |
Previous Message | Robert Haas | 2018-01-26 01:40:27 | Re: [PATCH] Logical decoding of TRUNCATE |