Re: The suppress_redundant_updates_trigger() works incorrectly

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