Hi Stephen.
On 2018/01/26 10:16, Stephen Frost wrote:
> * Amit Langote (Langote_Amit_f8@lab.ntt.co.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
