minimal update

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
DO INSTEAD 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

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 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 <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

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:

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:

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:

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

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.com
PostgreSQL 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.com
PostgreSQL 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

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

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