Re: Support for REINDEX CONCURRENTLY

On Mon, Jun 17, 2013 at 9:23 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
>
>
>
> On Mon, Jun 17, 2013 at 5:23 AM, Andres Freund <andres@2ndquadrant.com>
> wrote:
>>
>> On 2013-06-17 04:20:03 +0900, Fujii Masao wrote:
>> > On Thu, Jun 6, 2013 at 1:29 PM, Michael Paquier
>> > <michael.paquier@gmail.com> wrote:
>> > > Hi all,
>> > >
>> > > Please find attached the latest versions of REINDEX CONCURRENTLY for
>> > > the 1st
>> > > commit fest of 9.4:
>> > > - 20130606_1_remove_reltoastidxid_v9.patch, removing reltoastidxid, to
>> > > allow
>> > > a toast relation to have multiple indexes running in parallel (extra
>> > > indexes
>> > > could be created by a REINDEX CONCURRENTLY processed)
>> > > - 20130606_2_reindex_concurrently_v26.patch, correcting some comments
>> > > and
>> > > fixed a lock in index_concurrent_create on an index relation not
>> > > released at
>> > > the end of a transaction
>> >
>> > Could you let me know how this patch has something to do with MVCC
>> > catalog
>> > access patch? Should we wait for MVCC catalog access patch to be
>> > committed
>> > before starting to review this patch?
>>
>> I wondered the same. The MVCC catalog patch, if applied, would make it
>> possible to make the actual relfilenode swap concurrently instead of
>> requiring to take access exlusive locks which obviously is way nicer. On
>> the other hand, that function is only a really small part of this patch,
>> so it seems quite possible to make another pass at it before relying on
>> mvcc catalog scans.
>
> As mentionned by Andres, the only thing that the MVCC catalog patch can
> improve here
> is the index swap phase (index_concurrent_swap:index.c) where the
> relfilenode of the
> old and new indexes are exchanged. Now an AccessExclusiveLock is taken on
> the 2 relations
> being swap, we could leverage that to ShareUpdateExclusiveLock with the MVCC
> catalog
> access I think.
>
> Also, with the MVCC catalog patch in, we could add some isolation tests for
> REINDEX CONCURRENTLY (there were some tests in one of the previous
> versions),
> what is currently not possible due to the exclusive lock taken at swap
> phase.
>
> Btw, those are minor things in the patch, so I think that it would be better
> to not wait
> for the MVCC catalog patch. Even if you think that it would be better to
> wait for it,
> you could even begin with the 1st patch allowing a toast relation to have
> multiple
> indexes (removal of reltoastidxid) which does not depend at all on it.

Here are the review comments of the removal_of_reltoastidxid patch.
I've not completed the review yet, but I'd like to post the current comments
before going to bed ;)

*** a/src/backend/catalog/system_views.sql
-            pg_stat_get_blocks_fetched(X.oid) -
-                    pg_stat_get_blocks_hit(X.oid) AS tidx_blks_read,
-            pg_stat_get_blocks_hit(X.oid) AS tidx_blks_hit
+            pg_stat_get_blocks_fetched(X.indrelid) -
+                    pg_stat_get_blocks_hit(X.indrelid) AS tidx_blks_read,
+            pg_stat_get_blocks_hit(X.indrelid) AS tidx_blks_hit

ISTM that X.indrelid indicates the TOAST table not the TOAST index.
Shouldn't we use X.indexrelid instead of X.indrelid?

You changed some SQLs because of removal of reltoastidxid.
Could you check that the original SQL and changed one return
the same value, again?

doc/src/sgml/diskusage.sgml
> There will be one index on the
> <acronym>TOAST</> table, if present.

I'm not sure if multiple indexes on TOAST table are viewable by a user.
If it's viewable, we need to correct the above description.

doc/src/sgml/monitoring.sgml
>     <entry><structfield>tidx_blks_read</></entry>
>     <entry><type>bigint</></entry>
>     <entry>Number of disk blocks read from this table's TOAST table index (if any)</entry>
>    </row>
>    <row>
>     <entry><structfield>tidx_blks_hit</></entry>
>     <entry><type>bigint</></entry>
>     <entry>Number of buffer hits in this table's TOAST table index (if any)</entry>

For the same reason as the above, we need to change "index" to "indexes"
in these descriptions?

*** a/src/bin/pg_dump/pg_dump.c
+                      "SELECT c.reltoastrelid, t.indexrelid "                      "FROM pg_catalog.pg_class c LEFT
JOIN"
 
-                      "pg_catalog.pg_class t ON (c.reltoastrelid = t.oid) "
-                      "WHERE c.oid = '%u'::pg_catalog.oid;",
+                      "pg_catalog.pg_index t ON (c.reltoastrelid = t.indrelid) "
+                      "WHERE c.oid = '%u'::pg_catalog.oid AND t.indisvalid "
+                      "LIMIT 1",

Is there the case where TOAST table has more than one *valid* indexes?
If yes, is it really okay to choose just one index by using LIMIT 1?
If no, i.e., TOAST table should have only one valid index, we should get rid
of LIMIT 1 and check that only one row is returned from this query.
Fortunately, ISTM this check has been already done by the subsequent
call of ExecuteSqlQueryForSingleRow(). Thought?

Regards,

-- 
Fujii Masao