skip FK trigger on UPDATE
This patch implements an idea discussed on -hackers recently: if an
UPDATE on a table with a foreign key does not modify any of the table's
foreign key columns, we can avoid enqueueing the foreign queue check
trigger.
I basically just moved the logic for the "are the keys equal?" test from
the FK trigger itself into the code that enqueues the trigger. I then
removed the keys-are-equal check from the FK trigger. I also had to
change (somewhat awkwardly) RI_FKey_keyequal_upd() to work for updates
on either the PK table or the FK table. I also removed the bogus
TriggerData argument from RI_FKey_keyequal_upd(), since AFAICS it is no
needed and merely adds confusion.
This patch does cause one change to the regression test output:
*** ./expected/foreign_key.out Fri May 27 23:58:54 2005
--- ./results/foreign_key.out Sat May 28 00:46:20 2005
***************
*** 911,918 ****
DETAIL: Key (base1,ptest1)=(2,2) is still referenced from table
"pktable".
-- fails (1,1) is being referenced (twice)
update pktable set base1=3 where base1=1;
! ERROR: insert or update on table "pktable" violates foreign key
constraint "pktable_base2_fkey"
! DETAIL: Key (base2,ptest2)=(1,1) is not present in table "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;
--- 911,918 ----
DETAIL: Key (base1,ptest1)=(2,2) is still referenced from table
"pktable".
-- fails (1,1) is being referenced (twice)
update pktable set base1=3 where base1=1;
! ERROR: update or delete on "pktable" violates foreign key constraint
"pktable_base2_fkey" on "pktable"
! DETAIL: Key (base1,ptest1)=(1,1) is still referenced from table
"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;I found this a bit strange: on the one hand I think the new error
message is actually more sensible, but I'm not sure what caused the
change in behavior. I'll have more of a think about this tomorrow.
-Neil
Attachments:
fk_update_elide_check-4.patchtext/x-patch; name=fk_update_elide_check-4.patch; x-mac-creator=0; x-mac-type=0Download+264-217
Neil Conway <neilc@samurai.com> writes:
I basically just moved the logic for the "are the keys equal?" test from
the FK trigger itself into the code that enqueues the trigger. I then
removed the keys-are-equal check from the FK trigger. I also had to
change (somewhat awkwardly) RI_FKey_keyequal_upd() to work for updates
on either the PK table or the FK table. I also removed the bogus
TriggerData argument from RI_FKey_keyequal_upd(), since AFAICS it is no
needed and merely adds confusion.
It would probably be cleaner to have two keys-are-equal check routines
than to contort RI_FKey_keyequal_upd to work this way.
You seem to have also done a fair amount of unrelated hacking around.
What's the point of removing the distinction between check_ins and
check_upd functions? I think that may confuse existing client code
that looks at the triggers, without really buying much. A possibly
stronger argument is that if we find down the road that we need
separate functions again, we'll be in a bit of a sticky place; at
least if we need it to fix a bug without forcing initdb.
This patch does cause one change to the regression test output:
That's discomforting --- you had better find out exactly why that
changed.
regards, tom lane
Tom Lane wrote:
You seem to have also done a fair amount of unrelated hacking around.
What's the point of removing the distinction between check_ins and
check_upd functions?
I talked about this in an earlier message to -hackers: check_upd was
actually unused (check_ins was used for both inserts and updates).
I think that may confuse existing client code
that looks at the triggers, without really buying much. A possibly
stronger argument is that if we find down the road that we need
separate functions again, we'll be in a bit of a sticky place; at
least if we need it to fix a bug without forcing initdb.
Hmm, I suppose -- if you prefer I can have check_ins called by the
INSERT trigger and check_upd called by the UPDATE trigger, which
probably makes more sense.
-Neil
Neil Conway <neilc@samurai.com> writes:
Tom Lane wrote:
What's the point of removing the distinction between check_ins and
check_upd functions?
I talked about this in an earlier message to -hackers: check_upd was
actually unused (check_ins was used for both inserts and updates).
Hm, guess I missed (or forgot) that message.
Hmm, I suppose -- if you prefer I can have check_ins called by the
INSERT trigger and check_upd called by the UPDATE trigger, which
probably makes more sense.
Yeah ... I thought it was doing that already.
regards, tom lane
On Sun, 2005-05-29 at 21:06 -0400, Tom Lane wrote:
Neil Conway <neilc@samurai.com> writes:
Hmm, I suppose -- if you prefer I can have check_ins called by the
INSERT trigger and check_upd called by the UPDATE trigger, which
probably makes more sense.Yeah ... I thought it was doing that already.
Attached are two patches: one that changes ADD FOREIGN KEY to create
separate ON INSERT and ON UPDATE triggers that invoke different trigger
functions, and a revised version of the FK UPDATE enqueuing patch.
BTW, the regression test failure was just stupidity on my part: I had
updated the "expected" results using the regression test output from
some intermediate version of the patch without checking it carefully
enough. The attached patch doesn't FK enqueuing patch doesn't cause any
unexpected regression test changes.
Barring any objections I'll apply both of these to HEAD today or
tomorrow.
-Neil
Neil Conway <neilc@samurai.com> writes:
Attached are two patches: one that changes ADD FOREIGN KEY to create
separate ON INSERT and ON UPDATE triggers that invoke different trigger
functions, and a revised version of the FK UPDATE enqueuing patch.
Looks OK to me. Don't forget that the first of these should probably
include a catversion.h bump.
regards, tom lane