Re: equalTupleDescs() ignores ccvalid/ccnoinherit

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