Re: Fwd: BUG #18016: REINDEX TABLE failure

On Fri, Sep 1, 2023 at 9:55 AM Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>
> 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?

Please see attached v2 of the patch; no code changes since v1, just
comments are changed to describe the reason for behaviour and the
placement of code.

I have tried to make the comment describe in more detail the condition
it's trying to avoid. I've also referenced the function comments, as
you suggested, so that the reader can get more context about why the
code is placed _after_ certain other code.

Hopefully the comments are sufficiently descriptive. If you or another
committer feels the need to change the comments any further, please
feel free to edit them as necessary.

Best regards,
Gurjeet
http://Gurje.et