Fwd: BUG #18016: REINDEX TABLE failure

(Re-sending with -hackers list removed, to avoid message getting held
for moderation)

---------- Forwarded message ---------
From: Gurjeet Singh 
Date: Wed, Jul 26, 2023 at 4:01 PM

On Wed, Jul 26, 2023 at 2:53 PM Gurjeet Singh  wrote:
>
> On Wed, Jul 26, 2023 at 10:50 AM Nathan Bossart
>  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().

Best regards,
Gurjeet
http://Gurje.et