Backpatch FK changes to 7.3 and 7.2?
Followup-To: pgsql-hackers@postgresql.org
The changes I committed to address most of the FK deadlock problems
reported can easily be applied to the 7.3 and 7.2 source trees as well.
Except for a slight change in the text of the error message that gets
thrown "if one tries to delete a referenced PK for which a FK with ON
DELETE SET DEFAULT exists" (it's a rare case, believe me), this patch
would qualify for backpatching. The unnecessary FOR UPDATE lock of
referenced rows could be counted as a bug.
Opinions?
Jan
--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#================================================== JanWieck@Yahoo.com #
Jan Wieck <JanWieck@Yahoo.com> writes:
The changes I committed to address most of the FK deadlock problems
reported can easily be applied to the 7.3 and 7.2 source trees as well.
I would vote against it --- it seems a nontrivial change to be putting
into stable branches, especially without any testing ;-)
regards, tom lane
The changes I committed to address most of the FK deadlock problems
reported can easily be applied to the 7.3 and 7.2 source trees as well.Except for a slight change in the text of the error message that gets
thrown "if one tries to delete a referenced PK for which a FK with ON
DELETE SET DEFAULT exists" (it's a rare case, believe me), this patch
would qualify for backpatching. The unnecessary FOR UPDATE lock of
referenced rows could be counted as a bug.Opinions?
Since I seem to suffer from these horrible deadlock problems all the time, I'd like it to be backported to 7.3...
Chris
The changes I committed to address most of the FK deadlock problems
reported can easily be applied to the 7.3 and 7.2 source trees as well.Except for a slight change in the text of the error message that gets
thrown "if one tries to delete a referenced PK for which a FK with ON
DELETE SET DEFAULT exists" (it's a rare case, believe me), this patch
would qualify for backpatching. The unnecessary FOR UPDATE lock of
referenced rows could be counted as a bug.Opinions?
Since I seem to suffer from these horrible deadlock problems all the time, I'd like it to be backported to 7.3...
Me too!
--
Tatsuo Ishii
On Tue, 8 Apr 2003, Tatsuo Ishii wrote:
The changes I committed to address most of the FK deadlock problems
reported can easily be applied to the 7.3 and 7.2 source trees as well.Except for a slight change in the text of the error message that gets
thrown "if one tries to delete a referenced PK for which a FK with ON
DELETE SET DEFAULT exists" (it's a rare case, believe me), this patch
would qualify for backpatching. The unnecessary FOR UPDATE lock of
referenced rows could be counted as a bug.Opinions?
Since I seem to suffer from these horrible deadlock problems all the
time, I'd like it to be backported to 7.3...Me too!
As a note, this'll solve some of the deadlocks on fk update (generally the
key values aren't touched) but not insert related ones (two rows inserted
to the same primary key causing one to wait and possible deadlocks)
In any case, why don't we get a patch against 7.3, and make an
announcement and let people who are interested use it and test it. With
in-field testing it'd probably be safe enough. :)
Stephan Szabo wrote:
On Tue, 8 Apr 2003, Tatsuo Ishii wrote:
The changes I committed to address most of the FK deadlock problems
reported can easily be applied to the 7.3 and 7.2 source trees as well.Except for a slight change in the text of the error message that gets
thrown "if one tries to delete a referenced PK for which a FK with ON
DELETE SET DEFAULT exists" (it's a rare case, believe me), this patch
would qualify for backpatching. The unnecessary FOR UPDATE lock of
referenced rows could be counted as a bug.Opinions?
Since I seem to suffer from these horrible deadlock problems all the
time, I'd like it to be backported to 7.3...Me too!
As a note, this'll solve some of the deadlocks on fk update (generally the
key values aren't touched) but not insert related ones (two rows inserted
to the same primary key causing one to wait and possible deadlocks)In any case, why don't we get a patch against 7.3, and make an
announcement and let people who are interested use it and test it. With
in-field testing it'd probably be safe enough. :)
Here it is.
Jan
--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#================================================== JanWieck@Yahoo.com #
Attachments:
ri_triggers.c.diff.732text/plain; charset=us-ascii; name=ri_triggers.c.diff.732Download
*** ri_triggers.c.orig Fri Apr 4 10:41:45 2003
--- ri_triggers.c Sun Apr 6 12:36:54 2003
***************
*** 391,403 ****
}
/*
! * Note: We cannot avoid the check on UPDATE, even if old and new key
! * are the same. Otherwise, someone could DELETE the PK that consists
! * of the DEFAULT values, and if there are any references, a ON DELETE
! * SET DEFAULT action would update the references to exactly these
! * values but we wouldn't see that weired case (this is the only place
! * to see it).
*/
if (SPI_connect() != SPI_OK_CONNECT)
elog(WARNING, "SPI_connect() failed in RI_FKey_check()");
--- 391,409 ----
}
/*
! * No need to check anything if old and new references are the
! * same on UPDATE.
*/
+ if (TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event))
+ {
+ if (ri_KeysEqual(fk_rel, old_row, new_row, &qkey,
+ RI_KEYPAIR_FK_IDX))
+ {
+ heap_close(pk_rel, RowShareLock);
+ return PointerGetDatum(NULL);
+ }
+ }
+
if (SPI_connect() != SPI_OK_CONNECT)
elog(WARNING, "SPI_connect() failed in RI_FKey_check()");
***************
*** 2787,2792 ****
--- 2793,2808 ----
heap_close(fk_rel, RowExclusiveLock);
+ /*
+ * In the case we delete the row who's key is equal to the
+ * default values AND a referencing row in the foreign key
+ * table exists, we would just have updated it to the same
+ * values. We need to do another lookup now and in case a
+ * reference exists, abort the operation. That is already
+ * implemented in the NO ACTION trigger.
+ */
+ RI_FKey_noaction_del(fcinfo);
+
return PointerGetDatum(NULL);
/*
***************
*** 3077,3082 ****
--- 3093,3108 ----
elog(WARNING, "SPI_finish() failed in RI_FKey_setdefault_upd()");
heap_close(fk_rel, RowExclusiveLock);
+
+ /*
+ * In the case we updated the row who's key was equal to the
+ * default values AND a referencing row in the foreign key
+ * table exists, we would just have updated it to the same
+ * values. We need to do another lookup now and in case a
+ * reference exists, abort the operation. That is already
+ * implemented in the NO ACTION trigger.
+ */
+ RI_FKey_noaction_upd(fcinfo);
return PointerGetDatum(NULL);
I just created a patches/v7.3.2 directory in FTP, and copied this into
there ...
On Tue, 8 Apr 2003, Jan Wieck wrote:
Show quoted text
Stephan Szabo wrote:
On Tue, 8 Apr 2003, Tatsuo Ishii wrote:
The changes I committed to address most of the FK deadlock problems
reported can easily be applied to the 7.3 and 7.2 source trees as well.Except for a slight change in the text of the error message that gets
thrown "if one tries to delete a referenced PK for which a FK with ON
DELETE SET DEFAULT exists" (it's a rare case, believe me), this patch
would qualify for backpatching. The unnecessary FOR UPDATE lock of
referenced rows could be counted as a bug.Opinions?
Since I seem to suffer from these horrible deadlock problems all the
time, I'd like it to be backported to 7.3...Me too!
As a note, this'll solve some of the deadlocks on fk update (generally the
key values aren't touched) but not insert related ones (two rows inserted
to the same primary key causing one to wait and possible deadlocks)In any case, why don't we get a patch against 7.3, and make an
announcement and let people who are interested use it and test it. With
in-field testing it'd probably be safe enough. :)Here it is.
Jan
--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#================================================== JanWieck@Yahoo.com #
Thanks
"Marc G. Fournier" wrote:
I just created a patches/v7.3.2 directory in FTP, and copied this into
there ...On Tue, 8 Apr 2003, Jan Wieck wrote:
Stephan Szabo wrote:
[...]
In any case, why don't we get a patch against 7.3, and make an
announcement and let people who are interested use it and test it. With
in-field testing it'd probably be safe enough. :)Here it is.
--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#================================================== JanWieck@Yahoo.com #
Jan Wieck wrote:
In any case, why don't we get a patch against 7.3, and make an
announcement and let people who are interested use it and test it. With
in-field testing it'd probably be safe enough. :)Here it is.
[patch... skipping]
I applied the patch to a 7.3.2 installation, and did a make clean, make,
make check. There is one regression error. Is this an expected behaviour? Or
did I do something wrong? See regression diffs:
*** ./expected/foreign_key.out Sun Sep 22 02:37:09 2002
--- ./results/foreign_key.out Sat Apr 12 20:44:54 2003
***************
*** 882,888 ****
ERROR: $1 referential integrity violation - key in pktable still
referenced from pktable
-- fails (1,1) is being referenced (twice)
update pktable set base1=3 where base1=1;
! ERROR: $1 referential integrity violation - key referenced from pktable
not found in pktable
-- this sequence of two deletes will work, since after the first there
will be no (2,*) references
delete from pktable where base2=2;
delete from pktable where base1=2;
--- 882,888 ----
ERROR: $1 referential integrity violation - key in pktable still
referenced from pktable
-- fails (1,1) is being referenced (twice)
update pktable set base1=3 where base1=1;
! ERROR: $1 referential integrity violation - key in pktable still
referenced from pktable
-- this sequence of two deletes will work, since after the first there
will be no (2,*) references
delete from pktable where base2=2;
delete from pktable where base1=2;Best Regards,
Michael Paesold
Michael Paesold wrote:
Jan Wieck wrote:
In any case, why don't we get a patch against 7.3, and make an
announcement and let people who are interested use it and test it. With
in-field testing it'd probably be safe enough. :)Here it is.
[patch... skipping]
I applied the patch to a 7.3.2 installation, and did a make clean, make,
make check. There is one regression error. Is this an expected behaviour? Or
did I do something wrong? See regression diffs:
This is expected. The RI violation in the case of deleting a referenced
PK row that actually is the defaults values of the FK table is now
detected differently. Hence the change in the error message text.
Jan
*** ./expected/foreign_key.out Sun Sep 22 02:37:09 2002 --- ./results/foreign_key.out Sat Apr 12 20:44:54 2003 *************** *** 882,888 **** ERROR: $1 referential integrity violation - key in pktable still referenced from pktable -- fails (1,1) is being referenced (twice) update pktable set base1=3 where base1=1; ! ERROR: $1 referential integrity violation - key referenced from pktable not found in pktable -- this sequence of two deletes will work, since after the first there will be no (2,*) references delete from pktable where base2=2; delete from pktable where base1=2; --- 882,888 ---- ERROR: $1 referential integrity violation - key in pktable still referenced from pktable -- fails (1,1) is being referenced (twice) update pktable set base1=3 where base1=1; ! ERROR: $1 referential integrity violation - key in pktable still referenced from pktable -- this sequence of two deletes will work, since after the first there will be no (2,*) references delete from pktable where base2=2; delete from pktable where base1=2;Best Regards,
Michael Paesold
--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#================================================== JanWieck@Yahoo.com #
Seems like a small reasonable patch to me, and several folks want it.
---------------------------------------------------------------------------
Jan Wieck wrote:
Stephan Szabo wrote:
On Tue, 8 Apr 2003, Tatsuo Ishii wrote:
The changes I committed to address most of the FK deadlock problems
reported can easily be applied to the 7.3 and 7.2 source trees as well.Except for a slight change in the text of the error message that gets
thrown "if one tries to delete a referenced PK for which a FK with ON
DELETE SET DEFAULT exists" (it's a rare case, believe me), this patch
would qualify for backpatching. The unnecessary FOR UPDATE lock of
referenced rows could be counted as a bug.Opinions?
Since I seem to suffer from these horrible deadlock problems all the
time, I'd like it to be backported to 7.3...Me too!
As a note, this'll solve some of the deadlocks on fk update (generally the
key values aren't touched) but not insert related ones (two rows inserted
to the same primary key causing one to wait and possible deadlocks)In any case, why don't we get a patch against 7.3, and make an
announcement and let people who are interested use it and test it. With
in-field testing it'd probably be safe enough. :)Here it is.
Jan
--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#================================================== JanWieck@Yahoo.com #
*** ri_triggers.c.orig Fri Apr 4 10:41:45 2003 --- ri_triggers.c Sun Apr 6 12:36:54 2003 *************** *** 391,403 **** }/*
! * Note: We cannot avoid the check on UPDATE, even if old and new key
! * are the same. Otherwise, someone could DELETE the PK that consists
! * of the DEFAULT values, and if there are any references, a ON DELETE
! * SET DEFAULT action would update the references to exactly these
! * values but we wouldn't see that weired case (this is the only place
! * to see it).
*/
if (SPI_connect() != SPI_OK_CONNECT)
elog(WARNING, "SPI_connect() failed in RI_FKey_check()");--- 391,409 ---- }/* ! * No need to check anything if old and new references are the ! * same on UPDATE. */ + if (TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event)) + { + if (ri_KeysEqual(fk_rel, old_row, new_row, &qkey, + RI_KEYPAIR_FK_IDX)) + { + heap_close(pk_rel, RowShareLock); + return PointerGetDatum(NULL); + } + } + if (SPI_connect() != SPI_OK_CONNECT) elog(WARNING, "SPI_connect() failed in RI_FKey_check()");*************** *** 2787,2792 **** --- 2793,2808 ----heap_close(fk_rel, RowExclusiveLock);
+ /* + * In the case we delete the row who's key is equal to the + * default values AND a referencing row in the foreign key + * table exists, we would just have updated it to the same + * values. We need to do another lookup now and in case a + * reference exists, abort the operation. That is already + * implemented in the NO ACTION trigger. + */ + RI_FKey_noaction_del(fcinfo); + return PointerGetDatum(NULL);/* *************** *** 3077,3082 **** --- 3093,3108 ---- elog(WARNING, "SPI_finish() failed in RI_FKey_setdefault_upd()");heap_close(fk_rel, RowExclusiveLock); + + /* + * In the case we updated the row who's key was equal to the + * default values AND a referencing row in the foreign key + * table exists, we would just have updated it to the same + * values. We need to do another lookup now and in case a + * reference exists, abort the operation. That is already + * implemented in the NO ACTION trigger. + */ + RI_FKey_noaction_upd(fcinfo);return PointerGetDatum(NULL);
---------------------------(end of broadcast)---------------------------
TIP 4: Don't 'kill -9' the postmaster
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073Bruce Momjian wrote:
Seems like a small reasonable patch to me, and several folks want it.
I am bit worried with this regression issue. I posted this about a week ago
but didn't get any response. Anyone else tested this? I re-run the test on a
fresh download of 7.3.2. Same. Jan, you mentioned something concerning an
error messages - is this issue causing the regression error?
This is the message I posted before:
I applied the patch to a 7.3.2 installation, and did a make clean, make,
make check. There is one regression error. Is this an expected behaviour? Or
did I do something wrong? See regression diffs:
*** ./expected/foreign_key.out Sun Sep 22 02:37:09 2002
--- ./results/foreign_key.out Sat Apr 12 20:44:54 2003
***************
*** 882,888 ****
ERROR: $1 referential integrity violation - key in pktable still
referenced from pktable
-- fails (1,1) is being referenced (twice)
update pktable set base1=3 where base1=1;
! ERROR: $1 referential integrity violation - key referenced from pktable
not found in pktable
-- this sequence of two deletes will work, since after the first there
will be no (2,*) references
delete from pktable where base2=2;
delete from pktable where base1=2;
--- 882,888 ----
ERROR: $1 referential integrity violation - key in pktable still
referenced from pktable
-- fails (1,1) is being referenced (twice)
update pktable set base1=3 where base1=1;
! ERROR: $1 referential integrity violation - key in pktable still
referenced from pktable
-- this sequence of two deletes will work, since after the first there
will be no (2,*) references
delete from pktable where base2=2;
delete from pktable where base1=2;Regards,
Michael Paesold
--------------------------------------------------------------------------
-
Jan Wieck wrote:
Stephan Szabo wrote:
On Tue, 8 Apr 2003, Tatsuo Ishii wrote:
The changes I committed to address most of the FK deadlock
problems
reported can easily be applied to the 7.3 and 7.2 source trees
as well.
Except for a slight change in the text of the error message that
gets
thrown "if one tries to delete a referenced PK for which a FK
with ON
DELETE SET DEFAULT exists" (it's a rare case, believe me), this
patch
would qualify for backpatching. The unnecessary FOR UPDATE lock
of
referenced rows could be counted as a bug.
Opinions?
Since I seem to suffer from these horrible deadlock problems all
the
time, I'd like it to be backported to 7.3...
Me too!
As a note, this'll solve some of the deadlocks on fk update (generally
the
key values aren't touched) but not insert related ones (two rows
inserted
to the same primary key causing one to wait and possible deadlocks)
In any case, why don't we get a patch against 7.3, and make an
announcement and let people who are interested use it and test it.
With
Show quoted text
in-field testing it'd probably be safe enough. :)
Here it is.
Jan
--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#================================================== JanWieck@Yahoo.com #
Michael Paesold wrote:
Bruce Momjian wrote:
Seems like a small reasonable patch to me, and several folks want it.
I am bit worried with this regression issue. I posted this about a week ago
but didn't get any response. Anyone else tested this? I re-run the test on a
fresh download of 7.3.2. Same. Jan, you mentioned something concerning an
error messages - is this issue causing the regression error?
I replied on Saturday to your question explaining that that change was
expected. You might consider telling those GMX idiots to think again
about their spam filters.
Jan
This is the message I posted before:
I applied the patch to a 7.3.2 installation, and did a make clean, make,
make check. There is one regression error. Is this an expected behaviour? Or
did I do something wrong? See regression diffs:*** ./expected/foreign_key.out Sun Sep 22 02:37:09 2002 --- ./results/foreign_key.out Sat Apr 12 20:44:54 2003 *************** *** 882,888 **** ERROR: $1 referential integrity violation - key in pktable still referenced from pktable -- fails (1,1) is being referenced (twice) update pktable set base1=3 where base1=1; ! ERROR: $1 referential integrity violation - key referenced from pktable not found in pktable -- this sequence of two deletes will work, since after the first there will be no (2,*) references delete from pktable where base2=2; delete from pktable where base1=2; --- 882,888 ---- ERROR: $1 referential integrity violation - key in pktable still referenced from pktable -- fails (1,1) is being referenced (twice) update pktable set base1=3 where base1=1; ! ERROR: $1 referential integrity violation - key in pktable still referenced from pktable -- this sequence of two deletes will work, since after the first there will be no (2,*) references delete from pktable where base2=2; delete from pktable where base1=2;Regards,
Michael Paesold--------------------------------------------------------------------------
-
Jan Wieck wrote:
Stephan Szabo wrote:
On Tue, 8 Apr 2003, Tatsuo Ishii wrote:
The changes I committed to address most of the FK deadlock
problems
reported can easily be applied to the 7.3 and 7.2 source trees
as well.
Except for a slight change in the text of the error message that
gets
thrown "if one tries to delete a referenced PK for which a FK
with ON
DELETE SET DEFAULT exists" (it's a rare case, believe me), this
patch
would qualify for backpatching. The unnecessary FOR UPDATE lock
of
referenced rows could be counted as a bug.
Opinions?
Since I seem to suffer from these horrible deadlock problems all
the
time, I'd like it to be backported to 7.3...
Me too!
As a note, this'll solve some of the deadlocks on fk update (generally
the
key values aren't touched) but not insert related ones (two rows
inserted
to the same primary key causing one to wait and possible deadlocks)
In any case, why don't we get a patch against 7.3, and make an
announcement and let people who are interested use it and test it.With
in-field testing it'd probably be safe enough. :)
Here it is.
Jan
--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#================================================== JanWieck@Yahoo.com #---------------------------(end of broadcast)---------------------------
TIP 5: Have you checked our extensive FAQ?
--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#================================================== JanWieck@Yahoo.com #