Fwd: BUG #18016: REINDEX TABLE failure

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

---------- Forwarded message ---------
From: Gurjeet Singh <gurjeet@singh.im>
Date: Wed, Jul 26, 2023 at 2:53 PM


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:
> > The code block movement involved slightly more thought and care than I
> > had previously imagined. As explained in comments in the patch, the
> > enumeration and suppression of indexes on the main table must happen
> > before any CommandCounterIncrement() call, hence the
> > reindex-the-toast-table-if-any code had to be placed after that
> > enumeration.
>
> Do we need to add another CCI after reindexing the TOAST table?  It looks
> like we presently do so between reindexing each relation, including the
> TOAST table.

Yes, we do need to do a CCI after reindex the relation's toast table.
But note that it is done by the recursive call to reindex_relation(),
right after it calls reindex_index().

> +        * 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.

  * REINDEX_REL_SUPPRESS_INDEX_USE: if true, the relation was just completely
...
  *  ... The caller is required to call us *without*
  * having made the rebuilt table visible by doing CommandCounterIncrement;
  * we'll do CCI after having collected the index list.  (This way we can still
  * use catalog indexes while collecting the list.)

I hope that makes sense.

Best regards,
Gurjeet
http://Gurje.et