Re: Fwd: BUG #18016: REINDEX TABLE failure

On Thu, 30 Nov 2023 at 03:14, Gurjeet Singh <gurjeet(at)singh(dot)im> wrote:
>
> 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.

CFBot shows that the patch does not apply anymore as in [1]:
=== Applying patches on top of PostgreSQL commit ID
06a66d87dbc7e06581af6765131ea250063fb4ac ===
=== applying patch
./v2-0001-Reindex-the-toast-table-if-any-before-the-main-ta.patch
patching file src/backend/catalog/index.c
...
Hunk #5 FAILED at 4001.
1 out of 5 hunks FAILED -- saving rejects to file
src/backend/catalog/index.c.rej

Please have a look and post an updated version.

[1] - http://cfbot.cputube.org/patch_46_4443.log

Regards,
Vignesh