Re: [HACKERS] REINDEX CONCURRENTLY 2.0 - Mailing list pgsql-hackers

Hello

I review code and documentation and i have few notes. Did you register this patch in CF app?

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.

> 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.
 

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.

> 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

>       <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
indexeswe still need reindex with lock table or recreate this index concurrently.
 

> +       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?

regards, Sergei