Re: Backpatch FK changes to 7.3 and 7.2?

Followup-To: pgsql-hackers(at)postgresql(dot)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(at)Yahoo(dot)com #

Jan Wieck <JanWieck(at)Yahoo(dot)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(at)Yahoo(dot)com #

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:
> >
> > 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(at)Yahoo(dot)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(at)Yahoo(dot)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(at)Yahoo(dot)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(at)Yahoo(dot)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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

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?

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(at)Yahoo(dot)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(at)Yahoo(dot)com #
>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/docs/faqs/FAQ.html

--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#================================================== JanWieck(at)Yahoo(dot)com #