Thread: Trigger for Audit Table
I'm asking for a sanity check:
This is a very simple audit table setup where I use a BEFORE UPDATE
trigger to save an existing record.
The table stores templates (for a CMS) and looks something like this:
create table template (
id SERIAL PRIMARY KEY,
path text UNIQUE NOT NULL,
content text NOT NULL,
last_updated_time timestamp(0) with time zone NOT NULL default now()
);
And then an audit table:
create table template_history (
id SERIAL PRIMARY KEY,
template_id integer NOT NULL REFERENCES template ON DELETE CASCADE,
path text NOT NULL,
content text NOT NULL,
last_updated_time timestamp(0) with time zone NOT NULL
);
(The "path" is not the primary key because the template's path
might get renamed (moved), but I still want to track its history.)
My trigger is very simple:
CREATE OR REPLACE FUNCTION audit_template() RETURNS TRIGGER AS '
BEGIN
INSERT INTO template_history
( template_id, path, content, last_updated_time, person )
select
id, path, content, last_updated_time, person
from
template where id = 1;
RETURN NEW;
END'
language 'plpgsql';
CREATE TRIGGER template_history_add BEFORE UPDATE ON template
for each row execute procedure audit_template();
I realize this is a *BEFORE* UPDATE trigger, but I have this vague
memory of seeing a post stating that you can't be sure the existing
row has not been updated yet. Perhaps that was just a concern if
another trigger was to modify the row. But, I can't seem to find that
post now which is why I'm asking for the sanity check.
Are there potential problems with this setup?
--
Bill Moseley
moseley@hank.org
Bill Moseley <moseley@hank.org> writes:
> I'm asking for a sanity check:
> And then an audit table:
> create table template_history (
> id SERIAL PRIMARY KEY,
> template_id integer NOT NULL REFERENCES template ON DELETE CASCADE,
> path text NOT NULL,
> content text NOT NULL,
> last_updated_time timestamp(0) with time zone NOT NULL
> );
Why would you want ON DELETE CASCADE? Or for that matter to have a
foreign key here at all? Surely the point of an audit table is to
remember history. If the audit entries all disappear the instant
the main-table entry is deleted, it's not much of an audit tool.
> My trigger is very simple:
> CREATE OR REPLACE FUNCTION audit_template() RETURNS TRIGGER AS '
> BEGIN
> INSERT INTO template_history
> ( template_id, path, content, last_updated_time, person )
> select
> id, path, content, last_updated_time, person
> from
> template where id = 1;
> RETURN NEW;
> END'
> language 'plpgsql';
This is not going to work because the row's not there yet (I won't
bother pointing out the thinko in the WHERE clause); and even if it did
work it'd be unnecessarily inefficient. Just use the NEW row that's
passed to the trigger:
INSERT INTO template_history(...) VALUES(NEW.id, NEW.path, ...)
If you have other BEFORE triggers on this table that can change the
NEW row, then it might be better to make this an AFTER trigger so it can
be sure the NEW row it sees won't change anymore. But AFTER triggers
are distinctly less efficient, so if you're not intending to add more
triggers then using a BEFORE trigger is probably the way to go.
regards, tom lane
On Fri, Mar 09, 2007 at 06:50:39PM -0500, Tom Lane wrote: > Bill Moseley <moseley@hank.org> writes: > > I'm asking for a sanity check: > > > And then an audit table: > > > create table template_history ( > > id SERIAL PRIMARY KEY, > > template_id integer NOT NULL REFERENCES template ON DELETE CASCADE, > > path text NOT NULL, > > content text NOT NULL, > > last_updated_time timestamp(0) with time zone NOT NULL > > ); > > Why would you want ON DELETE CASCADE? Or for that matter to have a > foreign key here at all? Surely the point of an audit table is to > remember history. If the audit entries all disappear the instant > the main-table entry is deleted, it's not much of an audit tool. In this case the templates are short lived so only want to track history while the "live" record is around. That is, it's expected that the template and all its history will get wiped out once in a while. Still, I agree with you that it would be better to use a different approach and not cascade delete. The foreign key is there so can find the history related to the primary record. That's what ties the history records together (the path can change during the life of the template). > > > My trigger is very simple: > > > CREATE OR REPLACE FUNCTION audit_template() RETURNS TRIGGER AS ' > > BEGIN > > INSERT INTO template_history > > ( template_id, path, content, last_updated_time, person ) > > select > > id, path, content, last_updated_time, person > > from > > template where id = 1; > > RETURN NEW; > > END' > > language 'plpgsql'; > > This is not going to work because the row's not there yet. This is a BEFORE *UPDATE* trigger, not a BEFORE INSERT, so the row is there. The audit table is written when the primary record changes and the old version is written to the audit table, not the new version. Yes, it's more common to write the audit for every insert and update (so the most recent version is also in the audit table). > (I won't bother pointing out the thinko in the WHERE clause); Darn cut-n-paste errors > and even if it did > work it'd be unnecessarily inefficient. Just use the NEW row that's > passed to the trigger: > > INSERT INTO template_history(...) VALUES(NEW.id, NEW.path, ...) Ok, but as the id is a sequence. I need to test if NEW.id is set after the insert -- seems like not, IIRC, and I'd need to use curval(). > If you have other BEFORE triggers on this table that can change the > NEW row, then it might be better to make this an AFTER trigger so it can > be sure the NEW row it sees won't change anymore. But AFTER triggers > are distinctly less efficient, so if you're not intending to add more > triggers then using a BEFORE trigger is probably the way to go. It's just that pesky sequence I need access to after the insert happens. Thanks for the tips, Tom. -- Bill Moseley moseley@hank.org
Bill Moseley <moseley@hank.org> writes:
> On Fri, Mar 09, 2007 at 06:50:39PM -0500, Tom Lane wrote:
>> This is not going to work because the row's not there yet.
> This is a BEFORE *UPDATE* trigger, not a BEFORE INSERT, so the row is
> there. The audit table is written when the primary record changes
> and the old version is written to the audit table, not the new
> version.
Well, if you want to write the old data, it's still a silly way to do
it: write the OLD.* tuple instead of forcing a fresh search of the
table.
> Ok, but as the id is a sequence. I need to test if NEW.id is set after
> the insert -- seems like not, IIRC, and I'd need to use curval().
You're confusing rules with triggers. In a trigger, NEW.* and OLD.*
are physical rows and you don't need to worry about multi evaluation
or anything like that.
regards, tom lane
You can/should create it as an AFTER UPDATE trigger. The OLD row will contain the previous values. eg: INSERT INTO template_history ( template_id, path, content, last_updated_time, person ) values (OLD.id, OLD.path, OLD.content, OLD.last_updated_time, OLD.person); On Mar 9, 2:45 pm, mose...@hank.org (Bill Moseley) wrote: > My trigger is very simple: > > CREATE OR REPLACE FUNCTION audit_template() RETURNS TRIGGER AS ' > BEGIN > INSERT INTO template_history > ( template_id, path, content, last_updated_time, person ) > select > id, path, content, last_updated_time, person > from > template where id = 1; > > RETURN NEW; > END' > language 'plpgsql'; > > CREATE TRIGGER template_history_add BEFORE UPDATE ON template > for each row execute procedure audit_template(); > > I realize this is a *BEFORE* UPDATE trigger, but I have this vague > memory of seeing a post stating that you can't be sure the existing > row has not been updated yet. Perhaps that was just a concern if > another trigger was to modify the row. But, I can't seem to find that > post now which is why I'm asking for the sanity check. > > Are there potential problems with this setup? > > -- > Bill Moseley > mose...@hank.org > > ---------------------------(end of broadcast)--------------------------- > TIP 5: don't forget to increase your free space map settings
On Fri, Mar 09, 2007 at 01:27:51PM -0800, ksherlock@gmail.com wrote: > You can/should create it as an AFTER UPDATE trigger. The OLD row will > contain the previous values. Curiously, also works with a BEFORE UPDATE. Off to review the docs.... -- Bill Moseley moseley@hank.org