Content-Type: multipart/alternative;
 boundary="------------74ttM5n8CE0re3hqjnxROLi0"
Message-ID: <8f487a28-a559-4fed-bffc-54a92a201fe9@gmail.com>
Date: Thu, 25 Jul 2024 22:21:49 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
From: Ilya Gladyshev <ilya.v.gladyshev@gmail.com>
Subject: Re: REINDEX not updating partition progress
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers@lists.postgresql.org
References: <b72f2d89-820a-4fa2-9058-b155cf646f4f@gmail.com>
 <Zpnaz_gHMyf0xsso@paquier.xyz>
 <779baf7f-6fc7-4431-a08e-eec63d84c6a3@gmail.com>
 <18D00CB1-D34B-4B71-AC65-96F27D69A9D8@gmail.com>
 <ZqIS96vKtMR4g3o7@paquier.xyz>
Content-Language: en-US
In-Reply-To: <ZqIS96vKtMR4g3o7@paquier.xyz>
Archived-At: <https://www.postgresql.org/message-id/8f487a28-a559-4fed-bffc-54a92a201fe9%40gmail.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------74ttM5n8CE0re3hqjnxROLi0
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


> 25 июля 2024 г., в 09:55, Michael Paquier <michael@paquier.xyz> 
> написал(а):
>
> On Sun, Jul 21, 2024 at 11:41:43AM +0100, Ilya Gladyshev wrote:
>> Forgot to update partition_relid in reindex_index in the second 
>> patch. Fixed in attachment.
>
>        <structfield>relid</structfield> <type>oid</type>
>       </para>
>       <para>
> -       OID of the table on which the index is being created.
> +       OID of the table on which the command was run.
>       </para></entry>
>
> Hmm.  I am not sure if we really need to change the definition of this
> field, because it can have the same meaning when using a REINDEX on a
> partitioned table, pointing to the parent table (the partition) of the
> index currently rebuilt.
>
> Hence, rather than a partition_relid, could a partitioned_relid
> reflect better the situation, set only when issuing a REINDEX on a
> partitioned relation?

I'm not quite happy with the documentation update, but I think the 
approach for partitioned tables in this patch makes sense. I checked 
what other commands, that deal with partitions, (CREATE INDEX and 
ANALYZE) do, and they put a root partitioned table in "relid". ANALYZE 
has a separate column for the id of partition named 
current_child_table_relid, so I think it makes sense to have REINDEX do 
the same.

In addition, the current API for progress tracking doesn't have a way of 
updating "relid" without wiping out all other fields (that's what 
pgstat_progress_start_command does). This can definitely be changed, but 
that's another thing that made me not think in this direction.

> +   if (relkind == RELKIND_PARTITIONED_INDEX)
> +   {
> +       heapId = IndexGetRelation(relid, true);
> +   }
> Shouldn't we report the partitioned index OID rather than its parent
> table when running a REINDEX on a partitioned index?
> —
> Michael

It’s used to update the "relid" field of the progress report. It’s the 
one that’s described in docs currently as "OID of the table on which the 
index is being created.", so I think it’s correct.

--------------74ttM5n8CE0re3hqjnxROLi0
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit

<!DOCTYPE html>
<html>
  <head>

    <meta http-equiv="content-type" content="text/html; charset=UTF-8">
  </head>
  <body dir="auto"
style="overflow-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;">
    <br id="lineBreakAtBeginningOfMessage">
    <div class="AppleOriginalContents" style="direction: ltr;"><br>
      <blockquote type="cite">
        <div>25 июля 2024 г., в 09:55, Michael Paquier
          <a class="moz-txt-link-rfc2396E" href="mailto:michael@paquier.xyz">&lt;michael@paquier.xyz&gt;</a> написал(а):</div>
        <br class="Apple-interchange-newline">
        <div>
          <div>On Sun, Jul 21, 2024 at 11:41:43AM +0100, Ilya Gladyshev
            wrote:<br>
            <blockquote type="cite">Forgot to update partition_relid in
              reindex_index in the second patch. Fixed in attachment.<br>
            </blockquote>
            <br>
                   &lt;structfield&gt;relid&lt;/structfield&gt;
            &lt;type&gt;oid&lt;/type&gt;<br>
                  &lt;/para&gt;<br>
                  &lt;para&gt;<br>
            -       OID of the table on which the index is being
            created.<br>
            +       OID of the table on which the command was run.<br>
                  &lt;/para&gt;&lt;/entry&gt;<br>
            <br>
            Hmm.  I am not sure if we really need to change the
            definition of this<br>
            field, because it can have the same meaning when using a
            REINDEX on a<br>
            partitioned table, pointing to the parent table (the
            partition) of the<br>
            index currently rebuilt.<br>
            <br>
            Hence, rather than a partition_relid, could a
            partitioned_relid<br>
            reflect better the situation, set only when issuing a
            REINDEX on a<br>
            partitioned relation?<br>
          </div>
        </div>
      </blockquote>
      <div class="AppleOriginalContents" style="direction: ltr;"><br>
      </div>
      <div class="AppleOriginalContents" style="direction: ltr;">I'm not
        quite happy with the documentation update, but I think the
        approach for partitioned tables in this patch makes sense. I
        checked what other commands, that deal with partitions, (CREATE
        INDEX and ANALYZE) do, and they put a root partitioned table in
        "relid". ANALYZE has a separate column for the id of partition
        named current_child_table_relid, so I think it makes sense to
        have REINDEX do the same.</div>
    </div>
    <div class="AppleOriginalContents" style="direction: ltr;"><br>
    </div>
    <div class="AppleOriginalContents" style="direction: ltr;">In
      addition, the current API for progress tracking doesn't have a way
      of updating "relid" without wiping out all other fields (that's
      what pgstat_progress_start_command does). This can definitely be
      changed, but that's another thing that made me not think in this
      direction.<br>
    </div>
    <div class="AppleOriginalContents" style="direction: ltr;"><br>
      <blockquote type="cite">
        <div>
          <div>+   if (relkind == RELKIND_PARTITIONED_INDEX)<br>
            +   {<br>
            +       heapId = IndexGetRelation(relid, true);<br>
            +   }<br>
            Shouldn't we report the partitioned index OID rather than
            its parent<br>
            table when running a REINDEX on a partitioned index?<br>
            —<br>
            Michael<br>
          </div>
        </div>
      </blockquote>
      <br>
    </div>
    <div class="AppleOriginalContents" style="direction: ltr;">It’s used
      to update the "relid" field of the progress report. It’s the one
      that’s described in docs currently as "OID of the table on which
      the index is being created.", so I think it’s correct.<br>
    </div>
    <br>
  </body>
</html>

--------------74ttM5n8CE0re3hqjnxROLi0--