Re: Fwd: BUG #18016: REINDEX TABLE failure

On Wed, Jul 26, 2023 at 06:43:18PM -0700, Gurjeet Singh wrote:
> On Wed, Jul 26, 2023 at 2:53 PM Gurjeet Singh <gurjeet@singh.im> wrote:
>> On Wed, Jul 26, 2023 at 10:50 AM Nathan Bossart
>> <nathandbossart@gmail.com> wrote:
>> > On Mon, Jul 10, 2023 at 09:35:05AM -0700, Gurjeet Singh wrote:
>> > +        * 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.
>> >
>> > I'm not following this.  We've already obtained the list of index OIDs
>> > before this point.  Does this create problems when we try to open and lock
>> > the relations?  And if so, how?
>>
>> This comment is calling out the fact that there's a recursive call to
>> reindex_relation() inside the 'if' code block, and that that
>> reindex_relation() calls CCI. Hence this 'if' code block should _not_
>> be placed before the the calls to RelationGetIndexList() and
>> SetReindexPending(). Because if we do, then the CCI done by
>> reindex_relation() will impact what RelationGetIndexList() sees.
>>
>> This is to match the expectation set for the
>> REINDEX_REL_SUPPRESS_INDEX_USE flag.
> 
> Given that the issue is already explained by the flag's comments above
> the function, this comment paragraph in the patch may be considered
> extraneous. Feel free to remove it, if you think so.
> 
> 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.

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