Re: REINDEX CONCURRENTLY 2.0

On 02/13/2017 06:31 AM, Michael Paquier wrote:
>> - What should we do with REINDEX DATABASE CONCURRENTLY and the system
>> catalog? I so not think we can reindex the system catalog concurrently
>> safely, so what should REINDEX DATABASE do with the catalog indexes? Skip
>> them, reindex them while taking locks, or just error out?
>
> System indexes cannot have their OIDs changed as they are used in
> syscache lookups. So just logging a warning looks fine to me, and the
> price to pay to avoid taking an exclusive lock even for a short amount
> of time.

Good idea, I think I will add one line of warning if it finds any system
index in the schema.

>> - What level of information should be output in VERBOSE mode?
>
> Er, something like that as well, no?
> DETAIL: CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.00 s.

REINDEX (VERBOSE) currently prints one such line per index, which does
not really work for REINDEX (VERBOSE) CONCURRENTLY since it handles all
indexes on a relation at the same time. It is not immediately obvious
how this should work. Maybe one such detail line per table?

> This is a crasher:
> create table aa (a int primary key);
> reindex (verbose) schema concurrently public ;
>
> For invalid indexes sometimes snapshots are still active (after
> issuing the previous crash for example):
> =# reindex (verbose) table concurrently aa;
> WARNING: XX002: cannot reindex concurrently invalid index
> "public.aa_pkey_cct", skipping
> LOCATION: ReindexRelationConcurrently, indexcmds.c:2119
> WARNING: 01000: snapshot 0x7fde12003038 still active

Thanks for testing the patch! The crash was caused by things being
allocated in the wrong memory context when reindexing multiple tables
and therefore freed on the first intermediate commit. I have created a
new memory context to handle this in which I only allocate the lists
which need to survive between transactions..

Hm, when writing the above I just realized why ReindexTable/ReindexIndex
did not suffer from the same bug. It is because the first transaction
there allocated in the PortalHeapMemory context which survives commit. I
really need to look at if there is a clean way to handle memory contexts
in my patch.

I also found the snapshot still active bug, it seems to have been caused
by REINDEX TABLE CONCURRENTLY leaving an open snapshot which cannot be
popped by PortalRunUtility().

Thanks again!
Andreas