Date: Sun, 22 Oct 2023 14:02:45 -0700
From: Noah Misch <noah@leadboat.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Peter Geoghegan <pg@bowt.ie>, Tom Lane <tgl@sss.pgh.pa.us>,
	pgsql-hackers@postgresql.org
Subject: Re: pgstatindex vs. !indisready
Message-ID: <20231022210245.2f.nmisch@google.com>
References: <20231001195309.a3@google.com>
 <1867127.1696192645@sss.pgh.pa.us>
 <20231001205830.2f.nmisch@google.com>
 <CAH2-WzmFaz4RcJnyNrouHmf1x0FFTDFih_krm3aohVc3NmA9qQ@mail.gmail.com>
 <ZRoNwZD8gjGToV2R@paquier.xyz>
 <20231002013126.ad.nmisch@google.com>
 <ZRyrF7QHMYTj6xTq@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="9KwD0i3qLiwN7pwy"
Content-Disposition: inline
In-Reply-To: <ZRyrF7QHMYTj6xTq@paquier.xyz>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20231022210245.2f.nmisch%40google.com>
Precedence: bulk


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

On Wed, Oct 04, 2023 at 09:00:23AM +0900, Michael Paquier wrote:
> On Sun, Oct 01, 2023 at 06:31:26PM -0700, Noah Misch wrote:
> > The !indisvalid index may be missing tuples, yes.  In what way does that make
> > one of those four operations incorrect?
> 
> Reminding myself of what these four do, it looks that you're right and
> that the state of indisvalid is not going to change what they report.
> 
> Still, I'd like to agree with Tom's point to be more conservative and
> check also for indisvalid which is what the planner does.  These
> functions will be used in SELECTs, and one thing that worries me is
> that checks based on indisready may get copy-pasted somewhere else,
> leading to incorrect results where they get copied.  (indisready &&
> !indisvalid) is a "short"-term combination in a concurrent build
> process, as well (depends on how long one waits for the old snapshots
> before switching indisvalid, but that's way shorter than the period of
> time where the built indexes remain valid).

Neither choice would harm the user experience in an important way, so I've
followed your preference in the attached patch.

--9KwD0i3qLiwN7pwy
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="indisvalid-funcs-v1.patch"
Content-Transfer-Encoding: quoted-printable

Author:     Noah Misch <noah@leadboat.com>
Commit:     Noah Misch <noah@leadboat.com>

    Diagnose !indisvalid in more SQL functions.
   =20
    pgstatindex failed with ERRCODE_DATA_CORRUPTED, of the "can't-happen"
    class XX.  The other functions succeeded on an empty index; they might
    have malfunctioned if the failed index build left torn I/O or other
    complex state.  Report an ERROR in statistics functions pgstatindex,
    pgstatginindex, pgstathashindex, and pgstattuple.  Report DEBUG1 and
    skip all index I/O in maintenance functions brin_desummarize_range,
    brin_summarize_new_values, brin_summarize_range, and
    gin_clean_pending_list.  Back-patch to v11 (all supported versions).
   =20
    Reviewed by FIXME.
   =20
    Discussion: https://postgr.es/m/20231001195309.a3@google.com

diff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstat=
index.c
index d69ac1c..8e5a4d6 100644
--- a/contrib/pgstattuple/pgstatindex.c
+++ b/contrib/pgstattuple/pgstatindex.c
@@ -238,6 +238,18 @@ pgstatindex_impl(Relation rel, FunctionCallInfo fcinfo)
 				 errmsg("cannot access temporary tables of other sessions")));
=20
 	/*
+	 * A !indisready index could lead to ERRCODE_DATA_CORRUPTED later, so exit
+	 * early.  We're capable of assessing an indisready&&!indisvalid index,
+	 * but the results could be confusing.  For example, the index's size
+	 * could be too low for a valid index of the table.
+	 */
+	if (!rel->rd_index->indisvalid)
+		ereport(ERROR,
+				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+				 errmsg("index \"%s\" is not valid",
+						RelationGetRelationName(rel))));
+
+	/*
 	 * Read metapage
 	 */
 	{
@@ -523,6 +535,13 @@ pgstatginindex_internal(Oid relid, FunctionCallInfo fc=
info)
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 errmsg("cannot access temporary indexes of other sessions")));
=20
+	/* see pgstatindex_impl */
+	if (!rel->rd_index->indisvalid)
+		ereport(ERROR,
+				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+				 errmsg("index \"%s\" is not valid",
+						RelationGetRelationName(rel))));
+
 	/*
 	 * Read metapage
 	 */
