Re: equalTupleDescs() ignores ccvalid/ccnoinherit

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

In response to

Responses

Browse pgsql-hackers by date

  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