Greetings Amit,
* Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote:
> On 2017/12/20 6:46, Mark Dilger wrote:
> >> On Dec 12, 2017, at 10:32 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> >> Added to CF: https://commitfest.postgresql.org/16/1410/
> >
> > This compiles and passes the regression tests for me.
>
> Thanks for the review.
Still compiles and passes regression tests, which is good.
> > 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.
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.
Thanks!
Stephen