@@ -600,6 +619,13 @@ pgstathashindex(PG_FUNCTION_ARGS)
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 errmsg("cannot access temporary indexes of other sessions")));
=20
+	/* see pgstatindex_impl */
+	if (!rel->rd_index->indisvalid)
+		ereport(ERROR,
+				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+				 errmsg("index \"%s\" is not valid",
+						RelationGetRelationName(rel))));
+
 	/* Get the information we need from the metapage. */
 	memset(&stats, 0, sizeof(stats));
 	metabuf =3D _hash_getbuf(rel, HASH_METAPAGE, HASH_READ, LH_META_PAGE);
diff --git a/contrib/pgstattuple/pgstattuple.c b/contrib/pgstattuple/pgstat=
tuple.c
index 93b7834..3bd8b96 100644
--- a/contrib/pgstattuple/pgstattuple.c
+++ b/contrib/pgstattuple/pgstattuple.c
@@ -259,6 +259,13 @@ pgstat_relation(Relation rel, FunctionCallInfo fcinfo)
 	}
 	else if (rel->rd_rel->relkind =3D=3D RELKIND_INDEX)
 	{
+		/* see pgstatindex_impl */
+		if (!rel->rd_index->indisvalid)
+			ereport(ERROR,
+					(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+					 errmsg("index \"%s\" is not valid",
+							RelationGetRelationName(rel))));
+
 		switch (rel->rd_rel->relam)
 		{
 			case BTREE_AM_OID:
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index af392bc..25338a9 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -1100,8 +1100,14 @@ brin_summarize_range(PG_FUNCTION_ARGS)
 				 errmsg("could not open parent table of index \"%s\"",
 						RelationGetRelationName(indexRel))));
=20
-	/* OK, do it */
-	brinsummarize(indexRel, heapRel, heapBlk, true, &numSummarized, NULL);
+	/* see gin_clean_pending_list() */
+	if (indexRel->rd_index->indisvalid)
+		brinsummarize(indexRel, heapRel, heapBlk, true, &numSummarized, NULL);
+	else
+		ereport(DEBUG1,
+				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+				 errmsg("index \"%s\" is not valid",
+						RelationGetRelationName(indexRel))));
=20
 	/* Roll back any GUC changes executed by index functions */
 	AtEOXact_GUC(false, save_nestlevel);
@@ -1183,12 +1189,21 @@ brin_desummarize_range(PG_FUNCTION_ARGS)
 				 errmsg("could not open parent table of index \"%s\"",
 						RelationGetRelationName(indexRel))));
=20
-	/* the revmap does the hard work */
-	do
+	/* see gin_clean_pending_list() */
+	if (indexRel->rd_index->indisvalid)
 	{
-		done =3D brinRevmapDesummarizeRange(indexRel, heapBlk);
+		/* the revmap does the hard work */
+		do
+		{
+			done =3D brinRevmapDesummarizeRange(indexRel, heapBlk);
+		}
+		while (!done);
 	}
-	while (!done);
+	else
+		ereport(DEBUG1,
+				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+				 errmsg("index \"%s\" is not valid",
+						RelationGetRelationName(indexRel))));
=20
 	relation_close(indexRel, ShareUpdateExclusiveLock);
 	relation_close(heapRel, ShareUpdateExclusiveLock);
diff --git a/src/backend/access/gin/ginfast.c b/src/backend/access/gin/ginf=
ast.c
index eb6c554..657e941 100644
--- a/src/backend/access/gin/ginfast.c
+++ b/src/backend/access/gin/ginfast.c
@@ -1032,7 +1032,6 @@ gin_clean_pending_list(PG_FUNCTION_ARGS)
 	Oid			indexoid =3D PG_GETARG_OID(0);
 	Relation	indexRel =3D index_open(indexoid, RowExclusiveLock);
 	IndexBulkDeleteResult stats;
-	GinState	ginstate;
=20
 	if (RecoveryInProgress())
 		ereport(ERROR,
@@ -1064,8 +1063,26 @@ gin_clean_pending_list(PG_FUNCTION_ARGS)
 					   RelationGetRelationName(indexRel));
=20
 	memset(&stats, 0, sizeof(stats));
-	initGinState(&ginstate, indexRel);
-	ginInsertCleanup(&ginstate, true, true, true, &stats);
+
+	/*
+	 * Can't assume anything about the content of an !indisready index.  Make
+	 * those a no-op, not an error, so users can just run this function on all
+	 * indexes of the access method.  Since an indisready&&!indisvalid index
+	 * is merely awaiting missed aminsert calls, we're capable of processing
+	 * it.  Decline to do so, out of an abundance of caution.
+	 */
+	if (indexRel->rd_index->indisvalid)
+	{
+		GinState	ginstate;
+
+		initGinState(&ginstate, indexRel);
+		ginInsertCleanup(&ginstate, true, true, true, &stats);
+	}
+	else
+		ereport(DEBUG1,
+				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+				 errmsg("index \"%s\" is not valid",
+						RelationGetRelationName(indexRel))));
=20
 	index_close(indexRel, RowExclusiveLock);
=20

--9KwD0i3qLiwN7pwy--