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