Subject: =?UTF-8?B?UmU6IOWbnuWkje+8mmhvdyB0byBjcmVhdGUgaW5kZXggY29uY3VycmVu?=
 =?UTF-8?Q?tly_on_partitioned_table?=
To: Michael Paquier <michael@paquier.xyz>,
 Justin Pryzby <pryzby@telsasoft.com>
Cc: =?UTF-8?B?5p2O5p2wKOaFjui/vSk=?= <adger.lj@alibaba-inc.com>,
 pgsql-hackers <pgsql-hackers@lists.postgresql.org>,
 =?UTF-8?B?5pu+5paH5peMKOS5ieS7jik=?= <wenjing.zwj@alibaba-inc.com>,
 Alvaro Herrera <alvherre@2ndquadrant.com>
References: <20200618024114.GF118592@paquier.xyz>
 <20200616010221.GA2353@paquier.xyz> <20200615123742.GL52676@paquier.xyz>
 <20200808063744.GG20473@telsasoft.com> <20200809050009.GA17986@paquier.xyz>
 <20200809234423.GH20473@telsasoft.com> <20200812045438.GA11663@paquier.xyz>
 <20200812052820.GW20473@telsasoft.com> <20200812133708.GB11663@paquier.xyz>
 <20200814002945.GB2057@paquier.xyz> <20200902013944.GF1489@paquier.xyz>
From: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>
Message-ID: <4cefde7e-cc68-2e0c-077b-d6fe3a220344@postgrespro.ru>
Date: Thu, 3 Sep 2020 22:02:58 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20200902013944.GF1489@paquier.xyz>
Content-Type: multipart/alternative;
 boundary="------------7DBA1B340A77DDC0BB814372"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------7DBA1B340A77DDC0BB814372
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 8bit

On 02.09.2020 04:39, Michael Paquier wrote:
>
> The problem with dropped relations in REINDEX has been addressed by
> 1d65416, so I have gone through this patch again and simplified the
> use of session locks, these being taken only when doing a REINDEX
> CONCURRENTLY for a given partition.  This part is in a rather
> committable shape IMO, so I would like to get it done first, before
> looking more at the other cases with CIC and CLUSTER.  I am still
> planning to go through it once again.
> --
> Michael

Thank you for advancing this work.

I was reviewing the previous version, but the review became outdated 
before I sent it. Overall design is fine, but I see a bunch of things 
that need to be fixed before commit.

First of all, this patch fails at cfbot:

indexcmds.c:2848:7: error: variable ‘parentoid’ set but not used 
[-Werror=unused-but-set-variable]
Oid parentoid;^


It seems to be just a typo. With this minor fix the patch compiles and 
passes tests.

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 75008eebde..f5b3c53a83 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2864,7 +2864,7 @@ reindex_partitions(Oid relid, int options, bool concurrent,
  
                 /* Save partition OID */
                 old_context = MemoryContextSwitchTo(reindex_context);
-               partitions = lappend_oid(partitions, partoid);
+               partitions = lappend_oid(partitions, parentoid);
                 MemoryContextSwitchTo(old_context);
         }


If this guessed fix is correct, I see the problem in the patch logic. In 
reindex_partitions() we collect parent relations to pass them to 
reindex_multiple_internal(). It implicitly changes the logic from 
REINDEX INDEX to REINDEX RELATION, which is not the same, if table has 
more than one index. For example, if I add one more index to a 
partitioned table from a create_index.sql test:

create index idx on concur_reindex_part_0_2 using btree (c2);

and call

REINDEX INDEX CONCURRENTLY concur_reindex_part_index;

idx will be reindexed as well. I doubt that it is the desired behavior.


A few more random issues, that I noticed at first glance:

1) in create_index.sql

Are these two lines intentional checks that must fail? If so, I propose 
to add a comment.

REINDEX TABLE concur_reindex_part_index;
REINDEX TABLE CONCURRENTLY concur_reindex_part_index;

A few lines around also look like they were copy-pasted and need a 
second look.

2)  This part of ReindexRelationConcurrently() is misleading.

         case RELKIND_PARTITIONED_TABLE:
         case RELKIND_PARTITIONED_INDEX:
         default:
             /* Return error if type of relation is not supported */
             ereport(ERROR,
                     (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                      errmsg("cannot reindex this type of relation concurrently")));


Maybe assertion is enough. It seems, that we should never reach this 
code because both ReindexTable and ReindexIndex handle those relkinds 
separately.  Which leads me to the next question.

3) Is there a reason, why reindex_partitions() is not inside 
ReindexRelationConcurrently() ? I think that logically it belongs there.

4) I haven't tested multi-level partitions yet. In any case, it would be 
good to document this behavior explicitly.


I need a bit more time to review this patch more thoroughly. Please, 
wait for it, before committing.

-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


