Thread: minimal update
For some time I have been working on removing some inefficiencies from a large DW-type app. This app does a large daily batch update, and this is what is the major bottleneck. One of the things I have been doing is to remove unnecessary updates (which are particualrly expensive in our index-rich setting). Several times now I have wished that there was a switch on the UPDATE command that said "do minimal instead of maximal updating". i.e., don't update records with identical replacements. At the moment I have to write things like: update tname set foo = bar ... where foo is null or foo <> bar ... This becomes more than tedious when the update might be setting thirty or forty fields, and I have to write such tests for each of them. It would be so much nicer to be able to write something like: update tname minimally set foo = bar ... Is this an insane idea, or would it be possible, practical and useful? cheers andrew
On Fri, Nov 02, 2007 at 11:49:38AM -0400, Andrew Dunstan wrote: > For some time I have been working on removing some inefficiencies > from a large DW-type app. This app does a large daily batch update, > and this is what is the major bottleneck. One of the things I have > been doing is to remove unnecessary updates (which are particualrly > expensive in our index-rich setting). Several times now I have > wished that there was a switch on the UPDATE command that said "do > minimal instead of maximal updating". i.e., don't update records > with identical replacements. At the moment I have to write things > like: > > update tname set foo = bar ... where foo is null or foo <> bar > ... One way I've done this is make RULEs which basically drop non-updating "UPDATEs" on the floor. CREATE RULE foo_drop_empty_updates AS ON UPDATE TO foo WHERE ROW(OLD.*)::foo IS NOT DISTINCT FROM ROW(NEW.*)::foo DOINSTEAD NOTHING; It's pretty easy to automate rule creation, but since Postgres doesn't have DDL triggers, it's also a bit of a foot gun. By the way, the above has what I think of as an infelicity in 8.2.5, namely that you need non-obvious contortions to get it to work. I'm thinking OLD IS NOT DISTINCT FROM NEW should Just Work(TM). > This becomes more than tedious when the update might be setting thirty > or forty fields, and I have to write such tests for each of them. It > would be so much nicer to be able to write something like: > > update tname minimally set foo = bar ... > > Is this an insane idea, or would it be possible, practical and useful? I don't know about the sanity, but I've done it a couple of places :) Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
David Fetter <david@fetter.org> writes:
> On Fri, Nov 02, 2007 at 11:49:38AM -0400, Andrew Dunstan wrote:
>> At the moment I have to write things like:
>>
>> update tname set foo = bar ... where foo is null or foo <> bar
> One way I've done this is make RULEs which basically drop non-updating
> "UPDATEs" on the floor.
A BEFORE UPDATE trigger would be better, and probably hardly more
expensive than a wired-in facility (especially if you were willing to
write it in C).
regards, tom lane
Tom Lane wrote: > David Fetter <david@fetter.org> writes: > >> On Fri, Nov 02, 2007 at 11:49:38AM -0400, Andrew Dunstan wrote: >> >>> At the moment I have to write things like: >>> >>> update tname set foo = bar ... where foo is null or foo <> bar >>> > > >> One way I've done this is make RULEs which basically drop non-updating >> "UPDATEs" on the floor. >> > > A BEFORE UPDATE trigger would be better, and probably hardly more > expensive than a wired-in facility (especially if you were willing to > write it in C). > > > Yes. I also prefer the trigger idea to a rule because triggers are easy to enable and disable. It's still a lot of work for what must be a common want, though. Could it be done generically? cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> Tom Lane wrote:
>> A BEFORE UPDATE trigger would be better, and probably hardly more
>> expensive than a wired-in facility (especially if you were willing to
>> write it in C).
> Yes. I also prefer the trigger idea to a rule because triggers are easy
> to enable and disable. It's still a lot of work for what must be a
> common want, though. Could it be done generically?
Well, you could write the trigger in C and it'd work for any table.
I think it could be as simple as a memcmp of the tuples' data areas,
since we now require padding bytes to be 0 ...
regards, tom lane
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > >> Tom Lane wrote: >> >>> A BEFORE UPDATE trigger would be better, and probably hardly more >>> expensive than a wired-in facility (especially if you were willing to >>> write it in C). >>> > > >> Yes. I also prefer the trigger idea to a rule because triggers are easy >> to enable and disable. It's still a lot of work for what must be a >> common want, though. Could it be done generically? >> > > Well, you could write the trigger in C and it'd work for any table. > I think it could be as simple as a memcmp of the tuples' data areas, > since we now require padding bytes to be 0 ... > > > Ah. Good. Thanks, that's the piece I was missing. cheers andrew
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > >> Tom Lane wrote: >> >>> A BEFORE UPDATE trigger would be better, and probably hardly more >>> expensive than a wired-in facility (especially if you were willing to >>> write it in C). >>> > > >> Yes. I also prefer the trigger idea to a rule because triggers are easy >> to enable and disable. It's still a lot of work for what must be a >> common want, though. Could it be done generically? >> > > Well, you could write the trigger in C and it'd work for any table. > I think it could be as simple as a memcmp of the tuples' data areas, > since we now require padding bytes to be 0 ... > > > Something like this fragment? newtuple = trigdata->tg_newtuple; oldtuple = trigdata->tg_trigtuple; rettuple = newtuple; if (newtuple->t_len == oldtuple->t_len && newtuple->t_data->t_hoff == oldtuple->t_data->t_hoff && memcmp(GETSTRUCT(newtuple),GETSTRUCT(oldtuple), newtuple->t_len - newtuple->t_data->t_hoff) == 0) rettuple =NULL; return PointerGetDatum(rettuple); Also, when did we first require padding bytes to be 0? cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> Tom Lane wrote:
>> Well, you could write the trigger in C and it'd work for any table.
>> I think it could be as simple as a memcmp of the tuples' data areas,
>> since we now require padding bytes to be 0 ...
> Something like this fragment?
> newtuple = trigdata->tg_newtuple;
> oldtuple = trigdata->tg_trigtuple;
> rettuple = newtuple;
> if (newtuple->t_len == oldtuple->t_len &&
> newtuple->t_data->t_hoff == oldtuple->t_data->t_hoff &&
> memcmp(GETSTRUCT(newtuple),GETSTRUCT(oldtuple),
> newtuple->t_len - newtuple->t_data->t_hoff) == 0)
> rettuple = NULL;
> return PointerGetDatum(rettuple);
Close, but I think you also need to take care to compare natts and
the null bitmaps (if any). Might be worth comparing OIDs too, though
AFAIR there is no mechanism for substituting a different OID during
UPDATE. Probably the easiest coding is to memcmp all the way from
offsetof(t_bits) to t_len, after comparing natts and the HASNULL and
HASOID flags.
> Also, when did we first require padding bytes to be 0?
The 8.3 varvarlena patch is what requires it, but in practice
heap_formtuple has always started with a palloc0, so I think it would
work a long ways back.
regards, tom lane
On Nov 2, 2007, at 13:44 , Andrew Dunstan wrote: > Ah. Good. Thanks, that's the piece I was missing. What would be the disadvantages of always doing this, i.e., just making this part of the normal update path in the backend? I'd think it should save on unnecessarily dead tuples as well. Michael Glaesemann grzm seespotcode net
Michael Glaesemann <grzm@seespotcode.net> writes:
> What would be the disadvantages of always doing this, i.e., just
> making this part of the normal update path in the backend?
(1) cycles wasted to no purpose in the vast majority of cases.
(2) visibly inconsistent behavior for apps that pay attention
to ctid/xmin/etc.
(3) visibly inconsistent behavior for apps that have AFTER triggers.
There's enough other overhead in issuing an update (network,
parsing/planning/etc) that a sanely coded application should try
to avoid issuing no-op updates anyway. The proposed trigger is
just a band-aid IMHO.
I think having it as an optional trigger is a reasonable compromise.
regards, tom lane
Tom Lane wrote: > Michael Glaesemann <grzm@seespotcode.net> writes: > >> What would be the disadvantages of always doing this, i.e., just >> making this part of the normal update path in the backend? >> > > (1) cycles wasted to no purpose in the vast majority of cases. > > (2) visibly inconsistent behavior for apps that pay attention > to ctid/xmin/etc. > > (3) visibly inconsistent behavior for apps that have AFTER triggers. > > There's enough other overhead in issuing an update (network, > parsing/planning/etc) that a sanely coded application should try > to avoid issuing no-op updates anyway. The proposed trigger is > just a band-aid IMHO. > > I think having it as an optional trigger is a reasonable compromise. > > > Right. I never proposed making this the default behaviour, for all these good reasons. The point about making the app try to avoid no-op updates is that this can impose some quite considerable code complexity on the app, especially where the number of updated fields is large. It's fragile and error-prone. A simple switch that can turn a trigger on or off will be nicer. Syntax support for that might be even nicer, but there appears to be some resistance to that, so I can easily settle for the trigger. cheers andrew
On Nov 8, 2007, at 10:46 , Andrew Dunstan wrote: > > > Tom Lane wrote: >> Michael Glaesemann <grzm@seespotcode.net> writes: >> >>> What would be the disadvantages of always doing this, i.e., just >>> making this part of the normal update path in the backend? >>> >> >> (1) cycles wasted to no purpose in the vast majority of cases. >> >> (2) visibly inconsistent behavior for apps that pay attention >> to ctid/xmin/etc. >> >> (3) visibly inconsistent behavior for apps that have AFTER triggers. >> >> There's enough other overhead in issuing an update (network, >> parsing/planning/etc) that a sanely coded application should try >> to avoid issuing no-op updates anyway. The proposed trigger is >> just a band-aid IMHO. >> >> I think having it as an optional trigger is a reasonable compromise. >> >> >> > > Right. I never proposed making this the default behaviour, for all > these good reasons. > > The point about making the app try to avoid no-op updates is that > this can impose some quite considerable code complexity on the app, > especially where the number of updated fields is large. It's > fragile and error-prone. A simple switch that can turn a trigger on > or off will be nicer. Syntax support for that might be even nicer, > but there appears to be some resistance to that, so I can easily > settle for the trigger. This confirms what I thought. Thanks. Michael Glaesemann grzm seespotcode net
On Nov 2, 2007, at 10:49 AM, Andrew Dunstan wrote: > update tname set foo = bar ... where foo is null or foo <> bar ... FYI, you should be able to do WHERE foo IS DISTINCT FROM bar instead. -- Decibel!, aka Jim C. Nasby, Database Architect decibel@decibel.org Give your computer some brain candy! www.distributed.net Team #1828
Decibel! wrote: > On Nov 2, 2007, at 10:49 AM, Andrew Dunstan wrote: >> update tname set foo = bar ... where foo is null or foo <> bar ... > > FYI, you should be able to do WHERE foo IS DISTINCT FROM bar instead. True, that's a bit nicer. It's still more than somewhat ugly and fragile if there a lot of foos and the bars are complex expressions. cheers andrew
Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>
>> Tom Lane wrote:
>>
>>> Well, you could write the trigger in C and it'd work for any table.
>>> I think it could be as simple as a memcmp of the tuples' data areas,
>>> since we now require padding bytes to be 0 ...
>>>
>
>
>> Something like this fragment?
>>
>
>
>> newtuple = trigdata->tg_newtuple;
>> oldtuple = trigdata->tg_trigtuple;
>> rettuple = newtuple;
>>
>
>
>> if (newtuple->t_len == oldtuple->t_len &&
>> newtuple->t_data->t_hoff == oldtuple->t_data->t_hoff &&
>> memcmp(GETSTRUCT(newtuple),GETSTRUCT(oldtuple),
>> newtuple->t_len - newtuple->t_data->t_hoff) == 0)
>> rettuple = NULL;
>>
>
>
>> return PointerGetDatum(rettuple);
>>
>
> Close, but I think you also need to take care to compare natts and
> the null bitmaps (if any). Might be worth comparing OIDs too, though
> AFAIR there is no mechanism for substituting a different OID during
> UPDATE. Probably the easiest coding is to memcmp all the way from
> offsetof(t_bits) to t_len, after comparing natts and the HASNULL and
> HASOID flags.
>
How does this look?
if (newtuple->t_len == oldtuple->t_len && newtuple->t_data->t_hoff == oldtuple->t_data->t_hoff &&
HeapTupleHeaderGetNatts(newtuple)== HeapTupleHeaderGetNatts(oldtuple) && (newtuple->t_data->t_infomask &
(HEAP_HASOID|HEAP_HASNULL))== (oldtuple->t_data->t_infomask & (HEAP_HASOID|HEAP_HASNULL)) &&
memcmp(newtuple->t_data+ offsetof(HeapTupleHeaderData, t_bits), oldtuple->t_data +
offsetof(HeapTupleHeaderData,t_bits) newtuple->t_len - offsetof(HeapTupleHeaderData, t_bits)) == 0)
rettuple = NULL;
return PointerGetDatum(rettuple);
cheers
andrew
>
>> Also, when did we first require padding bytes to be 0?
>>
>
> The 8.3 varvarlena patch is what requires it, but in practice
> heap_formtuple has always started with a palloc0, so I think it would
> work a long ways back.
>
> regards, tom lane
>
>
Andrew Dunstan <andrew@dunslane.net> writes:
> How does this look?
> if (newtuple->t_len == oldtuple->t_len &&
> newtuple->t_data->t_hoff == oldtuple->t_data->t_hoff &&
> HeapTupleHeaderGetNatts(newtuple) == HeapTupleHeaderGetNatts(oldtuple) &&
> (newtuple->t_data->t_infomask & (HEAP_HASOID|HEAP_HASNULL)) == (oldtuple->t_data->t_infomask &
(HEAP_HASOID|HEAP_HASNULL))&&
> memcmp(newtuple->t_data + offsetof(HeapTupleHeaderData, t_bits),
> oldtuple->t_data + offsetof(HeapTupleHeaderData, t_bits)
> newtuple->t_len - offsetof(HeapTupleHeaderData, t_bits)) == 0)
> rettuple = NULL;
Looks sane. It might be even saner if you compare all of the
non-visibility-related infomask bits, viz
(newtuple->t_data->t_infomask & ~HEAP_XACT_MASK) ==(oldtuple->t_data->t_infomask & ~HEAP_XACT_MASK)
rather than just HASOID and HASNULL.
regards, tom lane
Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
>
>> How does this look?
>>
>
>
>> if (newtuple->t_len == oldtuple->t_len &&
>> newtuple->t_data->t_hoff == oldtuple->t_data->t_hoff &&
>> HeapTupleHeaderGetNatts(newtuple) == HeapTupleHeaderGetNatts(oldtuple) &&
>> (newtuple->t_data->t_infomask & (HEAP_HASOID|HEAP_HASNULL)) == (oldtuple->t_data->t_infomask &
(HEAP_HASOID|HEAP_HASNULL))&&
>> memcmp(newtuple->t_data + offsetof(HeapTupleHeaderData, t_bits),
>> oldtuple->t_data + offsetof(HeapTupleHeaderData, t_bits)
>> newtuple->t_len - offsetof(HeapTupleHeaderData, t_bits)) == 0)
>>
>
>
>> rettuple = NULL;
>>
>
> Looks sane. It might be even saner if you compare all of the
> non-visibility-related infomask bits, viz
>
> (newtuple->t_data->t_infomask & ~HEAP_XACT_MASK) ==
> (oldtuple->t_data->t_infomask & ~HEAP_XACT_MASK)
>
> rather than just HASOID and HASNULL.
>
>
>
Sadly, the memcmp is failing on my test ("update foo set bar = bar") on
8.2. Looks like I'm in for weekend with my fave debugger :-(
cheers
andrew
Andrew Dunstan wrote:
>
>
> Tom Lane wrote:
>> Andrew Dunstan <andrew@dunslane.net> writes:
>>
>>> How does this look?
>>>
>>
>>
>>> if (newtuple->t_len == oldtuple->t_len &&
>>> newtuple->t_data->t_hoff == oldtuple->t_data->t_hoff &&
>>> HeapTupleHeaderGetNatts(newtuple) ==
>>> HeapTupleHeaderGetNatts(oldtuple) &&
>>> (newtuple->t_data->t_infomask & (HEAP_HASOID|HEAP_HASNULL))
>>> == (oldtuple->t_data->t_infomask & (HEAP_HASOID|HEAP_HASNULL)) &&
>>> memcmp(newtuple->t_data + offsetof(HeapTupleHeaderData,
>>> t_bits),
>>> oldtuple->t_data + offsetof(HeapTupleHeaderData, t_bits)
>>> newtuple->t_len - offsetof(HeapTupleHeaderData,
>>> t_bits)) == 0)
>>>
>>
>>
>>> rettuple = NULL;
>>>
>>
>> Looks sane. It might be even saner if you compare all of the
>> non-visibility-related infomask bits, viz
>>
>> (newtuple->t_data->t_infomask & ~HEAP_XACT_MASK) ==
>> (oldtuple->t_data->t_infomask & ~HEAP_XACT_MASK)
>>
>> rather than just HASOID and HASNULL.
>>
>>
>>
>
> Sadly, the memcmp is failing on my test ("update foo set bar = bar")
> on 8.2. Looks like I'm in for weekend with my fave debugger :-(
>
>
Turns out we needed those pointers used in the arguments to memcmp cast
to char * so the pointer arithmetic would work right.
I'll be suggesting we add a utility function like this for 8.4.
cheers
andrew
I assume don't want a TODO for this? (Suppress UPDATE no changed columns) --------------------------------------------------------------------------- Andrew Dunstan wrote: > > > Tom Lane wrote: > > Michael Glaesemann <grzm@seespotcode.net> writes: > > > >> What would be the disadvantages of always doing this, i.e., just > >> making this part of the normal update path in the backend? > >> > > > > (1) cycles wasted to no purpose in the vast majority of cases. > > > > (2) visibly inconsistent behavior for apps that pay attention > > to ctid/xmin/etc. > > > > (3) visibly inconsistent behavior for apps that have AFTER triggers. > > > > There's enough other overhead in issuing an update (network, > > parsing/planning/etc) that a sanely coded application should try > > to avoid issuing no-op updates anyway. The proposed trigger is > > just a band-aid IMHO. > > > > I think having it as an optional trigger is a reasonable compromise. > > > > > > > > Right. I never proposed making this the default behaviour, for all these > good reasons. > > The point about making the app try to avoid no-op updates is that this > can impose some quite considerable code complexity on the app, > especially where the number of updated fields is large. It's fragile and > error-prone. A simple switch that can turn a trigger on or off will be > nicer. Syntax support for that might be even nicer, but there appears to > be some resistance to that, so I can easily settle for the trigger. > > cheers > > andrew > > ---------------------------(end of broadcast)--------------------------- > TIP 4: Have you searched our list archives? > > http://archives.postgresql.org -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://postgres.enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
On Fri, Mar 7, 2008 at 9:40 PM, Bruce Momjian <bruce@momjian.us> wrote:
I am starting to implement this. Do we want to have this trigger function in the server, or in an external module?
Best regards,
--
gurjeet[.singh]@EnterpriseDB.com
singh.gurjeet@{ gmail | hotmail | indiatimes | yahoo }.com
EnterpriseDB http://www.enterprisedb.com
17° 29' 34.37"N, 78° 30' 59.76"E - Hyderabad *
18° 32' 57.25"N, 73° 56' 25.42"E - Pune
37° 47' 19.72"N, 122° 24' 1.69" W - San Francisco
http://gurjeet.frihost.net
Mail sent from my BlackLaptop device
I assume don't want a TODO for this? (Suppress UPDATE no changed
columns)
I am starting to implement this. Do we want to have this trigger function in the server, or in an external module?
Best regards,
-----------------------------------------------------------------------------
Andrew Dunstan wrote:
>
>
> Tom Lane wrote:
> > Michael Glaesemann <grzm@seespotcode.net> writes:
> >
> >> What would be the disadvantages of always doing this, i.e., just
> >> making this part of the normal update path in the backend?
> >>
> >
> > (1) cycles wasted to no purpose in the vast majority of cases.
> >
> > (2) visibly inconsistent behavior for apps that pay attention
> > to ctid/xmin/etc.
> >
> > (3) visibly inconsistent behavior for apps that have AFTER triggers.
> >
> > There's enough other overhead in issuing an update (network,
> > parsing/planning/etc) that a sanely coded application should try
> > to avoid issuing no-op updates anyway. The proposed trigger is
> > just a band-aid IMHO.
> >
> > I think having it as an optional trigger is a reasonable compromise.
> >
> >
> >
>
> Right. I never proposed making this the default behaviour, for all these
> good reasons.
>
> The point about making the app try to avoid no-op updates is that this
> can impose some quite considerable code complexity on the app,
> especially where the number of updated fields is large. It's fragile and
> error-prone. A simple switch that can turn a trigger on or off will be
> nicer. Syntax support for that might be even nicer, but there appears to
> be some resistance to that, so I can easily settle for the trigger.
>
> cheers
>
> andrew
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Have you searched our list archives?
>
> http://archives.postgresql.org
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
gurjeet[.singh]@EnterpriseDB.com
singh.gurjeet@{ gmail | hotmail | indiatimes | yahoo }.com
EnterpriseDB http://www.enterprisedb.com
17° 29' 34.37"N, 78° 30' 59.76"E - Hyderabad *
18° 32' 57.25"N, 73° 56' 25.42"E - Pune
37° 47' 19.72"N, 122° 24' 1.69" W - San Francisco
http://gurjeet.frihost.net
Mail sent from my BlackLaptop device
Gurjeet Singh wrote: > On Fri, Mar 7, 2008 at 9:40 PM, Bruce Momjian <bruce@momjian.us > <mailto:bruce@momjian.us>> wrote: > > > I assume don't want a TODO for this? (Suppress UPDATE no changed > columns) > > > I am starting to implement this. Do we want to have this trigger > function in the server, or in an external module? > > I have the trigger part of this done, in fact. What remains to be done is to add it to the catalog and document it. The intention is to make it a builtin as it will be generally useful. If you want to work on the remaining parts then I will happily ship you the C code for the trigger. cheers andrew
On Tue, Mar 18, 2008 at 7:46 PM, Andrew Dunstan <<a href="mailto:andrew@dunslane.net">andrew@dunslane.net</a>>
wrote:<br/><div class="gmail_quote"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204);
margin:0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div class="Ih2E3d"><br /><br /><br /><br /><br /> Gurjeet Singh
wrote:<br/> > On Fri, Mar 7, 2008 at 9:40 PM, Bruce Momjian <<a
href="mailto:bruce@momjian.us">bruce@momjian.us</a><br/></div><div class="Ih2E3d">> <mailto:<a
href="mailto:bruce@momjian.us">bruce@momjian.us</a>>>wrote:<br /> ><br /> ><br /> > I assume don't
wanta TODO for this? (Suppress UPDATE no changed<br /> > columns)<br /> ><br /> ><br /> > I am
startingto implement this. Do we want to have this trigger<br /> > function in the server, or in an external
module?<br/> ><br /> ><br /><br /></div>I have the trigger part of this done, in fact. What remains to be done<br
/>is to add it to the catalog and document it. The intention is to make it<br /> a builtin as it will be generally
useful.If you want to work on the<br /> remaining parts then I will happily ship you the C code for the trigger.<br
/><br/></blockquote></div><br />In fact, I just finished writing the C code and including it in the catalog (Just
testedthat it's visible in the catalog). I will test it to see if it does actually do what we want it to.<br /><br />I
haveincorporated all the suggestions above. Would love to see your code in the meantime.<br /><br />Here's the C
code:<br/><br /><span style="font-family: courier new,monospace;">Datum<br />trig_ignore_duplicate_updates(
PG_FUNCTION_ARGS)<br /> {<br /> TriggerData *trigData;<br /> HeapTuple oldTuple;<br /> HeapTuple newTuple;<br
/><br/> if (!CALLED_AS_TRIGGER(fcinfo))<br /> elog(ERROR, "trig_ignore_duplicate_updates: not called by
triggermanager.");<br /><br /> if( !TRIGGER_FIRED_BY_UPDATE(trigData->tg_event)<br /> &&
!TRIGGER_FIRED_BEFORE(trigData->tg_event)<br/> && !TRIGGER_FIRED_FOR_ROW(trigData->tg_event) )<br
/> {<br /> elog(ERROR, "trig_ignore_duplicate_updates: Can only be executed for UPDATE, BEFORE and FOR EACH
ROW.");<br/> }<br /><br /> trigData = (TriggerData *) fcinfo->context;<br /> oldTuple =
trigData->tg_trigtuple;<br/> newTuple = trigData->tg_newtuple;<br /><br /> if (newTuple->t_len ==
oldTuple->t_len<br/> && newTuple->t_data->t_hoff == oldTuple->t_data->t_hoff<br />
&& HeapTupleHeaderGetNatts(newTuple->t_data) == HeapTupleHeaderGetNatts(oldTuple->t_data)<br />
&&(newTuple->t_data->t_infomask & ~HEAP_XACT_MASK)<br /> ==
(oldTuple->t_data->t_infomask& ~HEAP_XACT_MASK)<br /> && memcmp( (char*)(newTuple->t_data)
+offsetof(HeapTupleHeaderData, t_bits),<br /> (char*)(oldTuple->t_data) +
offsetof(HeapTupleHeaderData,t_bits),<br /> newTuple->t_len - offsetof(HeapTupleHeaderData,
t_bits)) == 0 )<br /> {<br /> /* return without crating a new tuple */<br /> return PointerGetDatum(
NULL);<br /> }<br /> <br /> return PointerGetDatum( trigData->tg_newtuple );<br />}<br /></span><span
style="font-family:courier new,monospace;"></span><br style="font-family: courier new,monospace;" /><br clear="all"
/><br/>-- <br />gurjeet[.singh]@EnterpriseDB.com<br />singh.gurjeet@{ gmail | hotmail | indiatimes | yahoo }.com<br
/><br/>EnterpriseDB <a href="http://www.enterprisedb.com">http://www.enterprisedb.com</a><br /><br />17° 29' 34.37"N,
78°30' 59.76"E - Hyderabad *<br /> 18° 32' 57.25"N, 73° 56' 25.42"E - Pune<br />37° 47' 19.72"N, 122° 24' 1.69" W - San
Francisco<br/><br /><a href="http://gurjeet.frihost.net">http://gurjeet.frihost.net</a><br /><br />Mail sent from my
BlackLaptopdevice
Is there a version of this patch ready for application?
---------------------------------------------------------------------------
Gurjeet Singh wrote:
> On Tue, Mar 18, 2008 at 7:46 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
>
> >
> >
> >
> >
> >
> > Gurjeet Singh wrote:
> > > On Fri, Mar 7, 2008 at 9:40 PM, Bruce Momjian <bruce@momjian.us
> > > <mailto:bruce@momjian.us>> wrote:
> > >
> > >
> > > I assume don't want a TODO for this? (Suppress UPDATE no changed
> > > columns)
> > >
> > >
> > > I am starting to implement this. Do we want to have this trigger
> > > function in the server, or in an external module?
> > >
> > >
> >
> > I have the trigger part of this done, in fact. What remains to be done
> > is to add it to the catalog and document it. The intention is to make it
> > a builtin as it will be generally useful. If you want to work on the
> > remaining parts then I will happily ship you the C code for the trigger.
> >
> >
> In fact, I just finished writing the C code and including it in the catalog
> (Just tested that it's visible in the catalog). I will test it to see if it
> does actually do what we want it to.
>
> I have incorporated all the suggestions above. Would love to see your code
> in the meantime.
>
> Here's the C code:
>
> Datum
> trig_ignore_duplicate_updates( PG_FUNCTION_ARGS )
> {
> TriggerData *trigData;
> HeapTuple oldTuple;
> HeapTuple newTuple;
>
> if (!CALLED_AS_TRIGGER(fcinfo))
> elog(ERROR, "trig_ignore_duplicate_updates: not called by trigger
> manager.");
>
> if( !TRIGGER_FIRED_BY_UPDATE(trigData->tg_event)
> && !TRIGGER_FIRED_BEFORE(trigData->tg_event)
> && !TRIGGER_FIRED_FOR_ROW(trigData->tg_event) )
> {
> elog(ERROR, "trig_ignore_duplicate_updates: Can only be executed for
> UPDATE, BEFORE and FOR EACH ROW.");
> }
>
> trigData = (TriggerData *) fcinfo->context;
> oldTuple = trigData->tg_trigtuple;
> newTuple = trigData->tg_newtuple;
>
> if (newTuple->t_len == oldTuple->t_len
> && newTuple->t_data->t_hoff == oldTuple->t_data->t_hoff
> && HeapTupleHeaderGetNatts(newTuple->t_data) ==
> HeapTupleHeaderGetNatts(oldTuple->t_data)
> && (newTuple->t_data->t_infomask & ~HEAP_XACT_MASK)
> == (oldTuple->t_data->t_infomask & ~HEAP_XACT_MASK)
> && memcmp( (char*)(newTuple->t_data) + offsetof(HeapTupleHeaderData,
> t_bits),
> (char*)(oldTuple->t_data) + offsetof(HeapTupleHeaderData,
> t_bits),
> newTuple->t_len - offsetof(HeapTupleHeaderData, t_bits)
> ) == 0 )
> {
> /* return without crating a new tuple */
> return PointerGetDatum( NULL );
> }
>
> return PointerGetDatum( trigData->tg_newtuple );
> }
>
>
>
> --
> gurjeet[.singh]@EnterpriseDB.com
> singh.gurjeet@{ gmail | hotmail | indiatimes | yahoo }.com
>
> EnterpriseDB http://www.enterprisedb.com
>
> 17? 29' 34.37"N, 78? 30' 59.76"E - Hyderabad *
> 18? 32' 57.25"N, 73? 56' 25.42"E - Pune
> 37? 47' 19.72"N, 122? 24' 1.69" W - San Francisco
>
> http://gurjeet.frihost.net
>
> Mail sent from my BlackLaptop device
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Not that I know of. I never saw Gurjeet's completed code.
cheers
andrew
Bruce Momjian wrote:
> Is there a version of this patch ready for application?
>
> ---------------------------------------------------------------------------
>
> Gurjeet Singh wrote:
>
>> On Tue, Mar 18, 2008 at 7:46 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
>>
>>
>>>
>>>
>>>
>>> Gurjeet Singh wrote:
>>>
>>>> On Fri, Mar 7, 2008 at 9:40 PM, Bruce Momjian <bruce@momjian.us
>>>> <mailto:bruce@momjian.us>> wrote:
>>>>
>>>>
>>>> I assume don't want a TODO for this? (Suppress UPDATE no changed
>>>> columns)
>>>>
>>>>
>>>> I am starting to implement this. Do we want to have this trigger
>>>> function in the server, or in an external module?
>>>>
>>>>
>>>>
>>> I have the trigger part of this done, in fact. What remains to be done
>>> is to add it to the catalog and document it. The intention is to make it
>>> a builtin as it will be generally useful. If you want to work on the
>>> remaining parts then I will happily ship you the C code for the trigger.
>>>
>>>
>>>
>> In fact, I just finished writing the C code and including it in the catalog
>> (Just tested that it's visible in the catalog). I will test it to see if it
>> does actually do what we want it to.
>>
>> I have incorporated all the suggestions above. Would love to see your code
>> in the meantime.
>>
>> Here's the C code:
>>
>> Datum
>> trig_ignore_duplicate_updates( PG_FUNCTION_ARGS )
>> {
>> TriggerData *trigData;
>> HeapTuple oldTuple;
>> HeapTuple newTuple;
>>
>> if (!CALLED_AS_TRIGGER(fcinfo))
>> elog(ERROR, "trig_ignore_duplicate_updates: not called by trigger
>> manager.");
>>
>> if( !TRIGGER_FIRED_BY_UPDATE(trigData->tg_event)
>> && !TRIGGER_FIRED_BEFORE(trigData->tg_event)
>> && !TRIGGER_FIRED_FOR_ROW(trigData->tg_event) )
>> {
>> elog(ERROR, "trig_ignore_duplicate_updates: Can only be executed for
>> UPDATE, BEFORE and FOR EACH ROW.");
>> }
>>
>> trigData = (TriggerData *) fcinfo->context;
>> oldTuple = trigData->tg_trigtuple;
>> newTuple = trigData->tg_newtuple;
>>
>> if (newTuple->t_len == oldTuple->t_len
>> && newTuple->t_data->t_hoff == oldTuple->t_data->t_hoff
>> && HeapTupleHeaderGetNatts(newTuple->t_data) ==
>> HeapTupleHeaderGetNatts(oldTuple->t_data)
>> && (newTuple->t_data->t_infomask & ~HEAP_XACT_MASK)
>> == (oldTuple->t_data->t_infomask & ~HEAP_XACT_MASK)
>> && memcmp( (char*)(newTuple->t_data) + offsetof(HeapTupleHeaderData,
>> t_bits),
>> (char*)(oldTuple->t_data) + offsetof(HeapTupleHeaderData,
>> t_bits),
>> newTuple->t_len - offsetof(HeapTupleHeaderData, t_bits)
>> ) == 0 )
>> {
>> /* return without crating a new tuple */
>> return PointerGetDatum( NULL );
>> }
>>
>> return PointerGetDatum( trigData->tg_newtuple );
>> }
>>
>>
>>
>> --
>> gurjeet[.singh]@EnterpriseDB.com
>> singh.gurjeet@{ gmail | hotmail | indiatimes | yahoo }.com
>>
>> EnterpriseDB http://www.enterprisedb.com
>>
>> 17? 29' 34.37"N, 78? 30' 59.76"E - Hyderabad *
>> 18? 32' 57.25"N, 73? 56' 25.42"E - Pune
>> 37? 47' 19.72"N, 122? 24' 1.69" W - San Francisco
>>
>> http://gurjeet.frihost.net
>>
>> Mail sent from my BlackLaptop device
>>
>
>
Andrew Dunstan wrote: > > Not that I know of. I never saw Gurjeet's completed code. This is Gurjeet's code, but it is not complete. http://archives.postgresql.org/pgsql-hackers/2008-03/msg00668.php --------------------------------------------------------------------------- > > cheers > > andrew > > Bruce Momjian wrote: > > Is there a version of this patch ready for application? > > > > --------------------------------------------------------------------------- > > > > Gurjeet Singh wrote: > > > >> On Tue, Mar 18, 2008 at 7:46 PM, Andrew Dunstan <andrew@dunslane.net> wrote: > >> > >> > >>> > >>> > >>> > >>> Gurjeet Singh wrote: > >>> > >>>> On Fri, Mar 7, 2008 at 9:40 PM, Bruce Momjian <bruce@momjian.us > >>>> <mailto:bruce@momjian.us>> wrote: > >>>> > >>>> > >>>> I assume don't want a TODO for this? (Suppress UPDATE no changed > >>>> columns) > >>>> > >>>> > >>>> I am starting to implement this. Do we want to have this trigger > >>>> function in the server, or in an external module? > >>>> > >>>> > >>>> > >>> I have the trigger part of this done, in fact. What remains to be done > >>> is to add it to the catalog and document it. The intention is to make it > >>> a builtin as it will be generally useful. If you want to work on the > >>> remaining parts then I will happily ship you the C code for the trigger. > >>> > >>> > >>> > >> In fact, I just finished writing the C code and including it in the catalog > >> (Just tested that it's visible in the catalog). I will test it to see if it > >> does actually do what we want it to. > >> > >> I have incorporated all the suggestions above. Would love to see your code > >> in the meantime. > >> > >> Here's the C code: > >> > >> Datum > >> trig_ignore_duplicate_updates( PG_FUNCTION_ARGS ) > >> { > >> TriggerData *trigData; > >> HeapTuple oldTuple; > >> HeapTuple newTuple; > >> > >> if (!CALLED_AS_TRIGGER(fcinfo)) > >> elog(ERROR, "trig_ignore_duplicate_updates: not called by trigger > >> manager."); > >> > >> if( !TRIGGER_FIRED_BY_UPDATE(trigData->tg_event) > >> && !TRIGGER_FIRED_BEFORE(trigData->tg_event) > >> && !TRIGGER_FIRED_FOR_ROW(trigData->tg_event) ) > >> { > >> elog(ERROR, "trig_ignore_duplicate_updates: Can only be executed for > >> UPDATE, BEFORE and FOR EACH ROW."); > >> } > >> > >> trigData = (TriggerData *) fcinfo->context; > >> oldTuple = trigData->tg_trigtuple; > >> newTuple = trigData->tg_newtuple; > >> > >> if (newTuple->t_len == oldTuple->t_len > >> && newTuple->t_data->t_hoff == oldTuple->t_data->t_hoff > >> && HeapTupleHeaderGetNatts(newTuple->t_data) == > >> HeapTupleHeaderGetNatts(oldTuple->t_data) > >> && (newTuple->t_data->t_infomask & ~HEAP_XACT_MASK) > >> == (oldTuple->t_data->t_infomask & ~HEAP_XACT_MASK) > >> && memcmp( (char*)(newTuple->t_data) + offsetof(HeapTupleHeaderData, > >> t_bits), > >> (char*)(oldTuple->t_data) + offsetof(HeapTupleHeaderData, > >> t_bits), > >> newTuple->t_len - offsetof(HeapTupleHeaderData, t_bits) > >> ) == 0 ) > >> { > >> /* return without crating a new tuple */ > >> return PointerGetDatum( NULL ); > >> } > >> > >> return PointerGetDatum( trigData->tg_newtuple ); > >> } > >> > >> > >> > >> -- > >> gurjeet[.singh]@EnterpriseDB.com > >> singh.gurjeet@{ gmail | hotmail | indiatimes | yahoo }.com > >> > >> EnterpriseDB http://www.enterprisedb.com > >> > >> 17? 29' 34.37"N, 78? 30' 59.76"E - Hyderabad * > >> 18? 32' 57.25"N, 73? 56' 25.42"E - Pune > >> 37? 47' 19.72"N, 122? 24' 1.69" W - San Francisco > >> > >> http://gurjeet.frihost.net > >> > >> Mail sent from my BlackLaptop device > >> > > > > -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Right. In fact, I already had that part in fact - see http://people.planetpostgresql.org/andrew/index.php?/archives/22-Minimal-Update-Trigger.html What I was waiting for was the part where it gets put in the catalog, documented, etc. cheers andrew Bruce Momjian wrote: > Andrew Dunstan wrote: > >> Not that I know of. I never saw Gurjeet's completed code. >> > > This is Gurjeet's code, but it is not complete. > > http://archives.postgresql.org/pgsql-hackers/2008-03/msg00668.php > > --------------------------------------------------------------------------- > > >> cheers >> >> andrew >> >> Bruce Momjian wrote: >> >>> Is there a version of this patch ready for application? >>> >>> --------------------------------------------------------------------------- >>> >>> Gurjeet Singh wrote: >>> >>> >>>> On Tue, Mar 18, 2008 at 7:46 PM, Andrew Dunstan <andrew@dunslane.net> wrote: >>>> >>>> >>>> >>>>> >>>>> Gurjeet Singh wrote: >>>>> >>>>> >>>>>> On Fri, Mar 7, 2008 at 9:40 PM, Bruce Momjian <bruce@momjian.us >>>>>> <mailto:bruce@momjian.us>> wrote: >>>>>> >>>>>> >>>>>> I assume don't want a TODO for this? (Suppress UPDATE no changed >>>>>> columns) >>>>>> >>>>>> >>>>>> I am starting to implement this. Do we want to have this trigger >>>>>> function in the server, or in an external module? >>>>>> >>>>>> >>>>>> >>>>>> >>>>> I have the trigger part of this done, in fact. What remains to be done >>>>> is to add it to the catalog and document it. The intention is to make it >>>>> a builtin as it will be generally useful. If you want to work on the >>>>> remaining parts then I will happily ship you the C code for the trigger. >>>>> >>>>> >>>>> >>>>> >>>> In fact, I just finished writing the C code and including it in the catalog >>>> (Just tested that it's visible in the catalog). I will test it to see if it >>>> does actually do what we want it to. >>>> >>>> I have incorporated all the suggestions above. Would love to see your code >>>> in the meantime. >>>> >>>> Here's the C code: >>>> >>>> Datum >>>> trig_ignore_duplicate_updates( PG_FUNCTION_ARGS ) >>>> { >>>> TriggerData *trigData; >>>> HeapTuple oldTuple; >>>> HeapTuple newTuple; >>>> >>>> if (!CALLED_AS_TRIGGER(fcinfo)) >>>> elog(ERROR, "trig_ignore_duplicate_updates: not called by trigger >>>> manager."); >>>> >>>> if( !TRIGGER_FIRED_BY_UPDATE(trigData->tg_event) >>>> && !TRIGGER_FIRED_BEFORE(trigData->tg_event) >>>> && !TRIGGER_FIRED_FOR_ROW(trigData->tg_event) ) >>>> { >>>> elog(ERROR, "trig_ignore_duplicate_updates: Can only be executed for >>>> UPDATE, BEFORE and FOR EACH ROW."); >>>> } >>>> >>>> trigData = (TriggerData *) fcinfo->context; >>>> oldTuple = trigData->tg_trigtuple; >>>> newTuple = trigData->tg_newtuple; >>>> >>>> if (newTuple->t_len == oldTuple->t_len >>>> && newTuple->t_data->t_hoff == oldTuple->t_data->t_hoff >>>> && HeapTupleHeaderGetNatts(newTuple->t_data) == >>>> HeapTupleHeaderGetNatts(oldTuple->t_data) >>>> && (newTuple->t_data->t_infomask & ~HEAP_XACT_MASK) >>>> == (oldTuple->t_data->t_infomask & ~HEAP_XACT_MASK) >>>> && memcmp( (char*)(newTuple->t_data) + offsetof(HeapTupleHeaderData, >>>> t_bits), >>>> (char*)(oldTuple->t_data) + offsetof(HeapTupleHeaderData, >>>> t_bits), >>>> newTuple->t_len - offsetof(HeapTupleHeaderData, t_bits) >>>> ) == 0 ) >>>> { >>>> /* return without crating a new tuple */ >>>> return PointerGetDatum( NULL ); >>>> } >>>> >>>> return PointerGetDatum( trigData->tg_newtuple ); >>>> } >>>> >>>> >>>> >>>> -- >>>> gurjeet[.singh]@EnterpriseDB.com >>>> singh.gurjeet@{ gmail | hotmail | indiatimes | yahoo }.com >>>> >>>> EnterpriseDB http://www.enterprisedb.com >>>> >>>> 17? 29' 34.37"N, 78? 30' 59.76"E - Hyderabad * >>>> 18? 32' 57.25"N, 73? 56' 25.42"E - Pune >>>> 37? 47' 19.72"N, 122? 24' 1.69" W - San Francisco >>>> >>>> http://gurjeet.frihost.net >>>> >>>> Mail sent from my BlackLaptop device >>>> >>>> >>> >>> > >
Andrew Dunstan wrote: > > Right. In fact, I already had that part in fact - see > http://people.planetpostgresql.org/andrew/index.php?/archives/22-Minimal-Update-Trigger.html > > What I was waiting for was the part where it gets put in the catalog, > documented, etc. I can probably do that part. Send over what you have and I will work on it. Thanks. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > Andrew Dunstan wrote: > >> Right. In fact, I already had that part in fact - see >> http://people.planetpostgresql.org/andrew/index.php?/archives/22-Minimal-Update-Trigger.html >> >> What I was waiting for was the part where it gets put in the catalog, >> documented, etc. >> > > I can probably do that part. Send over what you have and I will work on > it. Thanks. > > It's very similar to what Gurjeet posted (but designed to work with earlier postgres versions) cheers andrew --- |#include "postgres.h" #include "commands/trigger.h" #include "access/htup.h" #ifdef PG_MODULE_MAGIC PG_MODULE_MAGIC; #endif /* for pre 8.3 */ #ifndef HeapTupleHeaderGetNatts #define HeapTupleHeaderGetNatts(th) ( (th)->t_natts ) #endif extern Datum min_update_trigger(PG_FUNCTION_ARGS); PG_FUNCTION_INFO_V1(min_update_trigger); Datum min_update_trigger(PG_FUNCTION_ARGS) { TriggerData *trigdata = (TriggerData *) fcinfo->context; HeapTuple newtuple, oldtuple, rettuple; /* make sure it's called as a trigger at all */ if (!CALLED_AS_TRIGGER(fcinfo)) elog(ERROR, "min_update_trigger:not called by trigger manager"); /* and that it's called on update */ if (! TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event)) elog(ERROR, "min_update_trigger:not called on update"); /* and that it's called before update */ if (! TRIGGER_FIRED_BEFORE(trigdata->tg_event)) elog(ERROR, "min_update_trigger:not called before update"); /* and that it's called for each row */ if (! TRIGGER_FIRED_FOR_ROW(trigdata->tg_event)) elog(ERROR, "min_update_trigger:not called for each row"); /* get tuple dat, set default return */ rettuple = newtuple = trigdata->tg_newtuple; oldtuple = trigdata->tg_trigtuple; if (newtuple->t_len == oldtuple->t_len && newtuple->t_data->t_hoff == oldtuple->t_data->t_hoff && HeapTupleHeaderGetNatts(newtuple->t_data) == HeapTupleHeaderGetNatts(oldtuple->t_data) && (newtuple->t_data->t_infomask& ~HEAP_XACT_MASK) == (oldtuple->t_data->t_infomask & ~HEAP_XACT_MASK) && memcmp(((char *)newtuple->t_data) + offsetof(HeapTupleHeaderData, t_bits), ((char *)oldtuple->t_data)+ offsetof(HeapTupleHeaderData, t_bits), newtuple->t_len - offsetof(HeapTupleHeaderData,t_bits)) == 0) rettuple = NULL; return PointerGetDatum(rettuple); }|
Bruce, did you ever look at completing this? cheers andrew Andrew Dunstan wrote: > > > Bruce Momjian wrote: >> Andrew Dunstan wrote: >> >>> Right. In fact, I already had that part in fact - see >>> http://people.planetpostgresql.org/andrew/index.php?/archives/22-Minimal-Update-Trigger.html >>> >>> >>> What I was waiting for was the part where it gets put in the >>> catalog, documented, etc. >>> >> >> I can probably do that part. Send over what you have and I will work on >> it. Thanks. >> >> > > It's very similar to what Gurjeet posted (but designed to work with > earlier postgres versions) > > cheers > > andrew > > --- > > |#include "postgres.h" > #include "commands/trigger.h" > #include "access/htup.h" > > #ifdef PG_MODULE_MAGIC > PG_MODULE_MAGIC; > #endif > > /* for pre 8.3 */ > #ifndef HeapTupleHeaderGetNatts > #define HeapTupleHeaderGetNatts(th) ( (th)->t_natts ) > #endif > > extern Datum min_update_trigger(PG_FUNCTION_ARGS); > > PG_FUNCTION_INFO_V1(min_update_trigger); > > Datum > min_update_trigger(PG_FUNCTION_ARGS) > { > TriggerData *trigdata = (TriggerData *) fcinfo->context; > HeapTuple newtuple, oldtuple, rettuple; > > /* make sure it's called as a trigger at all */ > if (!CALLED_AS_TRIGGER(fcinfo)) > elog(ERROR, "min_update_trigger: not called by trigger manager"); > > /* and that it's called on update */ > if (! TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event)) > elog(ERROR, "min_update_trigger: not called on update"); > > /* and that it's called before update */ > if (! TRIGGER_FIRED_BEFORE(trigdata->tg_event)) > elog(ERROR, "min_update_trigger: not called before update"); > > /* and that it's called for each row */ > if (! TRIGGER_FIRED_FOR_ROW(trigdata->tg_event)) > elog(ERROR, "min_update_trigger: not called for each row"); > > /* get tuple dat, set default return */ > rettuple = newtuple = trigdata->tg_newtuple; > oldtuple = trigdata->tg_trigtuple; > > if (newtuple->t_len == oldtuple->t_len && > newtuple->t_data->t_hoff == oldtuple->t_data->t_hoff && > HeapTupleHeaderGetNatts(newtuple->t_data) == > HeapTupleHeaderGetNatts(oldtuple->t_data) && > (newtuple->t_data->t_infomask & ~HEAP_XACT_MASK) == > (oldtuple->t_data->t_infomask & ~HEAP_XACT_MASK) && > memcmp(((char *)newtuple->t_data) + > offsetof(HeapTupleHeaderData, t_bits), > ((char *)oldtuple->t_data) + > offsetof(HeapTupleHeaderData, t_bits), > newtuple->t_len - > offsetof(HeapTupleHeaderData, t_bits)) == 0) > rettuple = NULL; > > return PointerGetDatum(rettuple); > }| > > > >
Andrew Dunstan wrote: > > Bruce, > > did you ever look at completing this? No, it is still in my email box unaddressed. Feel free to work on it; I doubt I can do it for 8.4. --------------------------------------------------------------------------- > > cheers > > andrew > > Andrew Dunstan wrote: > > > > > > Bruce Momjian wrote: > >> Andrew Dunstan wrote: > >> > >>> Right. In fact, I already had that part in fact - see > >>> http://people.planetpostgresql.org/andrew/index.php?/archives/22-Minimal-Update-Trigger.html > >>> > >>> > >>> What I was waiting for was the part where it gets put in the > >>> catalog, documented, etc. > >>> > >> > >> I can probably do that part. Send over what you have and I will work on > >> it. Thanks. > >> > >> > > > > It's very similar to what Gurjeet posted (but designed to work with > > earlier postgres versions) > > > > cheers > > > > andrew > > > > --- > > > > |#include "postgres.h" > > #include "commands/trigger.h" > > #include "access/htup.h" > > > > #ifdef PG_MODULE_MAGIC > > PG_MODULE_MAGIC; > > #endif > > > > /* for pre 8.3 */ > > #ifndef HeapTupleHeaderGetNatts > > #define HeapTupleHeaderGetNatts(th) ( (th)->t_natts ) > > #endif > > > > extern Datum min_update_trigger(PG_FUNCTION_ARGS); > > > > PG_FUNCTION_INFO_V1(min_update_trigger); > > > > Datum > > min_update_trigger(PG_FUNCTION_ARGS) > > { > > TriggerData *trigdata = (TriggerData *) fcinfo->context; > > HeapTuple newtuple, oldtuple, rettuple; > > > > /* make sure it's called as a trigger at all */ > > if (!CALLED_AS_TRIGGER(fcinfo)) > > elog(ERROR, "min_update_trigger: not called by trigger manager"); > > > > /* and that it's called on update */ > > if (! TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event)) > > elog(ERROR, "min_update_trigger: not called on update"); > > > > /* and that it's called before update */ > > if (! TRIGGER_FIRED_BEFORE(trigdata->tg_event)) > > elog(ERROR, "min_update_trigger: not called before update"); > > > > /* and that it's called for each row */ > > if (! TRIGGER_FIRED_FOR_ROW(trigdata->tg_event)) > > elog(ERROR, "min_update_trigger: not called for each row"); > > > > /* get tuple dat, set default return */ > > rettuple = newtuple = trigdata->tg_newtuple; > > oldtuple = trigdata->tg_trigtuple; > > > > if (newtuple->t_len == oldtuple->t_len && > > newtuple->t_data->t_hoff == oldtuple->t_data->t_hoff && > > HeapTupleHeaderGetNatts(newtuple->t_data) == > > HeapTupleHeaderGetNatts(oldtuple->t_data) && > > (newtuple->t_data->t_infomask & ~HEAP_XACT_MASK) == > > (oldtuple->t_data->t_infomask & ~HEAP_XACT_MASK) && > > memcmp(((char *)newtuple->t_data) + > > offsetof(HeapTupleHeaderData, t_bits), > > ((char *)oldtuple->t_data) + > > offsetof(HeapTupleHeaderData, t_bits), > > newtuple->t_len - > > offsetof(HeapTupleHeaderData, t_bits)) == 0) > > rettuple = NULL; > > > > return PointerGetDatum(rettuple); > > }| > > > > > > > > -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. +
Bruce Momjian wrote: > Andrew Dunstan wrote: > >> Bruce, >> >> did you ever look at completing this? >> > > No, it is still in my email box unaddressed. Feel free to work on it; I > doubt I can do it for 8.4. > > > OK. Where would be a good place to put the code? Maybe a new file src/backend/utils/adt/trigger_utils.c ? cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> OK. Where would be a good place to put the code? Maybe a new file
> src/backend/utils/adt/trigger_utils.c ?
I thought the plan was to make it a contrib module.
regards, tom lane
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > >> OK. Where would be a good place to put the code? Maybe a new file >> src/backend/utils/adt/trigger_utils.c ? >> > > I thought the plan was to make it a contrib module. > > > Well, previous discussion did mention catalog entries, which would suggest otherwise, but I can do it as a contrib module if that's the consensus. cheers andrew
Andrew Dunstan wrote: > > > Tom Lane wrote: >> Andrew Dunstan <andrew@dunslane.net> writes: >> >>> OK. Where would be a good place to put the code? Maybe a new file >>> src/backend/utils/adt/trigger_utils.c ? >>> >> >> I thought the plan was to make it a contrib module. >> >> >> > > Well, previous discussion did mention catalog entries, which would > suggest otherwise, but I can do it as a contrib module if that's the > consensus. What would be the actual reason to put it in contrib and not core? Are there any "dangers" by having it there? Or is it "just a hack" and not a "real solution"? //Magnus
Magnus Hagander wrote: > Andrew Dunstan wrote: > >> Tom Lane wrote: >> >>> Andrew Dunstan <andrew@dunslane.net> writes: >>> >>> >>>> OK. Where would be a good place to put the code? Maybe a new file >>>> src/backend/utils/adt/trigger_utils.c ? >>>> >>>> >>> I thought the plan was to make it a contrib module. >>> >>> >>> >>> >> Well, previous discussion did mention catalog entries, which would >> suggest otherwise, but I can do it as a contrib module if that's the >> consensus. >> > > What would be the actual reason to put it in contrib and not core? Are > there any "dangers" by having it there? Or is it "just a hack" and not a > "real solution"? > > > No, it's not just a hack. It's very close to what we'd probably do if we built the facility right into the language, although it does involve the overhead of calling the trigger. However, it performs reasonably well - not surprising since the guts of it is just a memcmp() call. cheers andrew
On 20 okt 2008, at 16.51, Andrew Dunstan <andrew@dunslane.net> wrote: > > > Magnus Hagander wrote: >> Andrew Dunstan wrote: >> >>> Tom Lane wrote: >>> >>>> Andrew Dunstan <andrew@dunslane.net> writes: >>>> >>>>> OK. Where would be a good place to put the code? Maybe a new file >>>>> src/backend/utils/adt/trigger_utils.c ? >>>>> >>>> I thought the plan was to make it a contrib module. >>>> >>>> >>> Well, previous discussion did mention catalog entries, which would >>> suggest otherwise, but I can do it as a contrib module if that's the >>> consensus. >>> >> >> What would be the actual reason to put it in contrib and not core? >> Are >> there any "dangers" by having it there? Or is it "just a hack" and >> not a >> "real solution"? >> >> >> > > No, it's not just a hack. It's very close to what we'd probably do > if we built the facility right into the language, although it does > involve the overhead of calling the trigger. However, it performs > reasonably well - not surprising since the guts of it is just a > memcmp() call. > In that case, why not put the trigger in core so people can use it easily? /magnus
On Tue, Oct 21, 2008 at 03:34:04PM +0200, Magnus Hagander wrote: > On 20 okt 2008, at 16.51, Andrew Dunstan <andrew@dunslane.net> wrote: >> Magnus Hagander wrote: >>> Andrew Dunstan wrote: >>>> Tom Lane wrote: >>>>> Andrew Dunstan <andrew@dunslane.net> writes: >>>>>> OK. Where would be a good place to put the code? Maybe a new >>>>>> file src/backend/utils/adt/trigger_utils.c ? >>>>>> >>>>> I thought the plan was to make it a contrib module. >>>>> >>>> Well, previous discussion did mention catalog entries, which >>>> would suggest otherwise, but I can do it as a contrib module if >>>> that's the consensus. >>> >>> What would be the actual reason to put it in contrib and not core? >>> Are there any "dangers" by having it there? Or is it "just a hack" >>> and not a "real solution"? >> >> No, it's not just a hack. It's very close to what we'd probably do >> if we built the facility right into the language, although it does >> involve the overhead of calling the trigger. However, it performs >> reasonably well - not surprising since the guts of it is just a >> memcmp() call. >> > In that case, why not put the trigger in core so people can use it > easily? +1 :) Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On Tue, Oct 21, 2008 at 9:34 AM, Magnus Hagander <magnus@hagander.net> wrote: > On 20 okt 2008, at 16.51, Andrew Dunstan <andrew@dunslane.net> wrote: >> >> No, it's not just a hack. It's very close to what we'd probably do if we >> built the facility right into the language, although it does involve the >> overhead of calling the trigger. However, it performs reasonably well - not >> surprising since the guts of it is just a memcmp() call. >> > In that case, why not put the trigger in core so people can use it easily? +1 This is hard to get right and a common source of errors. merlin
Magnus Hagander <magnus@hagander.net> writes:
> In that case, why not put the trigger in core so people can use it
> easily?
One advantage of making it a contrib module is that discussing how/when
to use it would fit more easily into the structure of the
documentation. There is no place in our docs that a "standard trigger"
would fit without seeming like a wart; but a contrib module can document
itself pretty much however it wants.
regards, tom lane
Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: > >> In that case, why not put the trigger in core so people can use it >> easily? >> > > One advantage of making it a contrib module is that discussing how/when > to use it would fit more easily into the structure of the > documentation. There is no place in our docs that a "standard trigger" > would fit without seeming like a wart; but a contrib module can document > itself pretty much however it wants. > I was thinking a new section on 'trigger functions' of the functions and operators chapter, linked from the 'create trigger' page. That doesn't seem like too much of a wart. cheers andrew
Andrew Dunstan wrote:
>
>
> Tom Lane wrote:
>> Magnus Hagander <magnus@hagander.net> writes:
>>
>>> In that case, why not put the trigger in core so people can use it
>>> easily?
>>>
>>
>> One advantage of making it a contrib module is that discussing how/when
>> to use it would fit more easily into the structure of the
>> documentation. There is no place in our docs that a "standard trigger"
>> would fit without seeming like a wart; but a contrib module can document
>> itself pretty much however it wants.
>>
>
> I was thinking a new section on 'trigger functions' of the functions
> and operators chapter, linked from the 'create trigger' page. That
> doesn't seem like too much of a wart.
>
>
There seems to be a preponderance of opinion for doing this as a
builtin. Here is a patch that does it that way, along with docs and
regression test.
cheers
andrew
Index: doc/src/sgml/func.sgml
===================================================================
RCS file: /cvsroot/pgsql/doc/src/sgml/func.sgml,v
retrieving revision 1.450
diff -c -r1.450 func.sgml
*** doc/src/sgml/func.sgml 14 Oct 2008 17:12:32 -0000 1.450
--- doc/src/sgml/func.sgml 22 Oct 2008 18:35:51 -0000
***************
*** 12817,12820 ****
--- 12817,12845 ----
</sect1>
+ <sect1 id="functions-trigger">
+ <title>Trigger Functions</title>
+
+ <para>
+ Currently <productname>PostgreSQL</> provides one built in trigger
+ function, <function>min_update_trigger</>, which will prevent any update
+ that does not actually change the data in the row from taking place, in
+ contrast to the normal behaviour which always performs the update
+ regardless of whether or not the data has changed.
+ </para>
+
+ <para>
+ The <function>min_update_trigger</> function can be added to a table
+ like this:
+ <programlisting>
+ CREATE TRIGGER _min_update
+ BEFORE UPDATE ON tablename
+ FOR EACH ROW EXECUTE PROCEDURE min_update_trigger();
+ </programlisting>
+ </para>
+ <para>
+ For mare information about creating triggers, see
+ <xref linkend="SQL-CREATETRIGGER">.
+ </para>
+ </sect1>
</chapter>
Index: src/backend/utils/adt/Makefile
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/Makefile,v
retrieving revision 1.69
diff -c -r1.69 Makefile
*** src/backend/utils/adt/Makefile 19 Feb 2008 10:30:08 -0000 1.69
--- src/backend/utils/adt/Makefile 22 Oct 2008 18:35:51 -0000
***************
*** 25,31 ****
tid.o timestamp.o varbit.o varchar.o varlena.o version.o xid.o \
network.o mac.o inet_net_ntop.o inet_net_pton.o \
ri_triggers.o pg_lzcompress.o pg_locale.o formatting.o \
! ascii.o quote.o pgstatfuncs.o encode.o dbsize.o genfile.o \
tsginidx.o tsgistidx.o tsquery.o tsquery_cleanup.o tsquery_gist.o \
tsquery_op.o tsquery_rewrite.o tsquery_util.o tsrank.o \
tsvector.o tsvector_op.o tsvector_parser.o \
--- 25,31 ----
tid.o timestamp.o varbit.o varchar.o varlena.o version.o xid.o \
network.o mac.o inet_net_ntop.o inet_net_pton.o \
ri_triggers.o pg_lzcompress.o pg_locale.o formatting.o \
! ascii.o quote.o pgstatfuncs.o encode.o dbsize.o genfile.o trigfuncs.o \
tsginidx.o tsgistidx.o tsquery.o tsquery_cleanup.o tsquery_gist.o \
tsquery_op.o tsquery_rewrite.o tsquery_util.o tsrank.o \
tsvector.o tsvector_op.o tsvector_parser.o \
Index: src/backend/utils/adt/trigfuncs.c
===================================================================
RCS file: src/backend/utils/adt/trigfuncs.c
diff -N src/backend/utils/adt/trigfuncs.c
*** /dev/null 1 Jan 1970 00:00:00 -0000
--- src/backend/utils/adt/trigfuncs.c 22 Oct 2008 18:35:51 -0000
***************
*** 0 ****
--- 1,73 ----
+ /*-------------------------------------------------------------------------
+ *
+ * trigfuncs.c
+ * Builtin functions for useful trigger support.
+ *
+ *
+ * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * $PostgreSQL:$
+ *
+ *-------------------------------------------------------------------------
+ */
+
+
+
+ #include "postgres.h"
+ #include "commands/trigger.h"
+ #include "access/htup.h"
+
+ /*
+ * min_update_trigger
+ *
+ * This trigger function will inhibit an update from being done
+ * if the OLD and NEW records are identical.
+ *
+ */
+
+ Datum
+ min_update_trigger(PG_FUNCTION_ARGS)
+ {
+ TriggerData *trigdata = (TriggerData *) fcinfo->context;
+ HeapTuple newtuple, oldtuple, rettuple;
+ HeapTupleHeader newheader, oldheader;
+
+ /* make sure it's called as a trigger */
+ if (!CALLED_AS_TRIGGER(fcinfo))
+ elog(ERROR, "min_update_trigger: not called by trigger manager");
+
+ /* and that it's called on update */
+ if (! TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event))
+ elog(ERROR, "min_update_trigger: not called on update");
+
+ /* and that it's called before update */
+ if (! TRIGGER_FIRED_BEFORE(trigdata->tg_event))
+ elog(ERROR, "min_update_trigger: not called before update");
+
+ /* and that it's called for each row */
+ if (! TRIGGER_FIRED_FOR_ROW(trigdata->tg_event))
+ elog(ERROR, "min_update_trigger: not called for each row");
+
+ /* get tuple data, set default return */
+ rettuple = newtuple = trigdata->tg_newtuple;
+ oldtuple = trigdata->tg_trigtuple;
+
+ newheader = newtuple->t_data;
+ oldheader = oldtuple->t_data;
+
+ if (newtuple->t_len == oldtuple->t_len &&
+ newheader->t_hoff == oldheader->t_hoff &&
+ (HeapTupleHeaderGetNatts(newheader) ==
+ HeapTupleHeaderGetNatts(oldheader) ) &&
+ ((newheader->t_infomask & ~HEAP_XACT_MASK) ==
+ (oldheader->t_infomask & ~HEAP_XACT_MASK) )&&
+ memcmp(((char *)newheader) + offsetof(HeapTupleHeaderData, t_bits),
+ ((char *)oldheader) + offsetof(HeapTupleHeaderData, t_bits),
+ newtuple->t_len - offsetof(HeapTupleHeaderData, t_bits)) == 0)
+ {
+ rettuple = NULL;
+ }
+
+ return PointerGetDatum(rettuple);
+ }
Index: src/include/catalog/pg_proc.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/catalog/pg_proc.h,v
retrieving revision 1.520
diff -c -r1.520 pg_proc.h
*** src/include/catalog/pg_proc.h 14 Oct 2008 17:12:33 -0000 1.520
--- src/include/catalog/pg_proc.h 22 Oct 2008 18:35:52 -0000
***************
*** 2290,2295 ****
--- 2290,2298 ----
DATA(insert OID = 1686 ( pg_get_keywords PGNSP PGUID 12 10 400 0 f f t t s 0 2249 "" "{25,18,25}" "{o,o,o}"
"{word,catcode,catdesc}"pg_get_keywords _null_ _null_ _null_ ));
DESCR("list of SQL keywords");
+ /* utility minimal update trigger */
+ DATA(insert OID = 1619 ( min_update_trigger PGNSP PGUID 12 1 0 0 f f t f v 0 2279 "" _null_ _null_ _null_
min_update_trigger_null_ _null_ _null_ ));
+ DESCR("minimal update trigger function");
/* Generic referential integrity constraint triggers */
DATA(insert OID = 1644 ( RI_FKey_check_ins PGNSP PGUID 12 1 0 0 f f t f v 0 2279 "" _null_ _null_ _null_
RI_FKey_check_ins_null_ _null_ _null_ ));
Index: src/include/utils/builtins.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/utils/builtins.h,v
retrieving revision 1.324
diff -c -r1.324 builtins.h
*** src/include/utils/builtins.h 13 Oct 2008 16:25:20 -0000 1.324
--- src/include/utils/builtins.h 22 Oct 2008 18:35:52 -0000
***************
*** 899,904 ****
--- 899,907 ----
extern Datum RI_FKey_setdefault_del(PG_FUNCTION_ARGS);
extern Datum RI_FKey_setdefault_upd(PG_FUNCTION_ARGS);
+ /* trigfuncs.c */
+ extern Datum min_update_trigger(PG_FUNCTION_ARGS);
+
/* encoding support functions */
extern Datum getdatabaseencoding(PG_FUNCTION_ARGS);
extern Datum database_character_set(PG_FUNCTION_ARGS);
Index: src/test/regress/expected/triggers.out
===================================================================
RCS file: /cvsroot/pgsql/src/test/regress/expected/triggers.out,v
retrieving revision 1.24
diff -c -r1.24 triggers.out
*** src/test/regress/expected/triggers.out 1 Feb 2007 19:10:30 -0000 1.24
--- src/test/regress/expected/triggers.out 22 Oct 2008 18:35:52 -0000
***************
*** 537,539 ****
--- 537,564 ----
NOTICE: row 2 not changed
DROP TABLE trigger_test;
DROP FUNCTION mytrigger();
+ -- minimal update trigger
+ CREATE TABLE min_update_test (
+ f1 text,
+ f2 int,
+ f3 int);
+ INSERT INTO min_update_test VALUES ('a',1,2),('b','2',null);
+ CREATE TRIGGER _min_update
+ BEFORE UPDATE ON min_update_test
+ FOR EACH ROW EXECUTE PROCEDURE min_update_trigger();
+ \set QUIET false
+ UPDATE min_update_test SET f1 = f1;
+ UPDATE 0
+ UPDATE min_update_test SET f2 = f2 + 1;
+ UPDATE 2
+ UPDATE min_update_test SET f3 = 2 WHERE f3 is null;
+ UPDATE 1
+ \set QUIET true
+ SELECT * FROM min_update_test;
+ f1 | f2 | f3
+ ----+----+----
+ a | 2 | 2
+ b | 3 | 2
+ (2 rows)
+
+ DROP TABLE min_update_test;
Index: src/test/regress/sql/triggers.sql
===================================================================
RCS file: /cvsroot/pgsql/src/test/regress/sql/triggers.sql,v
retrieving revision 1.13
diff -c -r1.13 triggers.sql
*** src/test/regress/sql/triggers.sql 26 Jun 2006 17:24:41 -0000 1.13
--- src/test/regress/sql/triggers.sql 22 Oct 2008 18:35:52 -0000
***************
*** 415,417 ****
--- 415,446 ----
DROP TABLE trigger_test;
DROP FUNCTION mytrigger();
+
+
+ -- minimal update trigger
+
+ CREATE TABLE min_update_test (
+ f1 text,
+ f2 int,
+ f3 int);
+
+ INSERT INTO min_update_test VALUES ('a',1,2),('b','2',null);
+
+ CREATE TRIGGER _min_update
+ BEFORE UPDATE ON min_update_test
+ FOR EACH ROW EXECUTE PROCEDURE min_update_trigger();
+
+ \set QUIET false
+
+ UPDATE min_update_test SET f1 = f1;
+
+ UPDATE min_update_test SET f2 = f2 + 1;
+
+ UPDATE min_update_test SET f3 = 2 WHERE f3 is null;
+
+ \set QUIET true
+
+ SELECT * FROM min_update_test;
+
+ DROP TABLE min_update_test;
+
>>> Andrew Dunstan <andrew@dunslane.net> wrote: > Here is a patch that does it that way, along with docs s/mare/more/ -Kevin
Kevin Grittner wrote: >>>> Andrew Dunstan <andrew@dunslane.net> wrote: >>>> > > >> Here is a patch that does it that way, along with docs >> > > s/mare/more/ > > Thanks. fixed in my tree.. cheers andrew
On Wed, 2008-10-22 at 14:43 -0400, Andrew Dunstan wrote: > > There seems to be a preponderance of opinion for doing this as a > builtin. Here is a patch that does it that way, along with docs and > regression test. In your example you use an underscore as the first character. The way you have done this it will probably exclude any other before row triggers from firing, which may have altered the value of one or more columns. The more probable choice for me would be to have a trigger that came after all other before triggers, and so should have a different name. It's just an example, so your choice is fine, but I think you should bring out that point more clearly for the average developer. Can we call the function "minimal_update_trigger", rather than min_... -- Simon Riggs www.2ndQuadrant.comPostgreSQL Training, Services and Support
Simon Riggs <simon@2ndQuadrant.com> writes:
> Can we call the function "minimal_update_trigger", rather than min_...
"Minimal" really fails to convey the point here IMHO. How about
something like "suppress_no_op_updates_trigger"?
regards, tom lane
Simon Riggs wrote: > On Wed, 2008-10-22 at 14:43 -0400, Andrew Dunstan wrote: > > > >> There seems to be a preponderance of opinion for doing this as a >> builtin. Here is a patch that does it that way, along with docs and >> regression test. >> > > In your example you use an underscore as the first character. The way > you have done this it will probably exclude any other before row > triggers from firing, which may have altered the value of one or more > columns. The more probable choice for me would be to have a trigger that > came after all other before triggers, and so should have a different > name. It's just an example, so your choice is fine, but I think you > should bring out that point more clearly for the average developer. > Fair point. I'll add that to the docs. > Can we call the function "minimal_update_trigger", rather than min_... > > If that's the general consensus, sure. I have no strong opinion. cheers andrew
On Wed, Oct 22, 2008 at 3:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Simon Riggs <simon@2ndQuadrant.com> writes: >> Can we call the function "minimal_update_trigger", rather than min_... > > "Minimal" really fails to convey the point here IMHO. How about > something like "suppress_no_op_updates_trigger"? +1. That's a much better name. ...Robert
On Wed, 2008-10-22 at 17:24 -0400, Robert Haas wrote: > On Wed, Oct 22, 2008 at 3:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Simon Riggs <simon@2ndQuadrant.com> writes: > >> Can we call the function "minimal_update_trigger", rather than min_... > > > > "Minimal" really fails to convey the point here IMHO. How about > > something like "suppress_no_op_updates_trigger"? > > +1. That's a much better name. > I think it means something to us, but "no op" is a very technical phrase that probably doesn't travel very well. Not everybody Majored in Comp Sci and speaks Amglish as their native language. Certainly this intention is much better than "minimal", but a more widely acceptable phrase is probably better. I will avoid trying to come up with something myself though. -- Simon Riggs www.2ndQuadrant.comPostgreSQL Training, Services and Support
Simon Riggs <simon@2ndQuadrant.com> writes:
>> On Wed, Oct 22, 2008 at 3:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> "Minimal" really fails to convey the point here IMHO. How about
>>> something like "suppress_no_op_updates_trigger"?
> I think it means something to us, but "no op" is a very technical phrase
> that probably doesn't travel very well.
Agreed --- I was hoping someone could improve on that part. The only
other words I could come up with were "empty" and "useless", neither of
which seem quite le mot juste ...
regards, tom lane
On Wed, Oct 22, 2008 at 06:05:26PM -0400, Tom Lane wrote: > Simon Riggs <simon@2ndQuadrant.com> writes: > >> On Wed, Oct 22, 2008 at 3:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >>> "Minimal" really fails to convey the point here IMHO. How about > >>> something like "suppress_no_op_updates_trigger"? > > > I think it means something to us, but "no op" is a very technical phrase > > that probably doesn't travel very well. > > Agreed --- I was hoping someone could improve on that part. The only > other words I could come up with were "empty" and "useless", neither of > which seem quite le mot juste ... > > regards, tom lane > redundant? Ken
>>> Simon Riggs <simon@2ndQuadrant.com> wrote: >> On Wed, Oct 22, 2008 at 3:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> > How about >> > something like "suppress_no_op_updates_trigger"? > > I think it means something to us, but "no op" is a very technical phrase > that probably doesn't travel very well. Not everybody Majored in Comp > Sci and speaks Amglish as their native language. > > Certainly this intention is much better than "minimal", but a more > widely acceptable phrase is probably better. I will avoid trying to come > up with something myself though. How about one of these?: suppress_same_value_updates_trigger suppress_no_change_updates_trigger suppress_no_effect_updates_trigger They all seem a bit awkward, but the best that came to mind. -Kevin
> How about one of these?: > suppress_same_value_updates_trigger > suppress_no_change_updates_trigger > suppress_no_effect_updates_trigger I like the first one. A trigger firing would be an "effect", and possibly a "change", but "same value" seems very clear. ...Robert
On Oct 22, 2008, at 1:43 PM, Andrew Dunstan wrote: > + if (!CALLED_AS_TRIGGER(fcinfo)) > + elog(ERROR, "min_update_trigger: not called by trigger > manager"); The error I get in 8.2 when calling a trigger function directly is: ERROR: trigger functions may only be called as triggers To stay consistent, I think the remaining errors should s/: not/ may only be/, ie: min_update_trigger may only be called on update > + /* and that it's called on update */ > + if (! TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event)) > + elog(ERROR, "min_update_trigger: not called on update"); > + > + /* and that it's called before update */ > + if (! TRIGGER_FIRED_BEFORE(trigdata->tg_event)) > + elog(ERROR, "min_update_trigger: not called before update"); > + > + /* and that it's called for each row */ > + if (! TRIGGER_FIRED_FOR_ROW(trigdata->tg_event)) > + elog(ERROR, "min_update_trigger: not called for each row"); -- Decibel!, aka Jim C. Nasby, Database Architect decibel@decibel.org Give your computer some brain candy! www.distributed.net Team #1828
Kenneth Marshall wrote:
> On Wed, Oct 22, 2008 at 06:05:26PM -0400, Tom Lane wrote:
>
>> Simon Riggs <simon@2ndQuadrant.com> writes:
>>
>>>> On Wed, Oct 22, 2008 at 3:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>>
>>>>> "Minimal" really fails to convey the point here IMHO. How about
>>>>> something like "suppress_no_op_updates_trigger"?
>>>>>
>>> I think it means something to us, but "no op" is a very technical phrase
>>> that probably doesn't travel very well.
>>>
>> Agreed --- I was hoping someone could improve on that part. The only
>> other words I could come up with were "empty" and "useless", neither of
>> which seem quite le mot juste ...
>>
>> regards, tom lane
>>
>>
> redundant?
>
>
>
I think I like this best of all the suggestions -
suppress_redundant_updates_trigger() is what I have now.
If there's no further discussion, I'll go ahead and commit this in a day
or two.
cheers
andrew
? GNUmakefile
? config.log
? config.status
? contrib/spi/.deps
? src/Makefile.global
? src/backend/postgres
? src/backend/access/common/.deps
? src/backend/access/gin/.deps
? src/backend/access/gist/.deps
? src/backend/access/hash/.deps
? src/backend/access/heap/.deps
? src/backend/access/index/.deps
? src/backend/access/nbtree/.deps
? src/backend/access/transam/.deps
? src/backend/bootstrap/.deps
? src/backend/catalog/.deps
? src/backend/catalog/postgres.bki
? src/backend/catalog/postgres.description
? src/backend/catalog/postgres.shdescription
? src/backend/commands/.deps
? src/backend/executor/.deps
? src/backend/lib/.deps
? src/backend/libpq/.deps
? src/backend/main/.deps
? src/backend/nodes/.deps
? src/backend/optimizer/geqo/.deps
? src/backend/optimizer/path/.deps
? src/backend/optimizer/plan/.deps
? src/backend/optimizer/prep/.deps
? src/backend/optimizer/util/.deps
? src/backend/parser/.deps
? src/backend/port/.deps
? src/backend/postmaster/.deps
? src/backend/regex/.deps
? src/backend/rewrite/.deps
? src/backend/snowball/.deps
? src/backend/snowball/snowball_create.sql
? src/backend/storage/buffer/.deps
? src/backend/storage/file/.deps
? src/backend/storage/freespace/.deps
? src/backend/storage/ipc/.deps
? src/backend/storage/large_object/.deps
? src/backend/storage/lmgr/.deps
? src/backend/storage/page/.deps
? src/backend/storage/smgr/.deps
? src/backend/tcop/.deps
? src/backend/tsearch/.deps
? src/backend/utils/.deps
? src/backend/utils/probes.h
? src/backend/utils/adt/.deps
? src/backend/utils/cache/.deps
? src/backend/utils/error/.deps
? src/backend/utils/fmgr/.deps
? src/backend/utils/hash/.deps
? src/backend/utils/init/.deps
? src/backend/utils/mb/.deps
? src/backend/utils/mb/conversion_procs/conversion_create.sql
? src/backend/utils/mb/conversion_procs/ascii_and_mic/.deps
? src/backend/utils/mb/conversion_procs/cyrillic_and_mic/.deps
? src/backend/utils/mb/conversion_procs/euc_cn_and_mic/.deps
? src/backend/utils/mb/conversion_procs/euc_jis_2004_and_shift_jis_2004/.deps
? src/backend/utils/mb/conversion_procs/euc_jp_and_sjis/.deps
? src/backend/utils/mb/conversion_procs/euc_kr_and_mic/.deps
? src/backend/utils/mb/conversion_procs/euc_tw_and_big5/.deps
? src/backend/utils/mb/conversion_procs/latin2_and_win1250/.deps
? src/backend/utils/mb/conversion_procs/latin_and_mic/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_ascii/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_big5/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_cyrillic/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_euc_cn/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_euc_jis_2004/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_euc_jp/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_euc_kr/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_euc_tw/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_gb18030/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_gbk/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_iso8859/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_iso8859_1/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_johab/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_shift_jis_2004/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_sjis/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_uhc/.deps
? src/backend/utils/mb/conversion_procs/utf8_and_win/.deps
? src/backend/utils/misc/.deps
? src/backend/utils/mmgr/.deps
? src/backend/utils/resowner/.deps
? src/backend/utils/sort/.deps
? src/backend/utils/time/.deps
? src/bin/initdb/.deps
? src/bin/initdb/initdb
? src/bin/pg_config/.deps
? src/bin/pg_config/pg_config
? src/bin/pg_controldata/.deps
? src/bin/pg_controldata/pg_controldata
? src/bin/pg_ctl/.deps
? src/bin/pg_ctl/pg_ctl
? src/bin/pg_dump/.deps
? src/bin/pg_dump/pg_dump
? src/bin/pg_dump/pg_dumpall
? src/bin/pg_dump/pg_restore
? src/bin/pg_resetxlog/.deps
? src/bin/pg_resetxlog/pg_resetxlog
? src/bin/psql/.deps
? src/bin/psql/psql
? src/bin/scripts/.deps
? src/bin/scripts/clusterdb
? src/bin/scripts/createdb
? src/bin/scripts/createlang
? src/bin/scripts/createuser
? src/bin/scripts/dropdb
? src/bin/scripts/droplang
? src/bin/scripts/dropuser
? src/bin/scripts/reindexdb
? src/bin/scripts/vacuumdb
? src/include/pg_config.h
? src/include/stamp-h
? src/interfaces/ecpg/compatlib/.deps
? src/interfaces/ecpg/compatlib/exports.list
? src/interfaces/ecpg/compatlib/libecpg_compat.so.3.1
? src/interfaces/ecpg/ecpglib/.deps
? src/interfaces/ecpg/ecpglib/exports.list
? src/interfaces/ecpg/ecpglib/libecpg.so.6.1
? src/interfaces/ecpg/include/ecpg_config.h
? src/interfaces/ecpg/pgtypeslib/.deps
? src/interfaces/ecpg/pgtypeslib/exports.list
? src/interfaces/ecpg/pgtypeslib/libpgtypes.so.3.1
? src/interfaces/ecpg/preproc/.deps
? src/interfaces/ecpg/preproc/ecpg
? src/interfaces/libpq/.deps
? src/interfaces/libpq/exports.list
? src/interfaces/libpq/libpq.so.5.2
? src/pl/plperl/.deps
? src/pl/plperl/SPI.c
? src/pl/plpgsql/src/.deps
? src/pl/plpython/.deps
? src/pl/tcl/.deps
? src/pl/tcl/modules/pltcl_delmod
? src/pl/tcl/modules/pltcl_listmod
? src/pl/tcl/modules/pltcl_loadmod
? src/port/.deps
? src/port/pg_config_paths.h
? src/test/regress/.deps
? src/test/regress/log
? src/test/regress/pg_regress
? src/test/regress/results
? src/test/regress/testtablespace
? src/test/regress/tmp_check
? src/test/regress/expected/constraints.out
? src/test/regress/expected/copy.out
? src/test/regress/expected/create_function_1.out
? src/test/regress/expected/create_function_2.out
? src/test/regress/expected/largeobject.out
? src/test/regress/expected/largeobject_1.out
? src/test/regress/expected/misc.out
? src/test/regress/expected/tablespace.out
? src/test/regress/sql/constraints.sql
? src/test/regress/sql/copy.sql
? src/test/regress/sql/create_function_1.sql
? src/test/regress/sql/create_function_2.sql
? src/test/regress/sql/largeobject.sql
? src/test/regress/sql/misc.sql
? src/test/regress/sql/tablespace.sql
? src/timezone/.deps
? src/timezone/zic
Index: doc/src/sgml/func.sgml
===================================================================
RCS file: /cvsroot/pgsql/doc/src/sgml/func.sgml,v
retrieving revision 1.450
diff -c -r1.450 func.sgml
*** doc/src/sgml/func.sgml 14 Oct 2008 17:12:32 -0000 1.450
--- doc/src/sgml/func.sgml 29 Oct 2008 19:26:53 -0000
***************
*** 12817,12820 ****
--- 12817,12850 ----
</sect1>
+ <sect1 id="functions-trigger">
+ <title>Trigger Functions</title>
+
+ <para>
+ Currently <productname>PostgreSQL</> provides one built in trigger
+ function, <function>suppress_redundant_updates_trigger</>,
+ which will prevent any update
+ that does not actually change the data in the row from taking place, in
+ contrast to the normal behaviour which always performs the update
+ regardless of whether or not the data has changed.
+ </para>
+
+ <para>
+ The <function>suppress_redundant_updates_trigger</> function can be
+ added to a table like this:
+ <programlisting>
+ CREATE TRIGGER z_min_update
+ BEFORE UPDATE ON tablename
+ FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();
+ </programlisting>
+ In many cases, you would want to fire this trigger last for each row.
+ Bearing in mind that triggers fire in name order, you would then
+ choose a trigger name that comes after then name of any other trigger
+ you might have on the table.
+ </para>
+ <para>
+ For more information about creating triggers, see
+ <xref linkend="SQL-CREATETRIGGER">.
+ </para>
+ </sect1>
</chapter>
Index: src/backend/utils/adt/Makefile
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/Makefile,v
retrieving revision 1.69
diff -c -r1.69 Makefile
*** src/backend/utils/adt/Makefile 19 Feb 2008 10:30:08 -0000 1.69
--- src/backend/utils/adt/Makefile 29 Oct 2008 19:26:53 -0000
***************
*** 25,31 ****
tid.o timestamp.o varbit.o varchar.o varlena.o version.o xid.o \
network.o mac.o inet_net_ntop.o inet_net_pton.o \
ri_triggers.o pg_lzcompress.o pg_locale.o formatting.o \
! ascii.o quote.o pgstatfuncs.o encode.o dbsize.o genfile.o \
tsginidx.o tsgistidx.o tsquery.o tsquery_cleanup.o tsquery_gist.o \
tsquery_op.o tsquery_rewrite.o tsquery_util.o tsrank.o \
tsvector.o tsvector_op.o tsvector_parser.o \
--- 25,31 ----
tid.o timestamp.o varbit.o varchar.o varlena.o version.o xid.o \
network.o mac.o inet_net_ntop.o inet_net_pton.o \
ri_triggers.o pg_lzcompress.o pg_locale.o formatting.o \
! ascii.o quote.o pgstatfuncs.o encode.o dbsize.o genfile.o trigfuncs.o \
tsginidx.o tsgistidx.o tsquery.o tsquery_cleanup.o tsquery_gist.o \
tsquery_op.o tsquery_rewrite.o tsquery_util.o tsrank.o \
tsvector.o tsvector_op.o tsvector_parser.o \
Index: src/backend/utils/adt/trigfuncs.c
===================================================================
RCS file: src/backend/utils/adt/trigfuncs.c
diff -N src/backend/utils/adt/trigfuncs.c
*** /dev/null 1 Jan 1970 00:00:00 -0000
--- src/backend/utils/adt/trigfuncs.c 29 Oct 2008 19:26:53 -0000
***************
*** 0 ****
--- 1,73 ----
+ /*-------------------------------------------------------------------------
+ *
+ * trigfuncs.c
+ * Builtin functions for useful trigger support.
+ *
+ *
+ * Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * $PostgreSQL:$
+ *
+ *-------------------------------------------------------------------------
+ */
+
+
+
+ #include "postgres.h"
+ #include "commands/trigger.h"
+ #include "access/htup.h"
+
+ /*
+ * suppress_redundant_updates_trigger
+ *
+ * This trigger function will inhibit an update from being done
+ * if the OLD and NEW records are identical.
+ *
+ */
+
+ Datum
+ suppress_redundant_updates_trigger(PG_FUNCTION_ARGS)
+ {
+ TriggerData *trigdata = (TriggerData *) fcinfo->context;
+ HeapTuple newtuple, oldtuple, rettuple;
+ HeapTupleHeader newheader, oldheader;
+
+ /* make sure it's called as a trigger */
+ if (!CALLED_AS_TRIGGER(fcinfo))
+ elog(ERROR, "suppress_redundant_updates_trigger: must be called as trigger");
+
+ /* and that it's called on update */
+ if (! TRIGGER_FIRED_BY_UPDATE(trigdata->tg_event))
+ elog(ERROR, "suppress_redundant_updates_trigger: may only be called on update");
+
+ /* and that it's called before update */
+ if (! TRIGGER_FIRED_BEFORE(trigdata->tg_event))
+ elog(ERROR, "suppress_redundant_updates_trigger: may only be called before update");
+
+ /* and that it's called for each row */
+ if (! TRIGGER_FIRED_FOR_ROW(trigdata->tg_event))
+ elog(ERROR, "suppress_redundant_updates_trigger: may only be called for each row");
+
+ /* get tuple data, set default return */
+ rettuple = newtuple = trigdata->tg_newtuple;
+ oldtuple = trigdata->tg_trigtuple;
+
+ newheader = newtuple->t_data;
+ oldheader = oldtuple->t_data;
+
+ if (newtuple->t_len == oldtuple->t_len &&
+ newheader->t_hoff == oldheader->t_hoff &&
+ (HeapTupleHeaderGetNatts(newheader) ==
+ HeapTupleHeaderGetNatts(oldheader) ) &&
+ ((newheader->t_infomask & ~HEAP_XACT_MASK) ==
+ (oldheader->t_infomask & ~HEAP_XACT_MASK) )&&
+ memcmp(((char *)newheader) + offsetof(HeapTupleHeaderData, t_bits),
+ ((char *)oldheader) + offsetof(HeapTupleHeaderData, t_bits),
+ newtuple->t_len - offsetof(HeapTupleHeaderData, t_bits)) == 0)
+ {
+ rettuple = NULL;
+ }
+
+ return PointerGetDatum(rettuple);
+ }
Index: src/include/catalog/pg_proc.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/catalog/pg_proc.h,v
retrieving revision 1.520
diff -c -r1.520 pg_proc.h
*** src/include/catalog/pg_proc.h 14 Oct 2008 17:12:33 -0000 1.520
--- src/include/catalog/pg_proc.h 29 Oct 2008 19:26:55 -0000
***************
*** 2290,2295 ****
--- 2290,2298 ----
DATA(insert OID = 1686 ( pg_get_keywords PGNSP PGUID 12 10 400 0 f f t t s 0 2249 "" "{25,18,25}" "{o,o,o}"
"{word,catcode,catdesc}"pg_get_keywords _null_ _null_ _null_ ));
DESCR("list of SQL keywords");
+ /* utility minimal update trigger */
+ DATA(insert OID = 1619 ( suppress_redundant_updates_trigger PGNSP PGUID 12 1 0 0 f f t f v 0 2279 "" _null_
_null__null_ suppress_redundant_updates_trigger _null_ _null_ _null_ ));
+ DESCR("trigger func to suppress updates when new and old records match");
/* Generic referential integrity constraint triggers */
DATA(insert OID = 1644 ( RI_FKey_check_ins PGNSP PGUID 12 1 0 0 f f t f v 0 2279 "" _null_ _null_ _null_
RI_FKey_check_ins_null_ _null_ _null_ ));
Index: src/include/utils/builtins.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/utils/builtins.h,v
retrieving revision 1.324
diff -c -r1.324 builtins.h
*** src/include/utils/builtins.h 13 Oct 2008 16:25:20 -0000 1.324
--- src/include/utils/builtins.h 29 Oct 2008 19:26:55 -0000
***************
*** 899,904 ****
--- 899,907 ----
extern Datum RI_FKey_setdefault_del(PG_FUNCTION_ARGS);
extern Datum RI_FKey_setdefault_upd(PG_FUNCTION_ARGS);
+ /* trigfuncs.c */
+ extern Datum suppress_redundant_updates_trigger(PG_FUNCTION_ARGS);
+
/* encoding support functions */
extern Datum getdatabaseencoding(PG_FUNCTION_ARGS);
extern Datum database_character_set(PG_FUNCTION_ARGS);
Index: src/test/regress/expected/triggers.out
===================================================================
RCS file: /cvsroot/pgsql/src/test/regress/expected/triggers.out,v
retrieving revision 1.24
diff -c -r1.24 triggers.out
*** src/test/regress/expected/triggers.out 1 Feb 2007 19:10:30 -0000 1.24
--- src/test/regress/expected/triggers.out 29 Oct 2008 19:26:56 -0000
***************
*** 537,539 ****
--- 537,564 ----
NOTICE: row 2 not changed
DROP TABLE trigger_test;
DROP FUNCTION mytrigger();
+ -- minimal update trigger
+ CREATE TABLE min_updates_test (
+ f1 text,
+ f2 int,
+ f3 int);
+ INSERT INTO min_updates_test VALUES ('a',1,2),('b','2',null);
+ CREATE TRIGGER z_min_update
+ BEFORE UPDATE ON min_updates_test
+ FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();
+ \set QUIET false
+ UPDATE min_updates_test SET f1 = f1;
+ UPDATE 0
+ UPDATE min_updates_test SET f2 = f2 + 1;
+ UPDATE 2
+ UPDATE min_updates_test SET f3 = 2 WHERE f3 is null;
+ UPDATE 1
+ \set QUIET true
+ SELECT * FROM min_updates_test;
+ f1 | f2 | f3
+ ----+----+----
+ a | 2 | 2
+ b | 3 | 2
+ (2 rows)
+
+ DROP TABLE min_updates_test;
Index: src/test/regress/sql/triggers.sql
===================================================================
RCS file: /cvsroot/pgsql/src/test/regress/sql/triggers.sql,v
retrieving revision 1.13
diff -c -r1.13 triggers.sql
*** src/test/regress/sql/triggers.sql 26 Jun 2006 17:24:41 -0000 1.13
--- src/test/regress/sql/triggers.sql 29 Oct 2008 19:26:56 -0000
***************
*** 415,417 ****
--- 415,446 ----
DROP TABLE trigger_test;
DROP FUNCTION mytrigger();
+
+
+ -- minimal update trigger
+
+ CREATE TABLE min_updates_test (
+ f1 text,
+ f2 int,
+ f3 int);
+
+ INSERT INTO min_updates_test VALUES ('a',1,2),('b','2',null);
+
+ CREATE TRIGGER z_min_update
+ BEFORE UPDATE ON min_updates_test
+ FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();
+
+ \set QUIET false
+
+ UPDATE min_updates_test SET f1 = f1;
+
+ UPDATE min_updates_test SET f2 = f2 + 1;
+
+ UPDATE min_updates_test SET f3 = 2 WHERE f3 is null;
+
+ \set QUIET true
+
+ SELECT * FROM min_updates_test;
+
+ DROP TABLE min_updates_test;
+
Andrew Dunstan wrote: > > > Kenneth Marshall wrote: >> On Wed, Oct 22, 2008 at 06:05:26PM -0400, Tom Lane wrote: >> >>> Simon Riggs <simon@2ndQuadrant.com> writes: >>> >>>>> On Wed, Oct 22, 2008 at 3:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>>>> >>>>>> "Minimal" really fails to convey the point here IMHO. How about >>>>>> something like "suppress_no_op_updates_trigger"? >>>>>> >>>> I think it means something to us, but "no op" is a very technical >>>> phrase >>>> that probably doesn't travel very well. >>>> >>> Agreed --- I was hoping someone could improve on that part. The only >>> other words I could come up with were "empty" and "useless", neither of >>> which seem quite le mot juste ... >>> >>> regards, tom lane >>> >>> >> redundant? >> >> >> > > I think I like this best of all the suggestions - > suppress_redundant_updates_trigger() is what I have now. > > If there's no further discussion, I'll go ahead and commit this in a day > or two. Nitpicking, but you have: + <para> + Currently <productname>PostgreSQL</> provides one built in trigger + function, <function>suppress_redundant_updates_trigger</>, Should we perhaps mention the fulltext triggers (with the appropriate links) here? If it's intended to be an authoritative list of the "userspace" triggers we ship, I think that may be a good idea. //Magnus
Andrew Dunstan escribió: > + /* make sure it's called as a trigger */ > + if (!CALLED_AS_TRIGGER(fcinfo)) > + elog(ERROR, "suppress_redundant_updates_trigger: must be called as trigger"); Shouldn't these all be ereport()? -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc.
Alvaro Herrera wrote: > Andrew Dunstan escribió: > > >> + /* make sure it's called as a trigger */ >> + if (!CALLED_AS_TRIGGER(fcinfo)) >> + elog(ERROR, "suppress_redundant_updates_trigger: must be called as trigger"); >> > > Shouldn't these all be ereport()? > > Good point. I'll fix them. Maybe we should fix our C sample trigger, from which this was taken. cheers andrew
On Wed, Oct 29, 2008 at 03:48:09PM -0400, Andrew Dunstan wrote: > >>> + /* make sure it's called as a trigger */ >>> + if (!CALLED_AS_TRIGGER(fcinfo)) >>> + elog(ERROR, "suppress_redundant_updates_trigger: must be called as trigger"); >> >> Shouldn't these all be ereport()? > > Good point. > > I'll fix them. > > Maybe we should fix our C sample trigger, from which this was taken. Yes :) Does the attached have the right error code? Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Attachment
David Fetter wrote:
>>
>> Maybe we should fix our C sample trigger, from which this was taken.
>>
>
> Yes :)
>
> Does the attached have the right error code?
>
> - elog(ERROR, "trigf: not called by trigger manager");
> + ereport(ERROR,
> + (error(TRIGGERED_DATA_CHANGE_VIOLATION),
> + errmsg("trigf: not called by trigger manager")));
>
>
Not sure that's appropriate, but I can't see anything else that is very
appropriate either.
cheers
andrew
Andrew Dunstan wrote:
>
>
> David Fetter wrote:
>>>
>>> Maybe we should fix our C sample trigger, from which this was taken.
>>>
>>
>> Yes :)
>>
>> Does the attached have the right error code?
>>
>> - elog(ERROR, "trigf: not called by trigger manager");
>> + ereport(ERROR,
>> + (error(TRIGGERED_DATA_CHANGE_VIOLATION),
>> + errmsg("trigf: not called by trigger manager")));
>>
>>
>
> Not sure that's appropriate, but I can't see anything else that is
> very appropriate either.
The plpgsql code uses errcode(ERRCODE_FEATURE_NOT_SUPPORTED) for this
situation, so I guess we should be consistent with that.
cheers
andrew
Andrew Dunstan <andrew@dunslane.net> writes:
>> Not sure that's appropriate, but I can't see anything else that is
>> very appropriate either.
> The plpgsql code uses errcode(ERRCODE_FEATURE_NOT_SUPPORTED) for this
> situation, so I guess we should be consistent with that.
TRIGGERED_DATA_CHANGE_VIOLATION is most certainly NOT an appropriate
code here --- it's talking about invalid database content states.
The RI triggers use ERRCODE_E_R_I_E_TRIGGER_PROTOCOL_VIOLATED for these
sorts of conditions, and I think that's probably best practice. See
ri_CheckTrigger() in particular.
regards, tom lane
Andrew Dunstan <andrew@dunslane.net> writes:
> I think I like this best of all the suggestions -
> suppress_redundant_updates_trigger() is what I have now.
> If there's no further discussion, I'll go ahead and commit this in a day
> or two.
The documentation seems a bit lacking: it gives neither a hint of why
you might want to use this nor why it's not the built-in behavior.
Suggest expending a sentence or two pointing out that the trigger takes
nonzero execution time to do its comparisons, and that this may or may
not be repaid by eliminated updates, depending on whether the client
applications are actually in the habit of issuing useless update
commands.
I think you're missing an <indexentry> item for the function name, also.
regards, tom lane