On Tue, Mar 6, 2018 at 8:34 PM, David Rowley <david.row...@2ndquadrant.com> wrote: > One thing I've learned in my time working with PostgreSQL is that, if > there's a known hole, someone's probably going to fall down it > eventually. I like working with PostgreSQL because we're pretty > careful to not make holes that people can fall down, or if there is > some hole that cannot be filled in, we try to put a fence around it > with a sign, (e.g rename pg_xlog to pg_wal). I'm not strongly opposed > to your ideas, I probably don't have a complete understanding of the > idea anyway. But from what I understand it looks like you want to take > something that works quite well and make it work less well, and there > appears not to be a good reason provided of why you want to do that. > > Is it because you want to simplify the patch due to concerns about it > being too much logic to get right for PG11?
My understanding is that the patch as submitted is fundamentally broken in multiple ways. As Amit said a few emails upthread, "So while the patch's previous approach to convert the query's constant value to the desired type was wrong, this is wronger. :-(" I agree with that analysis. As I tried to explain in my last email, if you've got something like foo > 'blarfle'::type1 and foo > 'hoge'::type2, there may actually be no way at all to determine which of those clauses is more restrictive. The fact that > was used in the query to compare foo with a value of type1 and, separately, with a value of type2 means that those operators exist, but it does not follow that the opfamily provides an operator which can compare type1 to type2. As far as I can see, what this means is that, in general, the approach the patch takes to eliminating redundant clauses just doesn't work; and in the general case I don't think there's much hope of saving it. The question of whether the patch does too much work at execution time or not is maybe arguable -- my position is that it does -- but working properly in the face of cross-type comparisons is non-negotiable. The use of evaluate_expr() is also completely wrong and has got to be fixed. I already wrote about that upthread so I won't repeat it here. I'm pretty sure that the current design, if allowed to stand, would have lots of bad consequences. As I understand it, Amit is currently hacking on the patch to try to fix these issues. If he comes up with something that works properly with cross-type comparisons and doesn't abuse evaluate_expr() but still does more work than I'd ideally prefer at execution time, I'll consider holding my nose and consider it anyway. But considering the amount of rework that I think is needed, I don't really see why we wouldn't adopt a design that minimizes execution time work, too. In short, I don't think I'm trying to make something that works quite well work less well, because I don't think the patch as it stands can be correctly described as working quite well. > Let's say there was some view like: > > CREATE VIEW vw_ledger_2018 AS SELECT * FROM ledger WHERE postdate > BETWEEN '2018-01-01' AND '2018-12-13'; > > And a user comes along and does: > > SELECT * FROM vw_ledger_2018 WHERE postdate BETWEEN '2018-03-01' AND > '2018-03-31' > > If we just take the first from each op strategy then we'll not have > managed to narrow the case down to just the March partition. You might > argue that this should be resolved at some higher level in the > planner, but that does nothing for the run-time pruning case. Yeah, that's a good example of how this could happen in real life. So far I see three ways forward here: 1. If we've got multiple redundant quals, ignore all but one of them for purposes of partition pruning. Hope users don't get mad. 2. If we've got multiple redundant quals, do multiple checks. Hope this isn't annoyingly slow (or too much ugly code). 3. If we've got multiple redundant quals but no cross-type operators are in use, evaluate all of the expressions and pick the highest or lowest value as appropriate. Otherwise fall back to #1 or #2. For bonus points, do this when cross-type operators ARE in use and the additional cross-type operators that we need to figure out the highest or lowest value, as appropriate, is also available. I'm OK with any of those approaches; that is, I will not seek to block a merely patch on the basis of which of those options it chooses. I think they are all defensible. Options that are not OK include (a) trying to cast a value of one type to another type, because that could turn a query that would have simply returned no rows into an error case or (b) supposing that all types in an opfamily are binary coercible to each other, because that's just plain wrong. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company