Re: Fwd: BUG #18016: REINDEX TABLE failure

On Fri, Jul 28, 2023 at 11:00:56AM -0700, Nathan Bossart wrote:
> On Fri, Jul 28, 2023 at 10:50:50AM +0900, Michael Paquier wrote:
>> On Thu, Jul 27, 2023 at 04:14:41PM -0700, Nathan Bossart wrote:
>>> On Wed, Jul 26, 2023 at 06:43:18PM -0700, Gurjeet Singh wrote:
>>>> I felt the need for that paragraph, because it doesn't feel obvious to
>>>> me as to why we can't simply reindex the toast table as the first
>>>> thing in this function; the toast table reindex will trigger CCI, and
>>>> that'd be bad if done before RelationGetIndexList().
>>> 
>>> I see.  I'd suggest referencing the comment above the function, but in
>>> general I do think having a comment about this is appropriate.
>> 
>> +    * This should be done after the suppression of the use of indexes (above),
>> +    * because the recursive call to reindex_relation() below will invoke
>> +    * CommandCounterIncrement(), which may prevent enumeration of the indexes
>> +    * on the table.
>> 
>> This does not explain the reason why this would prevent the creation
>> of a consistent index list fetched from the parent table, does it?
>> Would some indexes be missing from what should be reindexed?  Or some
>> added unnecessarily?  Would that be that an incorrect list?
> 
> IIUC the issue is that something (e.g., VACUUM FULL, CLUSTER) might've just
> rebuilt the heap, so if we CCI'd before gathering the list of indexes, the
> new heap contents would become visible, and the indexes would be
> inconsistent with the heap.  This is a problem when the relation in
> question is a system catalog that needs to be consulted to gather the list
> of indexes.  To handle this, we avoid the CCI until after gathering the
> indexes so that the old heap contents appear valid and can be used as
> needed.  Once that is done, we mark the indexes as pending-rebuild and do a
> CCI, at which point the indexes become inconsistent with the heap.  This
> behavior appears to have been added by commit b9b8831.

How do we move this one forward?  Michael and I provided some feedback
about the comment, but AFAICT this patch is in good shape otherwise.
Gurjeet, would you mind putting together a new version of the patch so that
we can close on this one?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com