Re: [HACKERS] REINDEX CONCURRENTLY 2.0

Updated patch for some merge conflicts and addressing most of your
comments.  (I did not do anything about the syntax.)

On 09/12/2018 19:55, Sergei Kornilov wrote:
> I found one error in phase 4. Simple reproducer:
> 
>> create table test (i int);
>> create index this_is_very_large_exactly_maxnamelen_index_name_wink_wink_wink on test (i);
>> create index this_is_very_large_exactly_maxnamelen_index_name_wink_winkccold on test (i);
>> reindex table CONCURRENTLY test;
> 
> This fails with error
> 
>> ERROR:  duplicate key value violates unique constraint "pg_class_relname_nsp_index"
>> DETAIL:  Key (relname, relnamespace)=(this_is_very_large_exactly_maxnamelen_index_name_wink_win_ccold, 2200) already exists.
> 
> CommandCounterIncrement() in (or after) index_concurrently_swap will fix this issue.

fixed

>> ReindexPartitionedIndex(Relation parentIdx)
>> 	ereport(ERROR,
>> 			(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>> 			 errmsg("REINDEX is not yet implemented for partitioned indexes")));
> 
> I think we need add errhint("you can REINDEX each partition separately") or something similar. 
> Also can we omit this warning for reindex database? All partition must be in same database and warning in such case is useless: we have warning, but doing reindex for each partition => we reindex partitioned table correctly.

fixed by skipping in ReindexRelationConcurrently(), same as other
unsupported relkinds

> Another behavior issue i found with reindex (verbose) schema/database: INFO ereport is printed twice for each table.
> 
>> INFO:  relation "measurement_y2006m02" was reindexed
>> DETAIL:  CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.07 s.
>> INFO:  table "public.measurement_y2006m02" was reindexed
> 
> One from ReindexMultipleTables and another (with pg_rusage_show) from ReindexRelationConcurrently.

fixed with some restructuring

>> ReindexRelationConcurrently
>> if (!indexRelation->rd_index->indisvalid)
> 
> it is better use IndexIsValid macro here? And same question about added indexform->indisvalid in src/backend/commands/tablecmds.c

IndexIsValid() has been removed in the meantime.

>>       <para>
>>       An index build with the <literal>CONCURRENTLY</literal> option failed, leaving
>>       an <quote>invalid</quote> index. Such indexes are useless but it can be
>> -      convenient to use <command>REINDEX</command> to rebuild them. Note that
>> -      <command>REINDEX</command> will not perform a concurrent build. To build the
>> -      index without interfering with production you should drop the index and
>> -      reissue the <command>CREATE INDEX CONCURRENTLY</command> command.
>> +      convenient to use <command>REINDEX</command> to rebuild them.
>>      </para>
> 
> This documentation change seems wrong for me: reindex concurrently does not rebuild invalid indexes. To fix invalid indexes we still need reindex with lock table or recreate this index concurrently.

still being discussed elsewhere in this thread

>> +       A first pass to build the index is done for each new index entry.
>> +       Once the index is built, its flag <literal>pg_class.isready</literal> is
>> +       switched to <quote>true</quote>
>> +       At this point <literal>pg_class.indisvalid</literal> is switched to
>> +       <quote>true</quote> for the new index and to <quote>false</quote> for the old, and
>> +       Old indexes have <literal>pg_class.isready</literal> switched to <quote>false</quote>
> 
> Should be pg_index.indisvalid and pg_index.indisready, right?

fixed

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services