Content-Type: multipart/mixed; boundary="------------D6IFwAhHL6YBCxxNAjxNmdOJ"
Message-ID: <f5ff446f-72e2-44f7-80c6-4256cf7caeb3@gmail.com>
Date: Thu, 20 Apr 2023 13:00:00 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: BUG #17798: Incorrect memory access occurs when using BEFORE ROW
 UPDATE trigger
Content-Language: en-US
To: Richard Guo <guofenglinux@gmail.com>
Cc: pgsql-bugs@lists.postgresql.org
References: <17798-0907404928dcf0dd@postgresql.org>
 <87fe6c28-80b2-f709-a8db-d105e5240e39@gmail.com>
 <CAMbWs4_pg7ab=OJF2CGLzs=noWEjujerxfo92sm3RL9ce=ogsQ@mail.gmail.com>
 <CAMbWs48FHhuuuWbxygO4Wey7ahdaSwRjzJODKXx7=GFsge_aLg@mail.gmail.com>
From: Alexander Lakhin <exclusion@gmail.com>
In-Reply-To: 
 <CAMbWs48FHhuuuWbxygO4Wey7ahdaSwRjzJODKXx7=GFsge_aLg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/f5ff446f-72e2-44f7-80c6-4256cf7caeb3%40gmail.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------D6IFwAhHL6YBCxxNAjxNmdOJ
Content-Type: multipart/alternative;
 boundary="------------lcTzTIO0oUGhx8eHuz9Y0A0Z"

--------------lcTzTIO0oUGhx8eHuz9Y0A0Z
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

Hello Richard,

19.04.2023 11:29, Richard Guo wrote:
> I have a second look at this issue and now I think the fix in v1 patch
> is correct.  I think the comment needs to be updated for this change,
> maybe something like
>
>   * (Typically, newslot was also generated by ExecGetUpdateNewTuple, so
>   * that epqslot_clean will be that same slot and the copy step below
> - * is not needed.)
> + * is not needed.  And we need to materialize newslot in this case,
> + * since its tuple might be dependent on oldslot's storage, which
> + * might not be a local copy and be freed before we fetch newslot's
> + * tuple.)

Thanks for looking into this!

I've updated the comment based on your suggestion. Please look at the patch
attached.

I hesitate to add the isolation test to the patch as adding a dedicated
test just for this case seems too wasteful to me. Maybe that scenario could
be added to one of the existing trigger-related tests, but I couldn't find a
test, which is generic enough for that.

Best regards,
Alexander
--------------lcTzTIO0oUGhx8eHuz9Y0A0Z
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit

<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <div class="moz-cite-prefix">Hello Richard,<br>
      <br>
      19.04.2023 11:29, Richard Guo wrote:<br>
    </div>
    <blockquote type="cite"
cite="mid:CAMbWs48FHhuuuWbxygO4Wey7ahdaSwRjzJODKXx7=GFsge_aLg@mail.gmail.com">
      <meta http-equiv="content-type" content="text/html; charset=UTF-8">
      <div dir="ltr">I have a second look at this issue and now I think
        the fix in v1 patch<br>
        <div class="gmail_quote">
          <div>is correct.  I think the comment needs to be updated for
            this change,<br>
            maybe something like<br>
            <br>
              * (Typically, newslot was also generated by
            ExecGetUpdateNewTuple, so<br>
              * that epqslot_clean will be that same slot and the copy
            step below<br>
            - * is not needed.)<br>
            + * is not needed.  And we need to materialize newslot in
            this case,<br>
            + * since its tuple might be dependent on oldslot's storage,
            which<br>
            + * might not be a local copy and be freed before we fetch
            newslot's<br>
            + * tuple.)<br>
          </div>
        </div>
      </div>
    </blockquote>
    <br>
    Thanks for looking into this!<br>
    <br>
    I've updated the comment based on your suggestion. Please look at
    the patch<br>
    attached.<br>
    <br>
    I hesitate to add the isolation test to the patch as adding a
    dedicated<br>
    test just for this case seems too wasteful to me. Maybe that
    scenario could<br>
    be added to one of the existing trigger-related tests, but I
    couldn't find a<br>
    test, which is generic enough for that.<br>
    <br>
    Best regards,<br>
    Alexander<br>
  </body>
