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
Attachments:
ccvalid-ccnoinherit-v1.patchtext/plain; charset=us-asciiDownload
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index db8cb82..74cfb64 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -204,6 +204,7 @@ CreateTupleDescCopyConstr(TupleDesc tupdesc)
if (constr->check[i].ccbin)
cpy->check[i].ccbin = pstrdup(constr->check[i].ccbin);
cpy->check[i].ccvalid = constr->check[i].ccvalid;
+ cpy->check[i].ccnoinherit = constr->check[i].ccnoinherit;
}
}
@@ -458,7 +459,9 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
for (j = 0; j < n; check2++, j++)
{
if (strcmp(check1->ccname, check2->ccname) == 0 &&
- strcmp(check1->ccbin, check2->ccbin) == 0)
+ strcmp(check1->ccbin, check2->ccbin) == 0 &&
+ check1->ccvalid == check2->ccvalid &&
+ check1->ccnoinherit == check2->ccnoinherit)
break;
}
if (j >= n)
On Fri, Mar 21, 2014 at 12:12 AM, Noah Misch <noah@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:
http://www.postgresql.org/mailpref/pgsql-hackers
On 21 March 2014 18:26, Robert Haas <robertmhaas@gmail.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas <robertmhaas@gmail.com> writes:
On Fri, Mar 21, 2014 at 12:12 AM, Noah Misch <noah@leadboat.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Mar 21, 2014 at 06:59:05PM +0000, Simon Riggs wrote:
On 21 March 2014 18:26, Robert Haas <robertmhaas@gmail.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers