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