Re: Support for REINDEX CONCURRENTLY

On Wed, Jun 19, 2013 at 9:50 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Jun 19, 2013 at 12:36 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Tue, Jun 18, 2013 at 10:53 AM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>> An updated patch for the toast part is attached.
>>>
>>> On Tue, Jun 18, 2013 at 3:26 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)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.
> I changed it a little bit in a different way in my latest patch by
> adding a sum on all the indexes when getting tidx_blks stats.
>
>>>> 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.
> Not sure about that. The latest code is made such as only one valid
> index is present on the toast relation at the same time.
>
>>
>>>> *** 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....
> Well, I have to disagree. I am not able to reproduce it. Which version
> did you use? Here is what I get with the latest version of REINDEX
> CONCURRENTLY patch... I checked with the following process:

Sorry. This is my mistake.

Regards,

--
Fujii Masao