Re: equalTupleDescs() ignores ccvalid/ccnoinherit

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.

Thanks,
nm

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

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

On 21 March 2014 18:26, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

>> 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.

Agreed. It could lead to some other bug by not fixing it.

Well spotted, Noah, and thanks, since I believe it was my bug.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Mar 21, 2014 at 12:12 AM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>> 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.

I agree with Robert. This is a bug, let's fix it.

regards, tom lane

On Fri, Mar 21, 2014 at 06:59:05PM +0000, Simon Riggs wrote:
> On 21 March 2014 18:26, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> >> 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.
>
> Agreed. It could lead to some other bug by not fixing it.

Fair enough. I've back-patched it.

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com