From: Noah Misch <noah@leadboat.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Peter Geoghegan <pg@bowt.ie>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@postgresql.org
Subject: Re: pgstatindex vs. !indisready
Date: Sun, 22 Oct 2023 14:02:45 -0700
Message-ID: <20231022210245.2f.nmisch@google.com> (raw)
In-Reply-To: <ZRyrF7QHMYTj6xTq@paquier.xyz>
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>
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.
Author: Noah Misch <noah@leadboat.com>
Commit: Noah Misch <noah@leadboat.com>
Diagnose !indisvalid in more SQL functions.
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).
Reviewed by FIXME.
Discussion: https://postgr.es/m/20231001195309.a3@google.comdiff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstatindex.cindex 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")));
/*
+ * 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 fcinfo)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot access temporary indexes of other sessions")));
+ /* 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")));
+ /* 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 = _hash_getbuf(rel, HASH_METAPAGE, HASH_READ, LH_META_PAGE);
diff --git a/contrib/pgstattuple/pgstattuple.c b/contrib/pgstattuple/pgstattuple.cindex 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 == 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.cindex 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))));
- /* 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))));
/* 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))));
- /* the revmap does the hard work */- do+ /* see gin_clean_pending_list() */+ if (indexRel->rd_index->indisvalid)
{
- done = brinRevmapDesummarizeRange(indexRel, heapBlk);+ /* the revmap does the hard work */+ do+ {+ done = 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))));
relation_close(indexRel, ShareUpdateExclusiveLock);
relation_close(heapRel, ShareUpdateExclusiveLock);
diff --git a/src/backend/access/gin/ginfast.c b/src/backend/access/gin/ginfast.cindex 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 = PG_GETARG_OID(0);
Relation indexRel = index_open(indexoid, RowExclusiveLock);
IndexBulkDeleteResult stats;
- GinState ginstate;
if (RecoveryInProgress())
ereport(ERROR,
@@ -1064,8 +1063,26 @@ gin_clean_pending_list(PG_FUNCTION_ARGS)
RelationGetRelationName(indexRel));
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))));
index_close(indexRel, RowExclusiveLock);
Attachments:
[text/plain] indisvalid-funcs-v1.patch (6.2K, ../20231022210245.2f.nmisch@google.com/2-indisvalid-funcs-v1.patch)
download | inline diff:
Author: Noah Misch <noah@leadboat.com>
Commit: Noah Misch <noah@leadboat.com>
Diagnose !indisvalid in more SQL functions.
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).
Reviewed by FIXME.
Discussion: https://postgr.es/m/20231001195309.a3@google.com
diff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstatindex.cindex 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")));
/*
+ * 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 fcinfo)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot access temporary indexes of other sessions")));
+ /* 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")));
+ /* 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 = _hash_getbuf(rel, HASH_METAPAGE, HASH_READ, LH_META_PAGE);
diff --git a/contrib/pgstattuple/pgstattuple.c b/contrib/pgstattuple/pgstattuple.cindex 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 == 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.cindex 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))));
- /* 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))));
/* 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))));
- /* the revmap does the hard work */- do+ /* see gin_clean_pending_list() */+ if (indexRel->rd_index->indisvalid)
{
- done = brinRevmapDesummarizeRange(indexRel, heapBlk);+ /* the revmap does the hard work */+ do+ {+ done = 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))));
relation_close(indexRel, ShareUpdateExclusiveLock);
relation_close(heapRel, ShareUpdateExclusiveLock);
diff --git a/src/backend/access/gin/ginfast.c b/src/backend/access/gin/ginfast.cindex 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 = PG_GETARG_OID(0);
Relation indexRel = index_open(indexoid, RowExclusiveLock);
IndexBulkDeleteResult stats;
- GinState ginstate;
if (RecoveryInProgress())
ereport(ERROR,
@@ -1064,8 +1063,26 @@ gin_clean_pending_list(PG_FUNCTION_ARGS)
RelationGetRelationName(indexRel));
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))));
index_close(indexRel, RowExclusiveLock);
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: noah@leadboat.com, michael@paquier.xyz, pg@bowt.ie, tgl@sss.pgh.pa.us
Subject: Re: pgstatindex vs. !indisready
In-Reply-To: <20231022210245.2f.nmisch@google.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox