On 2017/06/08 19:25, Ashutosh Bapat wrote: > On Thu, Jun 8, 2017 at 3:13 PM, Amit Langote >>> I think this code could be removed entirely in light of commit >>> 3ec76ff1f2cf52e9b900349957b42d28128b7bc7. >> >> I am assuming you think that because now we emit IS NOT NULL constraint >> internally for any partition keys that do not allow null values (including >> all the keys of range partitions as of commit >> 3ec76ff1f2cf52e9b900349957b42d28128b7bc7). But those IS NOT NULL >> constraint expressions are inconclusive as far as the application of >> predicate_implied_by() to determine if we can skip the scan is concerned. >> So even if predicate_implied_by() returned 'true', we cannot conclude, >> just based on that result, that there are not any null values in the >> partition keys. > > I am not able to understand this. Are you saying that > predicate_implied_by() returns true even when it's not implied when > NOT NULL constraints are involved? That sounds like a bug in > predicate_implied_by(), which should be fixed instead of adding code > to pepper over it?
No, it's not a bug of predicate_implied_by(). I meant to say predicate_implied_by() isn't exactly designed for ATExecAttachPartition's purpose, especially its treatment of IS NOT NULL constraints is not suitable for this application. To prove that the table cannot contain NULLs when it shouldn't because of the partition constraint, we must look for explicit NOT NULL constraints on the partition key columns, instead of relying on the predicate_implied_by()'s proof. See the following explanation for why that is so (or at least I think is so): There is this text in the header comment of predicate_implied_by_simple_clause(), which is where the individual pairs of expressions are compared and/or passed to operator_predicate_proof(), which mentions that the treatment of IS NOT NULL predicate is based on the assumption that 'restrictions' that are passed to predicate_implied_by() are a query's WHERE clause restrictions, *not* CHECK constraints that are checked when inserting data into a table. * When the predicate is of the form "foo IS NOT NULL", we can conclude that * the predicate is implied if the clause is a strict operator or function * that has "foo" as an input. In this case the clause must yield NULL when * "foo" is NULL, which we can take as equivalent to FALSE because we know * we are within an AND/OR subtree of a WHERE clause. (Again, "foo" is * already known immutable, so the clause will certainly always fail.) * Also, if the clause is just "foo" (meaning it's a boolean variable), * the predicate is implied since the clause can't be true if "foo" is NULL. As mentioned above, note the following part: which we can take as equivalent to FALSE because we know we are within an AND/OR subtree of a WHERE clause. Which is not what we are passing to predicate_implied_by() in ATExecAttachPartition(). We are passing it the table's CHECK constraint clauses which behave differently for the NULL result on NULL input - they *allow* the row to be inserted. Which means that there will be rows with NULLs in the partition key, even if predicate_refuted_by() said that there cannot be. We will end up *wrongly* skipping the validation scan if we relied on just the predicate_refuted_by()'s result. That's why there is code to check for explicit NOT NULL constraints on the partition key columns. If there are, it's OK then to assume that all the partition constraints are satisfied by existing constraints. One more thing: if any partition key happens to be an expression, which there cannot be NOT NULL constraints for, we just give up on skipping the scan, because we don't have any declared knowledge about whether those keys are also non-null, which we want for partitions that do not accept null values. Does that make sense? Thanks, Amit PS: Also interesting to note is the difference in behavior between ExecQual() and ExecCheck() on NULL result. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
