The suppress_redundant_updates_trigger() works incorrectly
Hi,
The suppress_redundant_updates_trigger() works incorrectly
on the table defined with "WITH_OIDS" option.
----------
(*) The latest 8.4devel tree without SE-PostgreSQL patch
postgres=# CREATE TABLE min_updates_test (
f1 text,
f2 int,
f3 int) with oids;
CREATE TABLE ^^^^^^^^^ <- Here is different from the regression test.
postgres=# INSERT INTO min_updates_test VALUES ('a',1,2),('b','2',null);
INSERT 0 2
postgres=# CREATE TRIGGER z_min_update
BEFORE UPDATE ON min_updates_test
FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();
CREATE TRIGGER
postgres=# UPDATE min_updates_test SET f1 = f1;
UPDATE 2
----------
The current version does not allow to update the "oid", so the older
value is preserved implicitly. However, it is done at heap_update().
Before-row-trigger functions are invoked before heap_update(), so
the field to store the "oid" is empty (InvalidOid) at the moment.
Then, suppress_redundant_updates_trigger() makes a decision there is
a difference between old and new versions.
It seems to me the older value has to be preserved just before
invocation of row-trigger functions.
Any comment?
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
KaiGai Kohei wrote:
Hi,
The suppress_redundant_updates_trigger() works incorrectly
on the table defined with "WITH_OIDS" option.----------
(*) The latest 8.4devel tree without SE-PostgreSQL patchpostgres=# CREATE TABLE min_updates_test (
f1 text,
f2 int,
f3 int) with oids;
CREATE TABLE ^^^^^^^^^ <- Here is different from the regression test.
postgres=# INSERT INTO min_updates_test VALUES ('a',1,2),('b','2',null);
INSERT 0 2
postgres=# CREATE TRIGGER z_min_update
BEFORE UPDATE ON min_updates_test
FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();
CREATE TRIGGER
postgres=# UPDATE min_updates_test SET f1 = f1;
UPDATE 2
----------The current version does not allow to update the "oid", so the older
value is preserved implicitly. However, it is done at heap_update().
Before-row-trigger functions are invoked before heap_update(), so
the field to store the "oid" is empty (InvalidOid) at the moment.
Then, suppress_redundant_updates_trigger() makes a decision there is
a difference between old and new versions.It seems to me the older value has to be preserved just before
invocation of row-trigger functions.
Any comment?
Good catch!
I think ideally we'd just adjust the comparison to avoid the system
attributes.
I'll have a look to see if it can be done simply.
cheers
andrew
Andrew Dunstan wrote:
KaiGai Kohei wrote:
Hi,
The suppress_redundant_updates_trigger() works incorrectly
on the table defined with "WITH_OIDS" option.Good catch!
I think ideally we'd just adjust the comparison to avoid the system
attributes.I'll have a look to see if it can be done simply.
The attached patch sets the OID to InvalidOid for the duration of the
memcmp if the HEAP_HASOID flag is set, and restores it afterwards.
That seems to handle the problem.
There's also an added regression test for the Oids case.
If there's no objection I'll apply this shortly.
cheers
andrew
Attachments:
min-update4.patchtext/x-patch; charset=iso-8859-1; name=min-update4.patchDownload
Index: src/backend/utils/adt/trigfuncs.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/trigfuncs.c,v
retrieving revision 1.2
diff -c -r1.2 trigfuncs.c
*** src/backend/utils/adt/trigfuncs.c 4 Nov 2008 00:29:39 -0000 1.2
--- src/backend/utils/adt/trigfuncs.c 5 Nov 2008 17:30:26 -0000
***************
*** 30,35 ****
--- 30,36 ----
TriggerData *trigdata = (TriggerData *) fcinfo->context;
HeapTuple newtuple, oldtuple, rettuple;
HeapTupleHeader newheader, oldheader;
+ Oid oldoid = InvalidOid;
/* make sure it's called as a trigger */
if (!CALLED_AS_TRIGGER(fcinfo))
***************
*** 62,67 ****
--- 63,74 ----
newheader = newtuple->t_data;
oldheader = oldtuple->t_data;
+ if (oldheader->t_infomask & HEAP_HASOID)
+ {
+ oldoid = HeapTupleHeaderGetOid(oldheader);
+ HeapTupleHeaderSetOid(oldheader, InvalidOid);
+ }
+
/* if the tuple payload is the same ... */
if (newtuple->t_len == oldtuple->t_len &&
newheader->t_hoff == oldheader->t_hoff &&
***************
*** 76,81 ****
--- 83,93 ----
/* ... then suppress the update */
rettuple = NULL;
}
+ else if (oldheader->t_infomask & HEAP_HASOID)
+ {
+ HeapTupleHeaderSetOid(oldheader, oldoid);
+ }
+
return PointerGetDatum(rettuple);
}
Index: src/test/regress/expected/triggers.out
===================================================================
RCS file: /cvsroot/pgsql/src/test/regress/expected/triggers.out,v
retrieving revision 1.25
diff -c -r1.25 triggers.out
*** src/test/regress/expected/triggers.out 3 Nov 2008 20:17:20 -0000 1.25
--- src/test/regress/expected/triggers.out 5 Nov 2008 17:30:27 -0000
***************
*** 542,551 ****
--- 542,559 ----
f1 text,
f2 int,
f3 int);
+ CREATE TABLE min_updates_test_oids (
+ f1 text,
+ f2 int,
+ f3 int) WITH OIDS;
INSERT INTO min_updates_test VALUES ('a',1,2),('b','2',null);
+ INSERT INTO min_updates_test_oids 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();
+ CREATE TRIGGER z_min_update
+ BEFORE UPDATE ON min_updates_test_oids
+ FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();
\set QUIET false
UPDATE min_updates_test SET f1 = f1;
UPDATE 0
***************
*** 553,558 ****
--- 561,572 ----
UPDATE 2
UPDATE min_updates_test SET f3 = 2 WHERE f3 is null;
UPDATE 1
+ UPDATE min_updates_test_oids SET f1 = f1;
+ UPDATE 0
+ UPDATE min_updates_test_oids SET f2 = f2 + 1;
+ UPDATE 2
+ UPDATE min_updates_test_oids SET f3 = 2 WHERE f3 is null;
+ UPDATE 1
\set QUIET true
SELECT * FROM min_updates_test;
f1 | f2 | f3
***************
*** 561,564 ****
--- 575,586 ----
b | 3 | 2
(2 rows)
+ SELECT * FROM min_updates_test_oids;
+ f1 | f2 | f3
+ ----+----+----
+ a | 2 | 2
+ b | 3 | 2
+ (2 rows)
+
DROP TABLE min_updates_test;
+ DROP TABLE min_updates_test_oids;
Index: src/test/regress/sql/triggers.sql
===================================================================
RCS file: /cvsroot/pgsql/src/test/regress/sql/triggers.sql,v
retrieving revision 1.14
diff -c -r1.14 triggers.sql
*** src/test/regress/sql/triggers.sql 3 Nov 2008 20:17:21 -0000 1.14
--- src/test/regress/sql/triggers.sql 5 Nov 2008 17:30:27 -0000
***************
*** 424,435 ****
--- 424,446 ----
f2 int,
f3 int);
+ CREATE TABLE min_updates_test_oids (
+ f1 text,
+ f2 int,
+ f3 int) WITH OIDS;
+
INSERT INTO min_updates_test VALUES ('a',1,2),('b','2',null);
+ INSERT INTO min_updates_test_oids 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();
+ CREATE TRIGGER z_min_update
+ BEFORE UPDATE ON min_updates_test_oids
+ FOR EACH ROW EXECUTE PROCEDURE suppress_redundant_updates_trigger();
+
\set QUIET false
UPDATE min_updates_test SET f1 = f1;
***************
*** 438,446 ****
--- 449,467 ----
UPDATE min_updates_test SET f3 = 2 WHERE f3 is null;
+ UPDATE min_updates_test_oids SET f1 = f1;
+
+ UPDATE min_updates_test_oids SET f2 = f2 + 1;
+
+ UPDATE min_updates_test_oids SET f3 = 2 WHERE f3 is null;
+
\set QUIET true
SELECT * FROM min_updates_test;
+ SELECT * FROM min_updates_test_oids;
+
DROP TABLE min_updates_test;
+ DROP TABLE min_updates_test_oids;
+
Andrew Dunstan <andrew@dunslane.net> writes:
The attached patch sets the OID to InvalidOid for the duration of the
memcmp if the HEAP_HASOID flag is set, and restores it afterwards.
This method is utterly, utterly unacceptable; you're probably trashing
the contents of a disk buffer there. Even assuming that there's zero
risk of a failure between the set and the restore, what if someone is in
process of writing the buffer to disk? Or even just examining the old
tuple?
regards, tom lane
I wrote:
This method is utterly, utterly unacceptable; you're probably trashing
the contents of a disk buffer there.
... however, it seems reasonable to assume that the *new* tuple is just
local storage. Why don't you just poke the old tuple's OID into the new
one before comparing?
regards, tom lane
Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
The attached patch sets the OID to InvalidOid for the duration of the
memcmp if the HEAP_HASOID flag is set, and restores it afterwards.This method is utterly, utterly unacceptable; you're probably trashing
the contents of a disk buffer there. Even assuming that there's zero
risk of a failure between the set and the restore, what if someone is in
process of writing the buffer to disk? Or even just examining the old
tuple?
OK, I guess I assumed we had a private copy.
Next thought is to split the memcmp() into two to avoid the Oid field,
where it's present. I was hoping to avoid the kinda ugly arithmetic.
cheers
andrew
Tom Lane wrote:
I wrote:
This method is utterly, utterly unacceptable; you're probably trashing
the contents of a disk buffer there.... however, it seems reasonable to assume that the *new* tuple is just
local storage. Why don't you just poke the old tuple's OID into the new
one before comparing?
OK, that will be easy enough. I assume I should still put InvalidOid
back again afterwards, in case someone downstream relies on it.
cheers
andrew
Andrew Dunstan <andrew@dunslane.net> writes:
Tom Lane wrote:
... however, it seems reasonable to assume that the *new* tuple is just
local storage. Why don't you just poke the old tuple's OID into the new
one before comparing?
OK, that will be easy enough. I assume I should still put InvalidOid
back again afterwards, in case someone downstream relies on it.
Can't imagine what ...
regards, tom lane
I wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
OK, that will be easy enough. I assume I should still put InvalidOid
back again afterwards, in case someone downstream relies on it.
Can't imagine what ...
Actually ... what *could* change in the future is that we might support
UPDATE'ing the OID to a different value. So what the code probably
needs to do is something like
if (relation->rd_rel->relhasoids &&
!OidIsValid(HeapTupleGetOid(newtup)))
HeapTupleSetOid(newtup, HeapTupleGetOid(oldtup));
(details stolen from heap_update)
regards, tom lane
Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
Tom Lane wrote:
... however, it seems reasonable to assume that the *new* tuple is just
local storage. Why don't you just poke the old tuple's OID into the new
one before comparing?OK, that will be easy enough. I assume I should still put InvalidOid
back again afterwards, in case someone downstream relies on it.Can't imagine what ...
OK, left off - anything for speed.
fix committed.
cheers
andrew
Tom Lane wrote:
I wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
OK, that will be easy enough. I assume I should still put InvalidOid
back again afterwards, in case someone downstream relies on it.Can't imagine what ...
Actually ... what *could* change in the future is that we might support
UPDATE'ing the OID to a different value. So what the code probably
needs to do is something likeif (relation->rd_rel->relhasoids &&
!OidIsValid(HeapTupleGetOid(newtup)))
HeapTupleSetOid(newtup, HeapTupleGetOid(oldtup));(details stolen from heap_update)
Your wish is my command. Done.
cheers
andrew
Andrew Dunstan wrote:
Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
Tom Lane wrote:
... however, it seems reasonable to assume that the *new* tuple is just
local storage. Why don't you just poke the old tuple's OID into the
new
one before comparing?OK, that will be easy enough. I assume I should still put InvalidOid
back again afterwards, in case someone downstream relies on it.Can't imagine what ...
OK, left off - anything for speed.
fix committed.
FYI, the reason why I noticed the behavior is SE-PostgreSQL also stores its
security attribute at the padding field of HeapTupleHeaderData, as "oid" doing.
Your fix can be also applied to preserve security attribute, however,
I reconsidered it is more preferable to separate its memcmp() into two
phases, the one is data fields, the other is system attributes, because
a fact of the field with InvalidOid shows the given query does not touch
the future writable system attribute, and it helps security modules to
check easiler whether the security attribute is tried to update, or not.
How do you think my approach within the attached patch?
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
Attachments:
suppress_redundant_updates_trigger.kaigai.patchtext/x-patch; name=suppress_redundant_updates_trigger.kaigai.patchDownload
*** base/src/backend/utils/adt/trigfuncs.c.orig 2008-11-06 10:24:39.000000000 +0900
--- base/src/backend/utils/adt/trigfuncs.c 2008-11-06 10:29:41.000000000 +0900
***************
*** 69,77 ****
* another OID value into newtuple. (That's not actually possible at
* present, but maybe someday.)
*/
! if (trigdata->tg_relation->rd_rel->relhasoids &&
! !OidIsValid(HeapTupleHeaderGetOid(newheader)))
! HeapTupleHeaderSetOid(newheader, HeapTupleHeaderGetOid(oldheader));
/* if the tuple payload is the same ... */
if (newtuple->t_len == oldtuple->t_len &&
--- 69,83 ----
* another OID value into newtuple. (That's not actually possible at
* present, but maybe someday.)
*/
! if (OidIsValid(HeapTupleGetOid(newtuple)) &&
! HeapTupleGetOid(newtuple) != HeapTupleGetOid(oldtuple))
! return PointerGetDatum(rettuple); /* anyway, to be updated */
!
! #if 0
! if (OidIsValid(HeapTupleGetSecurity(newtuple)) &&
! HeapTupleGetSecurity(newtuple) != HeapTupleGetSecurity(oldtuple))
! return PointerGetDatum(rettuple); /* anyway, to be updated */
! #endif
/* if the tuple payload is the same ... */
if (newtuple->t_len == oldtuple->t_len &&
***************
*** 80,88 ****
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)
{
/* ... then suppress the update */
rettuple = NULL;
--- 86,94 ----
HeapTupleHeaderGetNatts(oldheader)) &&
((newheader->t_infomask & ~HEAP_XACT_MASK) ==
(oldheader->t_infomask & ~HEAP_XACT_MASK)) &&
! memcmp(((char *)newheader) + newheader->t_hoff,
! ((char *)oldheader) + oldheader->t_hoff,
! newtuple->t_len - newheader->t_hoff) == 0)
{
/* ... then suppress the update */
rettuple = NULL;
KaiGai Kohei wrote:
*** 80,88 **** 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) { /* ... then suppress the update */ rettuple = NULL; --- 86,94 ---- HeapTupleHeaderGetNatts(oldheader)) && ((newheader->t_infomask & ~HEAP_XACT_MASK) == (oldheader->t_infomask & ~HEAP_XACT_MASK)) && ! memcmp(((char *)newheader) + newheader->t_hoff, ! ((char *)oldheader) + oldheader->t_hoff, ! newtuple->t_len - newheader->t_hoff) == 0) { /* ... then suppress the update */ rettuple = NULL;
Wouldn't this omit comparing the null bitmap?
cheers
andrew
Andrew Dunstan wrote:
KaiGai Kohei wrote:
*** 80,88 **** 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) { /* ... then suppress the update */ rettuple = NULL; --- 86,94 ---- HeapTupleHeaderGetNatts(oldheader)) && ((newheader->t_infomask & ~HEAP_XACT_MASK) == (oldheader->t_infomask & ~HEAP_XACT_MASK)) && ! memcmp(((char *)newheader) + newheader->t_hoff, ! ((char *)oldheader) + oldheader->t_hoff, ! newtuple->t_len - newheader->t_hoff) == 0) { /* ... then suppress the update */ rettuple = NULL;Wouldn't this omit comparing the null bitmap?
Oops, I added the comparison of null bitmap here.
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
Attachments:
suppress_redundant_updates_trigger.kaigai.2.patchtext/x-patch; name=suppress_redundant_updates_trigger.kaigai.2.patchDownload
*** base/src/backend/utils/adt/trigfuncs.c.orig 2008-11-06 10:24:39.000000000 +0900
--- base/src/backend/utils/adt/trigfuncs.c 2008-11-06 11:26:27.000000000 +0900
***************
*** 62,92 ****
newheader = newtuple->t_data;
oldheader = oldtuple->t_data;
- /*
- * We are called before the OID, if any, has been transcribed from the
- * old tuple to the new (in heap_update). To avoid a bogus compare
- * failure, copy the OID now. But check that someone didn't already put
- * another OID value into newtuple. (That's not actually possible at
- * present, but maybe someday.)
- */
- if (trigdata->tg_relation->rd_rel->relhasoids &&
- !OidIsValid(HeapTupleHeaderGetOid(newheader)))
- HeapTupleHeaderSetOid(newheader, HeapTupleHeaderGetOid(oldheader));
-
/* if the tuple payload is the same ... */
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)
{
/* ... then suppress the update */
rettuple = NULL;
}
return PointerGetDatum(rettuple);
}
--- 62,104 ----
newheader = newtuple->t_data;
oldheader = oldtuple->t_data;
/* if the tuple payload is the same ... */
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)))
{
+ if (HeapTupleHasNulls(newtuple) &&
+ memcmp(newheader->t_bits, oldheader->t_bits,
+ BITMAPLEN(HeapTupleHeaderGetNatts(newheader))) != 0)
+ goto skip;
+
+ /*
+ * We are called before the OID, if any, has been transcribed from
+ * the old tuple to the new (in heap_update). To avoid a bogus compare
+ * failure, compare the OID new. But check that someone didn't already
+ * put another OID value into newtuple. (That's not actually possible
+ * at present, but maybe someday.)
+ */
+ if (OidIsValid(HeapTupleGetOid(newtuple))
+ && HeapTupleGetOid(newtuple) != HeapTupleGetOid(oldtuple))
+ goto skip;
+ #if 0
+ if (OidIsValid(HeapTupleGetSecurity(newtuple))
+ && HeapTupleGetSecurity(newtuple) != HeapTupleGetSecurity(oldtuple))
+ goto skip;
+ #endif
+ if (memcmp(((char *)newheader) + newheader->t_hoff,
+ ((char *)oldheader) + oldheader->t_hoff,
+ newtuple->t_len - newheader->t_hoff) != 0)
+ goto skip;
+
/* ... then suppress the update */
rettuple = NULL;
}
+ skip:
return PointerGetDatum(rettuple);
}
KaiGai Kohei <kaigai@ak.jp.nec.com> writes:
Andrew Dunstan wrote:
Wouldn't this omit comparing the null bitmap?
Oops, I added the comparison of null bitmap here.
That's really, really ugly code. Why would it be necessary anyway?
Shouldn't the security tag be expected to match? I suppose that it
should be possible to alter a security tag with UPDATE, and that means
it cannot work the way OID does anyway. In a sane implementation the
field would already be valid before the triggers fire.
regards, tom lane
Tom Lane wrote:
KaiGai Kohei <kaigai@ak.jp.nec.com> writes:
Andrew Dunstan wrote:
Wouldn't this omit comparing the null bitmap?
Oops, I added the comparison of null bitmap here.
That's really, really ugly code. Why would it be necessary anyway?
Shouldn't the security tag be expected to match? I suppose that it
should be possible to alter a security tag with UPDATE, and that means
it cannot work the way OID does anyway. In a sane implementation the
field would already be valid before the triggers fire.
OK, I'll put a code to preserve it somewhere prior to triggers fire.
# Maybe, ExecBRUpdateTriggers()
However, I wonder if the oid field should be also preserved at same
place, not inside a specific trigger function.
What is your opinion?
Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
KaiGai Kohei <kaigai@kaigai.gr.jp> writes:
However, I wonder if the oid field should be also preserved at same
place, not inside a specific trigger function.
Possibly. I wasn't planning to mess with it now; but if you've fixed
the other problems with assigning to a system column then maybe we
should allow it for OIDs too.
regards, tom lane
Tom Lane wrote:
KaiGai Kohei <kaigai@kaigai.gr.jp> writes:
However, I wonder if the oid field should be also preserved at same
place, not inside a specific trigger function.Possibly. I wasn't planning to mess with it now; but if you've fixed
the other problems with assigning to a system column then maybe we
should allow it for OIDs too.
I moved the code to preserve system attributes in my tree, as follows:
http://code.google.com/p/sepgsql/source/detail?r=1190
The patch set does not allow to update "oid" column *now*, so the condition
of above if-block is always true. But the writable-system-attribute feature
can be implemented in similar way.
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>