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:

Reply via email to