|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|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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  addresses that. In fact, I started this thread prompted
>> by some discussion about Boolean partitions on that thread .
>> 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
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.
> 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.
|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|