Content-Type: multipart/alternative;
	boundary="------------0zF61ZQaZnE40CZfi58XTInL"
Message-ID: <fa936b64-f228-dd58-c0bd-44f19d43d5f6@amazon.com>
Date: Wed, 7 Sep 2022 10:48:39 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.2.1
From: "Drouvot, Bertrand" <bdrouvot@amazon.com>
Subject: Re: ALTER TABLE uses a bistate but not for toast tables
To: Justin Pryzby <pryzby@telsasoft.com>, <pgsql-hackers@postgresql.org>
References: <20220622143841.GS22452@telsasoft.com>
Content-Language: en-US
In-Reply-To: <20220622143841.GS22452@telsasoft.com>
Archived-At: 
 <https://www.postgresql.org/message-id/fa936b64-f228-dd58-c0bd-44f19d43d5f6%40amazon.com>
Precedence: bulk

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

Hi,

On 6/22/22 4:38 PM, Justin Pryzby wrote:
> ATRewriteTable() calls table_tuple_insert() with a bistate, to avoid clobbering
> and polluting the buffers.
>
> But heap_insert() then calls
> heap_prepare_insert() >
> heap_toast_insert_or_update >
> toast_tuple_externalize >
> toast_save_datum >
> heap_insert(toastrel, toasttup, mycid, options, NULL /* without bistate:( */);

Good catch!

> I came up with this patch.

+       /* Release pin after main table, before switching to write to 
toast table */
+       if (bistate)
+               ReleaseBulkInsertStatePin(bistate);

I'm not sure we should release and reuse here the bistate of the main 
table: it looks like that with the patch ReadBufferBI() on the main 
relation wont have the desired block already pinned (then would need to 
perform a read).

What do you think about creating earlier a new dedicated bistate for the 
toast table?

+       if (bistate)
+       {
+               table_finish_bulk_insert(toastrel, options); // XXX

I think it's too early, as it looks to me that at this stage we may have 
not finished the whole bulk insert yet.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services:https://aws.amazon.com

--------------0zF61ZQaZnE40CZfi58XTInL
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 text="#415ce2" bgcolor="#FFFFFF">
    <p>Hi,<br>
    </p>
    <div class="moz-cite-prefix">On 6/22/22 4:38 PM, Justin Pryzby
      wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:20220622143841.GS22452@telsasoft.com">
      <pre class="moz-quote-pre" wrap="">ATRewriteTable() calls table_tuple_insert() with a bistate, to avoid clobbering
and polluting the buffers.

But heap_insert() then calls
heap_prepare_insert() &gt;
heap_toast_insert_or_update &gt;
toast_tuple_externalize &gt;
toast_save_datum &gt;
heap_insert(toastrel, toasttup, mycid, options, NULL /* without bistate:( */);</pre>
    </blockquote>
    <p>Good catch! </p>
    <blockquote type="cite"
      cite="mid:20220622143841.GS22452@telsasoft.com">
      <pre class="moz-quote-pre" wrap="">I came up with this patch.  </pre>
    </blockquote>
    <p>+       /* Release pin after main table, before switching to
      write to toast table */<br>
      +       if (bistate)<br>
      +               ReleaseBulkInsertStatePin(bistate);</p>
    <p>I'm not sure we should release and reuse here the bistate of the
      main table: it looks like that with the patch ReadBufferBI() on
      the main relation wont have the desired block already pinned (then
      would need to perform a read).<br>
    </p>
    <p>What do you think about creating earlier a new dedicated bistate
      for the toast table?<br>
    </p>
    <p>+       if (bistate)<br>
      +       {<br>
      +               table_finish_bulk_insert(toastrel, options); //
      XXX</p>
    <p>I think it's too early, as it looks to me that at this stage we
      may have not finished the whole bulk insert yet.</p>
    <p>Regards,<br>
    </p>
    <pre class="moz-signature" cols="72">-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: <a class="moz-txt-link-freetext" href="https://aws.amazon.com">https://aws.amazon.com</a></pre>
  </body>
</html>

--------------0zF61ZQaZnE40CZfi58XTInL--