On Sun, 15 Apr 2018 at 19:09, Sergei Kornilov <s...@zsrv.org> wrote: > Attached updated patch follows recent Reorganize partitioning code commit.
I've made a pass over v10. I think it's in pretty good shape, but I did end up changing a few small things. 1. Tweaked the documents a bit. I was going to just change "Full table scan" to "A full table scan", but I ended up rewriting the entire paragraph. 2. In ATRewriteTables, updated comment to mention "If required" since the scan may not be required if SET NOT NULLs are already proved okay. 3. Updated another forgotten comment in ATRewriteTables. 4. Removed mention about table's with OIDs in ATExecAddColumn(). This seems left over from 578b229718. 5. Changed ATExecSetNotNull not to check for matching constraints if we're already going to make a pass over the table. Setting verify_new_notnull to true again does not make it any more true. :) 6. Tweaked NotNullImpliedByRelConstraints() a bit. You were calling lappend on an empty list. make_list1() is fine. Don't need a variable for that, just pass it to the function. 7. Tightened up the comments in ConstraintImpliedByRelConstraint() to mention that it only supports immutable quals containing vars with varno=1. Removed the comment near the bottom that mentioned that in passing. The only thing that I'm a bit unsure of is the tests. I've read the thread and I see the discussion above about it. I'd personally have thought INFO was fine since ATTACH PARTITION does that, but I see objections. It appears that all the tests just assume that the CHECK constraint was used to validate the SET NOT NULL. I'm not all that sure if there's any good reason not to set client_min_messages = 'debug1' just before your test then RESET client_min_messages; afterwards. No other tests seem to do it, but my only thoughts on the reason not to would be that it might fail if someone added another debug somewhere, but so what? they should update the expected results too. Oh, one other thing I don't really like is that ConstraintImpliedByRelConstraint() adds all of the other column's IS NOT NULL constraints to the existConstraint. This is always wasted effort for the SET NOT NULL use case. I wonder if a new flag should be added to control this, or even better, separate that part out and put back the function named PartConstraintImpliedByRelConstraint and have it do the IS NOT NULL building before calling ConstraintImpliedByRelConstraint(). No duplicate code that way and you also don't need to touch partbound.c Setting this on waiting on author. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
alter_table_set_not_null_by_constraints_only_v10_fixes.patch
Description: Binary data
