From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | Noah Misch <noah(at)leadboat(dot)com> |
Cc: | "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, Simon Riggs <simon(at)2ndquadrant(dot)com> |
Subject: | Re: equalTupleDescs() ignores ccvalid/ccnoinherit |
Date: | 2014-03-21 18:26:47 |
Message-ID: | CA+Tgmoao61UpGihMNBA8eRagFSm+G7UKne38hvCnhrzKFTDrjg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, Mar 21, 2014 at 12:12 AM, Noah Misch <noah(at)leadboat(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Simon Riggs | 2014-03-21 18:53:27 | Re: ALTER TABLE lock strength reduction patch is unsafe Reply-To: |
Previous Message | Andres Freund | 2014-03-21 18:22:31 | [RFC, POC] Don't require a NBuffer sized PrivateRefCount array of local buffer pins |