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