Re: CLUSTER not lose indexes

Alvaro Herrera <alvherre(at)atentus(dot)com> writes:
> There are (of course) things I don't understand. For example, whether
> (or when) I should use CommandCounterIncrement() after each
> index_create, or if I should call setRelhasindex() only once (and not
> once per index); or whether I need to acquire some lock on the indexes.

I think you probably want a CommandCounterIncrement at the bottom of the
loop (after setRelhasindex). If it works as-is it's just by chance,
ie due to internal CCI calls in index_create.

Locking newly-created indexes is not really necessary, since no one else
can see them until you commit anyhow.

+ tuple = SearchSysCache(RELOID, ObjectIdGetDatum(attrs->indexOID),
+ 0, 0, 0);
+ if (!HeapTupleIsValid(tuple))
+ break;

Breaking out of the loop hardly seems an appropriate response to this
failure condition. Not finding the index' pg_class entry is definitely
an error.

I'd also suggest more-liberal commenting, as well as more attention to
updating the existing comments to match new reality.

In general, I'm not thrilled about expending more code on the existing
fundamentally-broken implementation of CLUSTER. We need to look at
making use of the ability to write a new version of a table (or index)
under a new relfilenode value, without changing the table's OID.
However, some parts of your patch will probably still be needed when
someone gets around to making that happen, so I won't object for now.

regards, tom lane