--------------7DBA1B340A77DDC0BB814372
Content-Type: text/html; charset=windows-1252
Content-Transfer-Encoding: 8bit

<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html;
      charset=windows-1252">
  </head>
  <body>
    <div class="moz-cite-prefix">On 02.09.2020 04:39, Michael Paquier
      wrote:<br>
    </div>
    <blockquote type="cite" cite="mid:20200902013944.GF1489@paquier.xyz"><br>
      <pre class="moz-quote-pre" wrap="">The problem with dropped relations in REINDEX has been addressed by
1d65416, so I have gone through this patch again and simplified the
use of session locks, these being taken only when doing a REINDEX
CONCURRENTLY for a given partition.  This part is in a rather
committable shape IMO, so I would like to get it done first, before
looking more at the other cases with CIC and CLUSTER.  I am still
planning to go through it once again.
--
Michael
</pre>
    </blockquote>
    <p>Thank you for advancing this work.<br>
      <br>
      I was reviewing the previous version, but the review became
      outdated before I sent it. Overall design is fine, but I see a
      bunch of things that need to be fixed before commit.<br>
      <br>
      First of all, this patch fails at cfbot:<br>
    </p>
    <pre id="log" class="ansi log-body-content"><div class="log-line"><div class="log-line"><span id="0-2129">indexcmds.c:2848:7: error: variable ‘parentoid’ set but not used [-Werror=unused-but-set-variable]</span>
</div><span id="0-2130">   Oid   parentoid;</span><span id="0-2131">       ^</span></div></pre>
    <p><br>
      It seems to be just a typo. With this minor fix the patch compiles
      and passes tests.<br>
    </p>
    <pre id="log" class="ansi log-body-content"><pre>
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 75008eebde..f5b3c53a83 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2864,7 +2864,7 @@ reindex_partitions(Oid relid, int options, bool concurrent,
 
                /* Save partition OID */
                old_context = MemoryContextSwitchTo(reindex_context);
-               partitions = lappend_oid(partitions, partoid);
+               partitions = lappend_oid(partitions, parentoid);
                MemoryContextSwitchTo(old_context);
        }</pre></pre>
    <br>
    If this guessed fix is correct, I see the problem in the patch
    logic. In reindex_partitions() we collect parent relations to pass
    them to reindex_multiple_internal(). It implicitly changes the logic
    from REINDEX INDEX to REINDEX RELATION, which is not the same, if
    table has more than one index. For example, if I add one more index
    to a partitioned table from a create_index.sql test:<br>
    <pre id="log" class="ansi log-body-content"><pre>create index idx on concur_reindex_part_0_2 using btree (c2);
</pre></pre>
    <p>and call<br>
    </p>
    <pre id="log" class="ansi log-body-content"><pre>REINDEX INDEX CONCURRENTLY concur_reindex_part_index;
</pre></pre>
    idx will be reindexed as well. I doubt that it is the desired
    behavior.<br>
    <p><br>
      A few more random issues, that I noticed at first glance:<br>
      <br>
      1) in create_index.sql<br>
      <br>
      Are these two lines intentional checks that must fail? If so, I
      propose to add a comment.<br>
      <br>
      REINDEX TABLE concur_reindex_part_index;<br>
      REINDEX TABLE CONCURRENTLY concur_reindex_part_index;<br>
      <br>
      A few lines around also look like they were copy-pasted and need a
      second look.<br>
      <br>
      2)  This part of ReindexRelationConcurrently() is misleading. <br>
      <br>
    </p>
    <pre id="log" class="ansi log-body-content"><pre>        case RELKIND_PARTITIONED_TABLE:
        case RELKIND_PARTITIONED_INDEX:
        default:
            /* Return error if type of relation is not supported */
            ereport(ERROR,
                    (errcode(ERRCODE_WRONG_OBJECT_TYPE),
                     errmsg("cannot reindex this type of relation concurrently")));</pre></pre>
    <p><br>
      Maybe assertion is enough. It seems, that we should never reach
      this code because both ReindexTable and ReindexIndex handle those
      relkinds separately.  Which leads me to the next question.<br>
      <br>
      3) Is there a reason, why reindex_partitions() is not inside
      ReindexRelationConcurrently() ? I think that logically it belongs
      there.<br>
      <br>
      4) I haven't tested multi-level partitions yet. In any case, it
      would be good to document this behavior explicitly.<br>
      <br>
      <br>
      I need a bit more time to review this patch more thoroughly.
      Please, wait for it, before committing.</p>
    <pre class="moz-signature" cols="72">-- 
Anastasia Lubennikova
Postgres Professional: <a class="moz-txt-link-freetext" href="http://www.postgrespro.com">http://www.postgrespro.com</a>
The Russian Postgres Company</pre>
  </body>
</html>

--------------7DBA1B340A77DDC0BB814372--