Thread: 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
Attachment
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
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
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
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