Re: 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(at)ak(dot)jp(dot)nec(dot)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 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?
>
>
>

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

Andrew Dunstan <andrew(at)dunslane(dot)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(at)dunslane(dot)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(at)dunslane(dot)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(at)dunslane(dot)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(at)dunslane(dot)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(at)dunslane(dot)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)
>
>
>

Your wish is my command. Done.

cheers

andrew

Andrew Dunstan wrote:
>
>
> Tom Lane wrote:
>> Andrew Dunstan <andrew(at)dunslane(dot)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(at)ak(dot)jp(dot)nec(dot)com>

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(at)ak(dot)jp(dot)nec(dot)com>

KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)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(at)ak(dot)jp(dot)nec(dot)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(at)kaigai(dot)gr(dot)jp>

KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)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(at)ak(dot)jp(dot)nec(dot)com>