Re: [PATCHES] skip FK trigger on UPDATE
On Sun, 2005-05-29 at 23:09 -0400, Tom Lane wrote: > Looks OK to me. Don't forget that the first of these should probably > include a catversion.h bump. Both patches applied to HEAD, catversion bumped. Thanks for the comments, Tom. -Neil ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] skip FK trigger on UPDATE
Neil Conway <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] skip FK trigger on UPDATE
On Sun, 2005-05-29 at 21:06 -0400, Tom Lane wrote:
> Neil Conway <[EMAIL PROTECTED]> 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
Index: src/backend/commands/tablecmds.c
===
RCS file: /var/lib/cvs/pgsql/src/backend/commands/tablecmds.c,v
retrieving revision 1.157
diff -c -r1.157 tablecmds.c
*** src/backend/commands/tablecmds.c 10 May 2005 13:16:26 - 1.157
--- src/backend/commands/tablecmds.c 30 May 2005 02:33:56 -
***
*** 4380,4431
pfree(trig.tgargs);
}
- /*
- * Create the triggers that implement an FK constraint.
- */
static void
! createForeignKeyTriggers(Relation rel, FkConstraint *fkconstraint,
! Oid constrOid)
{
- RangeVar *myRel;
CreateTrigStmt *fk_trigger;
ListCell *fk_attr;
ListCell *pk_attr;
- ObjectAddress trigobj,
- constrobj;
-
- /*
- * Reconstruct a RangeVar for my relation (not passed in,
- * unfortunately).
- */
- myRel = makeRangeVar(get_namespace_name(RelationGetNamespace(rel)),
- pstrdup(RelationGetRelationName(rel)));
- /*
- * Preset objectAddress fields
- */
- constrobj.classId = ConstraintRelationId;
- constrobj.objectId = constrOid;
- constrobj.objectSubId = 0;
- trigobj.classId = TriggerRelationId;
- trigobj.objectSubId = 0;
-
- /* Make changes-so-far visible */
- CommandCounterIncrement();
-
- /*
- * Build and execute a CREATE CONSTRAINT TRIGGER statement for the
- * CHECK action.
- */
fk_trigger = makeNode(CreateTrigStmt);
fk_trigger->trigname = fkconstraint->constr_name;
fk_trigger->relation = myRel;
- fk_trigger->funcname = SystemFuncName("RI_FKey_check_ins");
fk_trigger->before = false;
fk_trigger->row = true;
! fk_trigger->actions[0] = 'i';
! fk_trigger->actions[1] = 'u';
! fk_trigger->actions[2] = '\0';
fk_trigger->isconstraint = true;
fk_trigger->deferrable = fkconstraint->deferrable;
--- 4380,4412
pfree(trig.tgargs);
}
static void
! CreateFKCheckTrigger(RangeVar *myRel, FkConstraint *fkconstraint,
! ObjectAddress *constrobj, ObjectAddress *trigobj,
! bool on_insert)
{
CreateTrigStmt *fk_trigger;
ListCell *fk_attr;
ListCell *pk_attr;
fk_trigger = makeNode(CreateTrigStmt);
fk_trigger->trigname = fkconstraint->constr_name;
fk_trigger->relation = myRel;
fk_trigger->before = false;
fk_trigger->row = true;
!
! /* Either ON INSERT or ON UPDATE */
! if (on_insert)
! {
! fk_trigger->funcname = SystemFuncName("RI_FKey_check_ins");
! fk_trigger->actions[0] = 'i';
! }
! else
! {
! fk_trigger->funcname = SystemFuncName("RI_FKey_check_upd");
! fk_trigger->actions[0] = 'u';
! }
! fk_trigger->actions[1] = '\0';
fk_trigger->isconstraint = true;
fk_trigger->deferrable = fkconstraint->deferrable;
***
*** 4453,4465
fk_trigger->args = lappend(fk_trigger->args, lfirst(pk_attr));
}
! trigobj.objectId = CreateTrigger(fk_trigger, true);
/* Register dependency from trigger to constraint */
! recordDependencyOn(&trigobj, &constrobj, DEPENDENCY_INTERNAL);
/* Make changes-so-far visible */
CommandCounterIncrement();
/*
* Build and execute a CREATE CONSTRAINT TRIGGER statement for the ON
--- 4434,4487
fk_trigger->args = lappend(fk_trigger->args, lfirst(pk_attr));
}
! trigobj->objectId = CreateTrigger(fk_trigger, true);
/* Register dependency from trigger to constraint */
! recordDependencyOn(trigobj, constrobj, DEPENDENCY_INTERNAL);
/* Make changes-so-far visible */
CommandCounterIncrement();
+ }
+
+ /*
+ * Create the triggers that implement an FK constraint.
+ */
+ static void
+ createForeignKeyTriggers(Relation rel, FkConstraint *fkconstraint,
+ Oid constrOid)
+ {
+ RangeVar *myRel;
+ CreateTrigStmt *fk_trigger;
+ ListCell *fk_attr;
+ ListCell *pk_attr;
+ ObjectAddress trigobj,
+ constrobj;
+
+ /*
+ * Reconstruct a RangeVar for my relation (not passed in,
+ * unfortunately).
+ */
+ myRel = makeRangeVar(get_namespace_name(RelationGetNamespace(rel)),
+ pstrdup(RelationGetRelationName(rel)));
+
+ /*
+ * Preset objec
Re: [PATCHES] skip FK trigger on UPDATE
Neil Conway <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] skip FK trigger on UPDATE
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 ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] skip FK trigger on UPDATE
Neil Conway <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
[PATCHES] 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
Index: src/backend/commands/tablecmds.c
===
RCS file: /Users/neilc/local/cvs/pgsql/src/backend/commands/tablecmds.c,v
retrieving revision 1.157
diff -c -r1.157 tablecmds.c
*** src/backend/commands/tablecmds.c 10 May 2005 13:16:26 - 1.157
--- src/backend/commands/tablecmds.c 27 May 2005 13:58:54 -
***
*** 1596,1603
case F_RI_FKEY_NOACTION_UPD:
return RI_TRIGGER_PK;
! case F_RI_FKEY_CHECK_INS:
! case F_RI_FKEY_CHECK_UPD:
return RI_TRIGGER_FK;
}
--- 1596,1602
case F_RI_FKEY_NOACTION_UPD:
return RI_TRIGGER_PK;
! case F_RI_FKEY_CHECK:
return RI_TRIGGER_FK;
}
***
*** 4305,4313
return;
/*
! * Scan through each tuple, calling RI_FKey_check_ins (insert trigger)
! * as if that tuple had just been inserted. If any of those fail, it
! * should ereport(ERROR) and that's that.
*/
MemSet(&trig, 0, sizeof(trig));
trig.tgoid = InvalidOid;
--- 4304,4312
return;
/*
! * Scan through each tuple, calling RI_FKey_check (insert trigger)
! * as if that tuple had just been inserted. If any of those fail,
! * it should ereport(ERROR) and that's that.
*/
MemSet(&trig, 0, sizeof(trig));
trig.tgoid = InvalidOid;
***
*** 4359,4365
MemSet(&fcinfo, 0, sizeof(fcinfo));
/*
! * We assume RI_FKey_check_ins won't look at flinfo...
*/
trigdata.type = T_TriggerData;
trigdata.tg_event = TRIGGER_EVENT_INSERT | TRIGGER_EVENT_ROW;
--- 4358,4364
MemSet(&fcinfo, 0, sizeof(fcinfo));
/*
! * We assume RI_FKey_check won't look at flinfo...
*/
trigdata.type = T_TriggerData;
trigdata.tg_event = TRIGGER_EVENT_INSERT | TRIGGER_EVENT_ROW;
***
*** 4372,4378
fcinfo.context = (Node *) &trigdata;
! RI_FKey_check_ins(&fcinfo);
}
heap_endscan(scan);
--- 4371,4377
fcinfo.context = (Node *) &trigdata;
! RI_FKey_check(&fcinfo);
}
heap_endscan(scan);
***
*** 4420,4426
fk_trigger = makeNode(CreateTrigStmt);
fk_trigger->trigname = fkconstraint->constr_name;
fk_trigger->relation = myRel;
! fk_trigger->funcname = SystemFuncName("RI_FKey_check_ins");
fk_trigger->before = false;
fk_trigger->row = true;
fk_trigger->actions[0] = 'i';
--- 4419,4425
fk_trigger = makeNode(CreateTrigStmt);
fk_trigger->trigname = fkconstraint->constr_name;
fk_trigger->relation = myRel;
! fk_trigger->funcname = SystemFuncName("RI_FKey_check");
fk_trigger->before = false;
fk_trigger->row = true;
fk_trigger->actions[0] = 'i';
Index: src/backend/commands/trigger.c
===
RCS file: /Users/neilc/loc
