Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 26 Sep 2024 16:01:42 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Tender Wang <tndrwang@gmail.com>
Cc: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18630: Incorrect memory access inside
 ReindexIsProcessingIndex() on VACUUM
Message-ID: <ZvUG1qsjXqppi2BO@paquier.xyz>
References: <18630-9aed99c38830657d@postgresql.org>
 <CAHewXNnNZXbq_sW=OP-O-1nnMEnpXJX6djbzF52LtDA6tOutiQ@mail.gmail.com>
 <CAHewXNkKeE3i7aT_nysWphRHhwMnyG1r55Di=RCNtLa5dRSd4Q@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="sOdwi4Zf5jAWQHGt"
Content-Disposition: inline
In-Reply-To: 
 <CAHewXNkKeE3i7aT_nysWphRHhwMnyG1r55Di=RCNtLa5dRSd4Q@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZvUG1qsjXqppi2BO%40paquier.xyz>
Precedence: bulk


--sOdwi4Zf5jAWQHGt
Content-Type: multipart/mixed; boundary="Vg1mJ5IvzsT0S5Um"
Content-Disposition: inline


--Vg1mJ5IvzsT0S5Um
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Wed, Sep 25, 2024 at 06:19:20PM +0800, Tender Wang wrote:
> Adding  if (flags & REINDEX_REL_SUPPRESS_INDEX_USE) check before calling
> RemoveReindexPending() may look better.
> Any thoughts?

The short answer to that is yes, see below.

Alexander's trick presented upthread is funky.  I was first confused
of what was the point until I noticed that this is just a way to make
REINDEX CONCURRENTLY fail and produce an invalid toast index.  The
first pg_sleep causes REINDEX to fail because we are waiting for the
first transaction to complete.  So this provides a controlled way to
get an index that should never be part of the list reported to
SetReindexPending() to begin with.  The VACUUM (PROCESS_MAIN FALSE,
FULL) is then disturbed.

Anyway, PROCESS_MAIN is not related to the failure, as it is just a
flavor grammar introduced for the sake of making toast-only rebuilds
easier for administrators.  The same failure reproduces if switching
the test case of upthread to do the REINDEX on the toast table, down
to 12 which is as far as I have tested because that's what we support
around here.

Regarding the fix, we should not disturb the list of indexes in a
relation returned by relcache.c, and invalid indexes are part of it.
So I'd agree with your point to just remove the index from the pending
list because we have to skip invalid toast indexes in the
reindex_relation() path as reindex_index() has to generate a hard
failure because we can never ever have two valid toast indexes, and
that's a guarantee we need to be very careful about.

Now, about REINDEX_REL_SUPPRESS_INDEX_USE.  Well, that's only used for
the VACUUM FULL/CLUSTER path which is where the indexes are marked as
being processed, so we have to reomve the invalid toast indexes from
the list only if this flag is used.  Hence the answer is yes.  I'd
rather keep the informative warning.  That's useful for monitoring
purposes.

Something worth noting while I have looked at this code..  Non-toast
invalid indexes are rebuilt but still marked as !indisvalid, actually.
That's a waste and we could just skip them for a CLUSTER/VACUUM FULL.
This cannot be true for a REINDEX TABLE, invalid indexes are switched
to valid in this case.
--
Michael

--Vg1mJ5IvzsT0S5Um
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="0001-Remove-invalid-toast-indexes-from-reindex-processing.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 4dc38bc64d96764c4673a73f646c26252bb7fe33 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Thu, 26 Sep 2024 15:59:59 +0900
Subject: [PATCH] Remove invalid toast indexes from reindex processing list =
in
 VACUUM FULL

Blah, blah...

Backpatch-through: 12
---
 src/backend/catalog/index.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index d0dcbf318d..3a4f2e260d 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -3989,6 +3989,14 @@ reindex_relation(const ReindexStmt *stmt, Oid relid,=
 int flags,
 					 errmsg("cannot reindex invalid index \"%s.%s\" on TOAST table, skipp=
ing",
 							get_namespace_name(indexNamespaceId),
 							get_rel_name(indexOid))));
+
+			/*
+			 * Remove this invalid toast index from the reindex pending list,
+			 * as it is skipped here due to the hard failure that would happen
+			 * in reindex_index() should we try to process it.
+			 */
+			if (flags & REINDEX_REL_SUPPRESS_INDEX_USE)
+				RemoveReindexPending(indexOid);
 			continue;
 		}
=20
--=20
2.45.2


--Vg1mJ5IvzsT0S5Um--

--sOdwi4Zf5jAWQHGt
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmb1BtYACgkQnvQgOdby
QH0Eig//R5FerHruATeF8NnmfJRNnGREgHyVXbU2go0baP9r9/j+WG8uY25rQUE6
71Y9R4vYl9NYEhtHs+dYERJi7eF+FGDNHnGSKu9wR6psBnVhQ0cXLqKanF4z7FX/
XUfohxUdSXhP4qbdlrBOXx001GKFgyZWDGGd6yzdiiV6SyznuIjjVzwPVsNsfax8
/X44tMrdWtM+9Pl2n4rjAyftl8qnjyWo7WA2/TIVnrlLL/Er4AFsBLDA3a4ZHWlq
olecUavGD3ECW9wgaLnMsGnIjWcuU7Kpwt+2hAWft8kEaegilTNkW1qH88yNYwPX
tl8tJJqoBqEb7YSwCzGDNwCSPVG6BWjEpEQjErhZmBQtjIvDWB6/jcQMcc5Ygm1j
GYkTAQ0UDCYNFtwMjhtKbsF/ip4jqiLqDx9wCV5ns/9OZHyZWj8hUe4fgIBdOLEj
la1Qe0Lfd1cr2a0rGlJIfen6aQ4hqtKO++aO9GLiZMh2MV8ws0YuC7mzbMf4kV+A
mUhcfY60r1kmT81QD5cF2BF3RKYd+0W4g1OIwmhJ65CzHtbldKzYh/Gef2Etxfly
JXdjUJYIK7nVUyExsowGp/WQ7fhKmX0CPraVE7SVIHDUOoI+UVwEk4RnUI1SgE1P
RioYujfhEvvSC8t50LeOhaz1aZm/cfA9N+sfKd+ze93VR0N9TbQ=
=2IBT
-----END PGP SIGNATURE-----

--sOdwi4Zf5jAWQHGt--