Date: Fri, 28 Jul 2023 11:00:56 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Gurjeet Singh <gurjeet@singh.im>, Tom Lane <tgl@sss.pgh.pa.us>,
	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: <20230728180056.GB3661419@nathanxps13>
References: 
 <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>
 <20230727231441.GB3612597@nathanxps13>
 <ZMMe+iIUYrZsMT8c@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZMMe+iIUYrZsMT8c@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/20230728180056.GB3661419%40nathanxps13>
Precedence: bulk

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.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com