Re: Support for REINDEX CONCURRENTLY

On Tue, Jun 18, 2013 at 10:53 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> An updated patch for the toast part is attached.
>
> On Tue, Jun 18, 2013 at 3:26 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> 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?
> Indeed good catch! We need in this case the statistics on the index
> and here I used the table OID. Btw, I also noticed that as multiple
> indexes may be involved for a given toast relation, it makes sense to
> actually calculate tidx_blks_read and tidx_blks_hit as the sum of all
> stats of the indexes.

Yep. You seem to need to change X.indexrelid to X.indrelid in GROUP clause.
Otherwise, you may get two rows of the same table from pg_statio_all_tables.

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

+   table (see <xref linkend="storage-toast">).  There will be one valid index
+   on the <acronym>TOAST</> table, if present. There also might be indexes

When I used gdb and tracked the code path of concurrent reindex patch,
I found it's possible that more than one *valid* toast indexes appear. Those
multiple valid toast indexes are viewable, for example, from pg_indexes.
I'm not sure whether this is the bug of concurrent reindex patch. But
if it's not,
you seem to need to change the above description again.

>> *** 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?
> I just rechecked the patch and is answer is no. The concurrent index
> is set as valid inside the same transaction as swap. So only the
> backend performing the swap will be able to see two valid toast
> indexes at the same time.

According to my quick gdb testing, this seems not to be true....

Regards,

-- 
Fujii Masao