On Fri, Jun 9, 2017 at 10:31 AM, Amit Langote <langote_amit...@lab.ntt.co.jp> wrote: > 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 for the long explanation. I guess, this should be written in comments somewhere in the code there. I see following comment in ATExecAttachPartition() -- * * There is a case in which we cannot rely on just the result of the * proof. */ -- I guess, this comment should be expanded to explain what you wrote above. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers