On 22 March 2018 at 22:38, Konstantin Knizhnik <k.knizh...@postgrespro.ru> wrote: > Attached please find rebased version of the patch.
Hi, I started looking over this patch and have a few comments: I don't think this range type idea is a great one. I don't think it's going to ever perform very well. I also see you're not checking the collation of the type anywhere. As of now, no range types have collation support, but you can't really go and code this with that assumption. I also don't really like the sequence scan over pg_range. Probably a better way to do this would be to add a new bt proc, like what was done in 0a459cec for 2 new functions. Something like BTISNEXTVAL_PROC and BTISPREVVAL_PROC. You'd then need to define functions for all the types based on integers, making functions which take 2 parameters of the type, and an additional collation param. The functions should return bool. int4_isnextval(2, 3, InvalidOid) would return true. You'd need to return false on wraparound. I also think that the patch is worth doing without the additional predicate_implied_by() smarts. In fact, I think strongly that the two should be considered as two independent patches. Partial indexes suffer from the same issue you're trying to address here and that would be resolved by any patch which makes changes to predicate_implied_by(). Probably the best place to put the code to skip the redundant quals is inside set_append_rel_size(). There's already code there that skips quals that are seen as const TRUE. This applies for UNION ALL targets with expressions that can be folded to constants once the qual is passed through adjust_appendrel_attrs() For example: explain select * from (select 1 as a from pg_class union all select 2 from pg_class) t where a = 1; I've attached a patch to show what I had in mind. I had to change how partition_qual is populated, which I was surprised to see only gets populated for sub-partitioned tables (the top-level parent won't have a qual since it's not partitioned) I didn't update the partition pruning code that assumes this is only populated for sub-partitioned table. That will need to be done. The patch comes complete with all the failing regression tests where the redundant quals have been removed by the new code. If you want to make this work for CHECK constraints too, then I think the code for that can be added to the same location as the code I added in the attached patch. You'd just need to fetch the check constraint quals and just add some extra code to check if the qual is redundant. Some new performance benchmarks would then be useful in order to find out how much overhead there is. We might learn that we don't want to enable it by default if it's too expensive. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
skip_redundant_partition_quals_poc.patch
Description: Binary data