Greetings Amit, * Amit Langote (langote_amit...@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...@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
signature.asc
Description: PGP signature
