Re: ALTER tbl rewrite loses CLUSTER ON index

On Thu, Apr 02, 2020 at 03:14:21PM +0900, Michael Paquier wrote:
> Now, regarding patch 0002, note that you have a problem for this part:
> - tuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexOid));
> - if (!HeapTupleIsValid(tuple)) /* probably can't happen */
> - {
> - relation_close(OldHeap, AccessExclusiveLock);
> - pgstat_progress_end_command();
> - return;
> - }
> - indexForm = (Form_pg_index) GETSTRUCT(tuple);
> - if (!indexForm->indisclustered)
> + if (!get_index_isclustered(indexOid))
> {
> - ReleaseSysCache(tuple);
> relation_close(OldHeap, AccessExclusiveLock);
> pgstat_progress_end_command();
> return;
> }
> - ReleaseSysCache(tuple);
>
> On an invalid tuple for pg_index, the new code would issue an error,
> while the old code would just return. And it seems to me that this
> can lead to problems because the parent relation is processed and
> locked at the beginning of cluster_rel(), *after* we know the index
> OID to work on.

> The refactoring is fine for the other two areas
> though, so I think that there is still value to apply
> get_index_isclustered() within mark_index_clustered() and cluster(),
> and I would suggest to keep 0002 to that.
>
> Justin, what do you think?

Sounds right. Or else get_index_isclustered() could be redefined to take a
boolean "do_elog" flag, and if syscache fails and that's false, then return
false instead of ERROR.

--
Justin