From: Julien Rouhaud <rjuju123@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Cc: Sergei Kornilov <sk@zsrv.org>
Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Cc: Michael Paquier <michael.paquier@gmail.com>
Cc: Andreas Karlsson <andreas@proxel.se>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: reindex concurrently and two toast indexes
Date: Fri, 6 Mar 2020 13:36:48 +0100
Message-ID: <20200306123648.GA49431@nol> (raw)
In-Reply-To: <20200306013844.GE52814@paquier.xyz>
References: <20200222150657.GA54846@nol>
<20200227073211.GA403330@paquier.xyz>
<20200227080735.l32fqcauy73lon7o@nol>
<20200303080642.GA1890@paquier.xyz>
<20200303092551.GB1890@paquier.xyz>
<20200304051510.GE2593@paquier.xyz>
<CAOBaU_Z=qkE42VQ25mps+zyAz+WUn3wZoev4uhGW+gAXBBS2YA@mail.gmail.com>
<20200305035354.GQ2593@paquier.xyz>
<20200305165707.GA35281@nol>
<20200306013844.GE52814@paquier.xyz>
On Fri, Mar 06, 2020 at 10:38:44AM +0900, Michael Paquier wrote:
> On Thu, Mar 05, 2020 at 05:57:07PM +0100, Julien Rouhaud wrote:
> > 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?
>
> Yes, that's too late.
>
> > Note that while looking at it, I noticed another bug in RIC:
> >
> > [...]
> >
> > # 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.
>
> This choice is intentional. The idea about bypassing invalid indexes
> for table-level REINDEX is that this would lead to a bloat in the
> number of relations to handling if multiple runs are failing, leading
> to more and more invalid indexes to handle each time. Allowing a
> single invalid non-toast index to be reindexed with CONCURRENTLY can
> be helpful in some cases, like for example a CIC for a unique index
> that failed and was invalid, where the relation already defined can be
> reused.
Ah I see, thanks for the clarification. I guess there's room for improvement
in the comments about that, since the ERRCODE_FEATURE_NOT_SUPPORTED usage is
quite misleading there.
v4 attached, which doesn't prevent a REINDEX INDEX CONCURRENTLY on any invalid
non-TOAST index anymore.
From 7bf57256192806e1caafc3dec68061e473d3978a Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@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.GA21832@telsasoft.com
Backpatch-through: 12
---
src/backend/catalog/index.c | 30 ++++++++++++++++++++++++++++++
src/backend/commands/indexcmds.c | 15 +++++++++++++++
2 files changed, 45 insertions(+)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.cindex 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.cindex 3f3a89fe92..08d74fecd1 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"
@@ -2309,6 +2310,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
@@ -2334,6 +2336,9 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
*/
irel = index_open(indOid, NoLock);
+ invalidtoastindex = (irel->rd_rel->relnamespace == PG_TOAST_NAMESPACE &&+ !irel->rd_index->indisvalid);+
if (irel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
{
ReindexPartitionedIndex(irel);
@@ -2343,6 +2348,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
--
2.20.1
Attachments:
[text/plain] 0001-Don-t-reindex-invalid-indexes-on-TOAST-tables-v4.patch (4.0K, ../20200306123648.GA49431@nol/2-0001-Don-t-reindex-invalid-indexes-on-TOAST-tables-v4.patch)
download | inline diff:
From 7bf57256192806e1caafc3dec68061e473d3978a Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <julien.rouhaud@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.GA21832@telsasoft.com
Backpatch-through: 12
---
src/backend/catalog/index.c | 30 ++++++++++++++++++++++++++++++
src/backend/commands/indexcmds.c | 15 +++++++++++++++
2 files changed, 45 insertions(+)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.cindex 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.cindex 3f3a89fe92..08d74fecd1 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"
@@ -2309,6 +2310,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
@@ -2334,6 +2336,9 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
*/
irel = index_open(indOid, NoLock);
+ invalidtoastindex = (irel->rd_rel->relnamespace == PG_TOAST_NAMESPACE &&+ !irel->rd_index->indisvalid);+
if (irel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
{
ReindexPartitionedIndex(irel);
@@ -2343,6 +2348,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
--
2.20.1
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: rjuju123@gmail.com, michael@paquier.xyz, pryzby@telsasoft.com, sk@zsrv.org, peter.eisentraut@2ndquadrant.com, michael.paquier@gmail.com, andreas@proxel.se
Subject: Re: reindex concurrently and two toast indexes
In-Reply-To: <20200306123648.GA49431@nol>
* 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