</html>

--------------lcTzTIO0oUGhx8eHuz9Y0A0Z--

--------------D6IFwAhHL6YBCxxNAjxNmdOJ
Content-Type: text/x-patch; charset=UTF-8;
 name="v2-01-Fix-memory-access-in-BRU-trigger.patch"
Content-Disposition: attachment;
 filename="v2-01-Fix-memory-access-in-BRU-trigger.patch"
Content-Transfer-Encoding: base64

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2NvbW1hbmRzL3RyaWdnZXIuYyBiL3NyYy9iYWNr
ZW5kL2NvbW1hbmRzL3RyaWdnZXIuYwppbmRleCA0YjI5NWY4ZGE1Li44MDI3NDk4YjczIDEw
MDY0NAotLS0gYS9zcmMvYmFja2VuZC9jb21tYW5kcy90cmlnZ2VyLmMKKysrIGIvc3JjL2Jh
Y2tlbmQvY29tbWFuZHMvdHJpZ2dlci5jCkBAIC0yOTgxLDcgKzI5ODEsOSBAQCBFeGVjQlJV
cGRhdGVUcmlnZ2VycyhFU3RhdGUgKmVzdGF0ZSwgRVBRU3RhdGUgKmVwcXN0YXRlLAogCQkg
KgogCQkgKiAoVHlwaWNhbGx5LCBuZXdzbG90IHdhcyBhbHNvIGdlbmVyYXRlZCBieSBFeGVj
R2V0VXBkYXRlTmV3VHVwbGUsIHNvCiAJCSAqIHRoYXQgZXBxc2xvdF9jbGVhbiB3aWxsIGJl
IHRoYXQgc2FtZSBzbG90IGFuZCB0aGUgY29weSBzdGVwIGJlbG93Ci0JCSAqIGlzIG5vdCBu
ZWVkZWQuKQorCQkgKiBpcyBub3QgbmVlZGVkLiAgVGhvdWdoIHdlIG5lZWQgdG8gbWF0ZXJp
YWxpemUgbmV3c2xvdCBpbiB0aGlzIGNhc2UsCisJCSAqIHNpbmNlIGl0cyB0dXBsZSBtaWdo
dCBzaGFyZSB0aGUgb2xkc2xvdCdzIGJ1ZmZlciwgd2hpY2ggbWlnaHQKKwkJICogYmUgZnJl
ZWQgb24gZmV0Y2hpbmcgdHJpZ3R1cGxlIGZyb20gb2xkc2xvdC4pCiAJCSAqLwogCQlpZiAo
ZXBxc2xvdF9jYW5kaWRhdGUgIT0gTlVMTCkKIAkJewpAQCAtMjk5Miw2ICsyOTk0LDggQEAg
RXhlY0JSVXBkYXRlVHJpZ2dlcnMoRVN0YXRlICplc3RhdGUsIEVQUVN0YXRlICplcHFzdGF0
ZSwKIAogCQkJaWYgKG5ld3Nsb3QgIT0gZXBxc2xvdF9jbGVhbikKIAkJCQlFeGVjQ29weVNs
b3QobmV3c2xvdCwgZXBxc2xvdF9jbGVhbik7CisJCQllbHNlCisJCQkJRXhlY01hdGVyaWFs
aXplU2xvdChuZXdzbG90KTsKIAkJfQogCiAJCXRyaWd0dXBsZSA9IEV4ZWNGZXRjaFNsb3RI
ZWFwVHVwbGUob2xkc2xvdCwgdHJ1ZSwgJnNob3VsZF9mcmVlX3RyaWcpOwo=

--------------D6IFwAhHL6YBCxxNAjxNmdOJ--