On Thu, Mar 05, 2020 at 12:53:54PM +0900, Michael Paquier wrote:
> On Wed, Mar 04, 2020 at 09:21:45AM +0100, Julien Rouhaud wrote:
> > Thanks for the patch! I started to look at it during the weekend, but
> > I got interrupted and unfortunately didn't had time to look at it
> > since.
>
> No problem, thanks for looking at it. I have looked at it again this
> morning, and applied it.
>
> > The fix looks good to me. I also tried multiple failure scenario and
> > it's unsurprisingly working just fine. Should we add some regression
> > tests for that? I guess most of it could be borrowed from the patch
> > to fix the toast index issue I sent last week.
>
> I have doubts when it comes to use a strategy based on
> pg_cancel_backend() and a match of application_name (see for example
> 5ad72ce but I cannot find the associated thread). I think that we
> could design something more robust here and usable by all tests, with
> two things coming into my mind:
> - A new meta-command for isolation tests to be able to cancel a
> session with PQcancel().
> - Fault injection in the backend.
> For the case of this thread, the cancellation command would be a better
> match.
I agree that the approach wasn't quite robust. I'll try to look at adding a
new command for isolationtester, but that's probably not something we want to
put in pg13?
Here's a v3 that takes address the various comments you previously noted, and
for which I also removed the regression tests.
Note that while looking at it, I noticed another bug in RIC:
# create table t1(id integer, val text); create index on t1(val);
CREATE TABLE
CREATE INDEX
# reindex table concurrently t1;
^CCancel request sent
ERROR: 57014: canceling statement due to user request
LOCATION: ProcessInterrupts, postgres.c:3171
# select indexrelid::regclass, indrelid::regclass, indexrelid, indrelid from
pg_index where not indisvalid;
indexrelid | indrelid | indexrelid |
indrelid
-------------------------------------+-------------------------+------------+----------
t1_val_idx_ccold | t1 | 16401 |
16395
pg_toast.pg_toast_16395_index_ccold | pg_toast.pg_toast_16395 | 16400 |
16398
(2 rows)
# reindex table concurrently t1;
WARNING: 0A000: cannot reindex invalid index "public.t1_val_idx_ccold"
concurrently, skipping
LOCATION: ReindexRelationConcurrently, indexcmds.c:2821
WARNING: XX002: cannot reindex invalid index
"pg_toast.pg_toast_16395_index_ccold" concurrently, skipping
LOCATION: ReindexRelationConcurrently, indexcmds.c:2867
REINDEX
# reindex index concurrently t1_val_idx_ccold;
REINDEX
That case is also fixed in this patch.
>From 77ec865a9a655b2b973846f9a8fa93c966ca55f5 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouh...@free.fr>
Date: Fri, 21 Feb 2020 20:15:04 +0100
Subject: [PATCH] Don't reindex invalid indexes on TOAST tables.
Such indexes can only be duplicated leftovers of failed REINDEX CONCURRENTLY
commands. As we only allow to drop invalid indexes on TOAST tables, reindexing
those would lead to useless duplicated indexes that can't be dropped anymore.
Reported-by: Sergei Kornilov, Justin Pryzby
Author: Julien Rouhaud
Reviewed-by: Michael Paquier
Discussion:
https://postgr.es/m/36712441546604286%40sas1-890ba5c2334a.qloud-c.yandex.net
Discussion: https://postgr.es/m/20200216190835.ga21...@telsasoft.com
Backpatch-through: 12
---
src/backend/catalog/index.c | 30 ++++++++++++++++++++
src/backend/commands/indexcmds.c | 47 +++++++++++++++++++++++++-------
2 files changed, 67 insertions(+), 10 deletions(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 7223679033..d3d28df97a 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -46,6 +46,7 @@
#include "catalog/pg_depend.h"
#include "catalog/pg_description.h"
#include "catalog/pg_inherits.h"
+#include "catalog/pg_namespace_d.h"
#include "catalog/pg_opclass.h"
#include "catalog/pg_operator.h"
#include "catalog/pg_tablespace.h"
@@ -3724,6 +3725,35 @@ reindex_relation(Oid relid, int flags, int options)
{
Oid indexOid = lfirst_oid(indexId);
+ /*
+ * We skip any invalid index on a TOAST table. Those
can only be
+ * a duplicate leftover of a failed REINDEX
CONCURRENTLY, and if we
+ * rebuild it it won't be possible to drop it anymore.
+ */
+ if (rel->rd_rel->relnamespace == PG_TOAST_NAMESPACE)
+ {
+ HeapTuple tup;
+ bool skipit;
+
+ tup = SearchSysCache1(INDEXRELID,
ObjectIdGetDatum(indexOid));
+ if (!HeapTupleIsValid(tup))
+ elog(ERROR, "cache lookup failed for
index %u", indexOid);
+
+ skipit = ((Form_pg_index)
GETSTRUCT(tup))->indisvalid == false;
+
+ ReleaseSysCache(tup);
+
+ if (skipit)
+ {
+ ereport(WARNING,
+
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot reindex
invalid TOAST index \"%s.%s\", skipping",
+
get_namespace_name(get_rel_namespace(indexOid)),
+
get_rel_name(indexOid))));
+ continue;
+ }
+ }
+
reindex_index(indexOid, !(flags &
REINDEX_REL_CHECK_CONSTRAINTS),
persistence, options);
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index ec20ba38d1..7985d98aa8 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -28,6 +28,7 @@
#include "catalog/pg_am.h"
#include "catalog/pg_constraint.h"
#include "catalog/pg_inherits.h"
+#include "catalog/pg_namespace_d.h"
#include "catalog/pg_opclass.h"
#include "catalog/pg_opfamily.h"
#include "catalog/pg_tablespace.h"
@@ -2337,6 +2338,7 @@ ReindexIndex(RangeVar *indexRelation, int options, bool
concurrent)
Oid indOid;
Relation irel;
char persistence;
+ bool invalidtoastindex;
/*
* Find and lock index, and check permissions on table; use callback to
@@ -2362,6 +2364,9 @@ ReindexIndex(RangeVar *indexRelation, int options, bool
concurrent)
*/
irel = index_open(indOid, NoLock);
+ invalidtoastindex = (irel->rd_rel->relkind == RELKIND_INDEX &&
+ !irel->rd_index->indisvalid);
+
if (irel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
{
ReindexPartitionedIndex(irel);
@@ -2371,6 +2376,16 @@ ReindexIndex(RangeVar *indexRelation, int options, bool
concurrent)
persistence = irel->rd_rel->relpersistence;
index_close(irel, NoLock);
+ if (invalidtoastindex)
+ {
+ ereport(WARNING,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot reindex invalid TOAST index
\"%s.%s\", skipping",
+
get_namespace_name(get_rel_namespace(indOid)),
+ get_rel_name(indOid))));
+ return;
+ }
+
if (concurrent && persistence != RELPERSISTENCE_TEMP)
ReindexRelationConcurrently(indOid, options);
else
@@ -2890,25 +2905,37 @@ ReindexRelationConcurrently(Oid relationOid, int
options)
case RELKIND_INDEX:
{
Oid heapId =
IndexGetRelation(relationOid, false);
+ Relation indexRelation =
index_open(relationOid,
+
ShareUpdateExclusiveLock);
if (IsCatalogRelationOid(heapId))
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot reindex
system catalogs concurrently")));
+ else if (!indexRelation->rd_index->indisvalid)
+ ereport(WARNING,
+
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot reindex
invalid index \"%s.%s\" concurrently, skipping",
+
get_namespace_name(get_rel_namespace(relationOid)),
+
get_rel_name(relationOid))));
+ else
+ {
+ /* Save the list of relation OIDs in
private context */
+ oldcontext =
MemoryContextSwitchTo(private_context);
- /* Save the list of relation OIDs in private
context */
- oldcontext =
MemoryContextSwitchTo(private_context);
+ /* Track the heap relation of this
index for session locks */
+ heapRelationIds =
list_make1_oid(heapId);
- /* Track the heap relation of this index for
session locks */
- heapRelationIds = list_make1_oid(heapId);
+ /*
+ * Save the list of relation OIDs in
private context. Note
+ * that invalid indexes are allowed
here.
+ */
+ indexIds = lappend_oid(indexIds,
relationOid);
- /*
- * Save the list of relation OIDs in private
context. Note
- * that invalid indexes are allowed here.
- */
- indexIds = lappend_oid(indexIds, relationOid);
+ MemoryContextSwitchTo(oldcontext);
+ }
- MemoryContextSwitchTo(oldcontext);
+ index_close(indexRelation, NoLock);
break;
}
case RELKIND_PARTITIONED_TABLE:
--
2.20.1
