On Fri, Mar 21, 2014 at 12:12 AM, Noah Misch <n...@leadboat.com> wrote: > We added these ConstrCheck fields for 9.2, but equalTupleDescs() did not get > the memo. I looked for resulting behavior problems, and I found one in > RelationClearRelation() only. Test case: > > set constraint_exclusion = on; > drop table if exists ccvalid_test; > create table ccvalid_test (c int); > alter table ccvalid_test add constraint x check (c > 0) not valid; > > begin; > -- constraint_exclusion won't use an invalid constraint. > explain (costs off) select * from ccvalid_test where c = 0; > -- Make it valid. > alter table ccvalid_test validate constraint x; > -- Local invalidation rebuilt the Relation and decided the TupleDesc hadn't > -- changed, so we're still not using the constraint. > explain (costs off) select * from ccvalid_test where c = 0; > commit; > > -- At COMMIT, we destroyed the then-closed Relation in response to shared > -- invalidation. Now constraint_exclusion sees the valid constraint. > explain (costs off) select * from ccvalid_test where c = 0; > > > Currently, the damage is limited to later commands in the transaction that > issued ALTER TABLE VALIDATE. Changing ccvalid requires AccessExclusiveLock, > so no other backend will have an affected, open relcache entry to rebuild. > Shared invalidation will make the current backend destroy its affected > relcache entry before starting a new transaction. However, the impact will > not be so limited once we allow ALTER TABLE VALIDATE to run with a mere > ShareUpdateExclusiveLock. (I discovered this bug while reviewing the patch > implementing that very feature.) > > I don't see a way to get trouble from the ccnoinherit omission. You can't > change ccnoinherit except by dropping and recreating the constraint, and each > of the drop and create operations would make equalTupleDescs() detect a > change. The same can be said of "ccbin", but equalTupleDescs() does compare > that field. For simplicity, I'll have it compare ccnoinherit. > > CreateTupleDescCopyConstr() also skips ccnoinherit. I don't see a resulting > live bug, but it's worth correcting. > > Given the minor symptoms in released versions, I lean against a back-patch.
FWIW, I'd lean toward a back-patch. It's probably not a big deal either way, but I have a hard time seeing what risk we're avoiding by not back-patching, and it seems potentially confusing to leave known-wrong logic floating around in older branches. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
