Date: Thu, 27 Jul 2023 16:14:41 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Gurjeet Singh <gurjeet@singh.im>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Michael Paquier <michael@paquier.xyz>,
	Richard =?iso-8859-1?Q?Vesel=FD?= <richard.vesely@softea.sk>,
	pgsql-bugs@lists.postgresql.org
Subject: Re: Fwd: BUG #18016: REINDEX TABLE failure
Message-ID: <20230727231441.GB3612597@nathanxps13>
References: <ZKirERBzkM48E0op@paquier.xyz>
 <1457241.1688775601@sss.pgh.pa.us>
 <CABwTF4XCTA=guV=4v3xkQnKhtqKsW2njeKeZCNtDhSFoYRWfDQ@mail.gmail.com>
 <ZKpn+44jciv6Ym3/@paquier.xyz>
 <1867443.1688912310@sss.pgh.pa.us>
 <CABwTF4WX=m5pQvKXvLFJoEH=hSd6O=iZSqxVqHKjFm+iL-AO=w@mail.gmail.com>
 <20230726175000.GA3259285@nathanxps13>
 <CABwTF4U+t-8sD2y2xhgHyj=ovPMbrsNJO5F49nvZZtpwFkwWDA@mail.gmail.com>
 <CABwTF4XyGzeEetn5AhsVTfY7zzBBcfjuMCfTLinOBWdbHbCWPw@mail.gmail.com>
 <CABwTF4XHEfL9rc25z+w9o2_Nmo+2_TAVm+LwB1tdtzu1+w12gA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CABwTF4XHEfL9rc25z+w9o2_Nmo+2_TAVm+LwB1tdtzu1+w12gA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20230727231441.GB3612597%40nathanxps13>
Precedence: bulk

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