On Sun, Aug 09, 2020 at 06:44:23PM -0500, Justin Pryzby wrote:
> On Sun, Aug 09, 2020 at 02:00:09PM +0900, Michael Paquier wrote:
>> For now, I would recommend to focus first on 0001 to add support for
>> partitioned tables and indexes to REINDEX. CIC is much more
>> complicated btw, but I am not entering in the details now.
>>
>> + /* Avoid erroring out */
>> if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
>> {
>> This comment does not help, and actually this becomes incorrect as
>> reindex for this relkind becomes supported once 0001 is done.
>
> I made a minimal change to avoid forgetting to eventually change
> that part.Why not changing it then? We already filter out per relkind in all the code paths calling reindex_relation(), be it in indexcmds.c for schema-level reindex or even tablecmds.c, so I have switched this part to an elog(). >> - We should *not* handle directly partitioned index and/or table in >> ReindexRelationConcurrently() to not complicate the logic where we >> gather all the indexes of a table/matview. So I think that the list >> of partition indexes/tables to work on should be built directly in >> ReindexIndex() and ReindexTable(), and then this should call the >> second part of ReindexMultipleTables() refactored in the previous >> point. > > I think I addressed these mostly as you intended. Mostly. I have been hacking on this patch, and basically rewrote it as the attached. The handling of the memory context used to keep the list of partitions intact across transactions was rather clunky: the context was not reset when we are done, and we would call more APIs than necessary while switching to it, like find_all_inheritors() which could do much more allocations. I have fixed that by minimizing the areas where the private context is used, switching to it only when saving a new OID in the list of partitions, or a session lock (see below for this part). While on it, I found that the test coverage was not enough, so I have extended the set of tests to make sure any concurrent and non-concurrent operation for partitioned tables and indexes change the correct set of relfilenodes for each operation. I have written some custom functions to minimize the duplication (the whole thing cannot be grouped as those commands cannot run in a transaction block). Speaking of which, the patch missed that REINDEX INDEX/TABLE should not run in a transaction block when working on a partitioned relation. And the documentation needs to be clear about the limitation of each operation, so I have written more about all that. The patch also has commented out areas with slashes or such, and I have added some elog() and some asserts to make sure that we don't cross any areas that should not work with partitioned relations. While hacking on this patch, I have found an old bug in the REINDEX logic: we build a list of relations to reindex in ReindexMultipleTables() for schema and database reindexes, but it happens that we don't recheck if the relations listed actually exists or not, so dropping a relation during a large reindex can cause sparse failures because of relations that cannot be found anymore. In the case of this thread, the problem is different though (the proposed patch was full of holes regarding that) and we need to use session locks on the parent *table* partitions (not the indexes) to avoid any issues within the first transaction building the list of relations to work on, similarly to REINDEX CONCURRENTLY. So I fixed this problem this way. For the schema and database cases, I think that we would need to do something similar to VACUUM, aka have an extra code path to skip relations not defined. I'll leave that for another thread. One last thing. I think that the patch is in a rather good shape, but there is one error message I am not happy with when running some commands in a transaction block. Say, this sequence: CREATE TABLE parent_tab (id int) PARTITION BY RANGE (id); CREATE INDEX parent_index ON parent_tab (id); BEGIN; REINDEX INDEX parent_index; -- error ERROR: 25001: REINDEX INDEX cannot run inside a transaction block LOCATION: PreventInTransactionBlock, xact.c:3386 This error can be confusing, because we don't tell directly that the relation involved here is partitioned, and REINDEX INDEX/TABLE are fine when doing their stuff on non-partitions. For other code paths, we have leveraged such errors to use the grammar specific to partitions, for example "CREATE TABLE .. PARTITION OF" or such as these don't cause translation issues, but we don't have a specific syntax of REINDEX for partitioned relations, and I don't think that we need more grammar just for that. The simplest idea I have here is to just use an error callback to set an errcontext(), saying roughly: "while reindexing partitioned table/index %s" while we go through PreventInTransactionBlock(). I have done nothing about that yet but adding an errcallback is simple enough. Perhaps somebody has a different idea here? -- Michael
diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
index c26a102b17..df32f5b201 100644
--- a/src/include/commands/defrem.h
+++ b/src/include/commands/defrem.h
@@ -34,8 +34,10 @@ extern ObjectAddress DefineIndex(Oid relationId,
bool check_not_in_use,
bool skip_build,
bool quiet);
-extern void ReindexIndex(RangeVar *indexRelation, int options, bool concurrent);
-extern Oid ReindexTable(RangeVar *relation, int options, bool concurrent);
+extern void ReindexIndex(RangeVar *indexRelation, int options, bool concurrent,
+ bool isTopLevel);
+extern Oid ReindexTable(RangeVar *relation, int options, bool concurrent,
+ bool isTopLevel);
extern void ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
int options, bool concurrent);
extern char *makeObjectName(const char *name1, const char *name2,
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 1be27eec52..4ea9d18157 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -77,6 +77,7 @@
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/pg_rusage.h"
+#include "utils/rel.h"
#include "utils/snapmgr.h"
#include "utils/syscache.h"
#include "utils/tuplesort.h"
@@ -3447,8 +3448,8 @@ reindex_index(Oid indexId, bool skip_constraint_checks, char persistence,
iRel->rd_rel->relam);
/*
- * The case of reindexing partitioned tables and indexes is handled
- * differently by upper layers, so this case shouldn't arise.
+ * Partitioned indexes should never get processed here, as they have no
+ * physical storage.
*/
if (iRel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
elog(ERROR, "unsupported relation kind for index \"%s\"",
@@ -3661,20 +3662,12 @@ reindex_relation(Oid relid, int flags, int options)
rel = table_open(relid, ShareLock);
/*
- * This may be useful when implemented someday; but that day is not today.
- * For now, avoid erroring out when called in a multi-table context
- * (REINDEX SCHEMA) and happen to come across a partitioned table. The
- * partitions may be reindexed on their own anyway.
+ * Partitioned tables should never get processed here, as they have no
+ * physical storage.
*/
if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- {
- ereport(WARNING,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("REINDEX of partitioned tables is not yet implemented, skipping \"%s\"",
- RelationGetRelationName(rel))));
- table_close(rel, ShareLock);
- return false;
- }
+ elog(ERROR, "unsupported relation kind for relation \"%s\"",
+ RelationGetRelationName(rel));
toast_relid = rel->rd_rel->reltoastrelid;
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 7819266a63..5817114a60 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -88,7 +88,11 @@ static List *ChooseIndexColumnNames(List *indexElems);
static void RangeVarCallbackForReindexIndex(const RangeVar *relation,
Oid relId, Oid oldRelId, void *arg);
static bool ReindexRelationConcurrently(Oid relationOid, int options);
-static void ReindexPartitionedIndex(Relation parentIdx);
+
+static void reindex_partitions(Oid relid, int options, bool concurrent,
+ bool isTopLevel);
+static void reindex_multiple_internal(List *relids, int options,
+ bool concurrent);
static void update_relispartition(Oid relationId, bool newval);
static bool CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts);
@@ -2420,11 +2424,10 @@ ChooseIndexColumnNames(List *indexElems)
* Recreate a specific index.
*/
void
-ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
+ReindexIndex(RangeVar *indexRelation, int options, bool concurrent, bool isTopLevel)
{
struct ReindexIndexCallbackState state;
Oid indOid;
- Relation irel;
char persistence;
/*
@@ -2445,22 +2448,10 @@ ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
RangeVarCallbackForReindexIndex,
&state);
- /*
- * Obtain the current persistence of the existing index. We already hold
- * lock on the index.
- */
- irel = index_open(indOid, NoLock);
-
- if (irel->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- {
- ReindexPartitionedIndex(irel);
- return;
- }
-
- persistence = irel->rd_rel->relpersistence;
- index_close(irel, NoLock);
-
- if (concurrent && persistence != RELPERSISTENCE_TEMP)
+ persistence = get_rel_persistence(indOid);
+ if (get_rel_relkind(indOid) == RELKIND_PARTITIONED_INDEX)
+ reindex_partitions(indOid, options, concurrent, isTopLevel);
+ else if (concurrent && persistence != RELPERSISTENCE_TEMP)
ReindexRelationConcurrently(indOid, options);
else
reindex_index(indOid, false, persistence,
@@ -2542,7 +2533,7 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
* Recreate all indexes of a table (and of its toast table, if any)
*/
Oid
-ReindexTable(RangeVar *relation, int options, bool concurrent)
+ReindexTable(RangeVar *relation, int options, bool concurrent, bool isTopLevel)
{
Oid heapOid;
bool result;
@@ -2560,7 +2551,9 @@ ReindexTable(RangeVar *relation, int options, bool concurrent)
0,
RangeVarCallbackOwnsTable, NULL);
- if (concurrent && get_rel_persistence(heapOid) != RELPERSISTENCE_TEMP)
+ if (get_rel_relkind(heapOid) == RELKIND_PARTITIONED_TABLE)
+ reindex_partitions(heapOid, options, concurrent, isTopLevel);
+ else if (concurrent && get_rel_persistence(heapOid) != RELPERSISTENCE_TEMP)
{
result = ReindexRelationConcurrently(heapOid, options);
@@ -2604,7 +2597,6 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
MemoryContext private_context;
MemoryContext old;
List *relids = NIL;
- ListCell *l;
int num_keys;
bool concurrent_warning = false;
@@ -2688,11 +2680,8 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
* Only regular tables and matviews can have indexes, so ignore any
* other kind of relation.
*
- * It is tempting to also consider partitioned tables here, but that
- * has the problem that if the children are in the same schema, they
- * would be processed twice. Maybe we could have a separate list of
- * partitioned tables, and expand that afterwards into relids,
- * ignoring any duplicates.
+ * Partitioned tables/indexes are skipped but matching leaf
+ * partitions are processed.
*/
if (classtuple->relkind != RELKIND_RELATION &&
classtuple->relkind != RELKIND_MATVIEW)
@@ -2755,22 +2744,154 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
table_endscan(scan);
table_close(relationRelation, AccessShareLock);
- /* Now reindex each rel in a separate transaction */
+ /*
+ * Process each relation listed in a separate transaction. Note that
+ * this commits and then starts a new transaction immediately.
+ */
+ reindex_multiple_internal(relids, options, concurrent);
+
+ MemoryContextDelete(private_context);
+}
+
+static void
+reindex_partitions(Oid relid, int options, bool concurrent,
+ bool isTopLevel)
+{
+ List *partitions = NIL;
+ char relkind = get_rel_relkind(relid);
+ MemoryContext reindex_context;
+ List *inhoids;
+ ListCell *lc;
+ LOCKMODE lockmode = concurrent ?
+ ShareUpdateExclusiveLock : AccessExclusiveLock;
+ List *partLocks = NIL;
+
+ Assert(relkind == RELKIND_PARTITIONED_INDEX ||
+ relkind == RELKIND_PARTITIONED_TABLE);
+
+ /* XXX: message is not great here for partitioned tables or indexes */
+ PreventInTransactionBlock(isTopLevel,
+ relkind == RELKIND_PARTITIONED_TABLE ?
+ "REINDEX TABLE" : "REINDEX INDEX");
+
+ /*
+ * Create special memory context for cross-transaction storage.
+ *
+ * Since it is a child of PortalContext, it will go away eventually even
+ * if we suffer an error so there is no need for special abort cleanup
+ * logic.
+ */
+ reindex_context = AllocSetContextCreate(PortalContext, "Reindex",
+ ALLOCSET_DEFAULT_SIZES);
+
+ /* ShareLock is enough to prevent schema modifications */
+ inhoids = find_all_inheritors(relid, ShareLock, NULL);
+
+ /*
+ * The list of relations to reindex are the physical partitions
+ * of the tree so discard any partitioned table or index, and take
+ * some session locks. For partition indexes, the parent table
+ * needs to be locked.
+ */
+ foreach (lc, inhoids)
+ {
+ Oid partoid = lfirst_oid(lc);
+ Oid parentoid;
+ char partkind = get_rel_relkind(partoid);
+ Relation rel;
+ MemoryContext old_context;
+ LockRelId *lockrelid;
+
+ if (partkind == RELKIND_PARTITIONED_INDEX ||
+ partkind == RELKIND_PARTITIONED_TABLE)
+ continue;
+
+ parentoid = (partkind == RELKIND_INDEX) ?
+ IndexGetRelation(partoid, false) : partoid;
+
+ rel = relation_open(parentoid, lockmode);
+
+ /* Save partition OID and the session lock of parent table */
+ old_context = MemoryContextSwitchTo(reindex_context);
+ partitions = lappend_oid(partitions, partoid);
+ lockrelid = palloc(sizeof(LockRelId));
+ *lockrelid = rel->rd_lockInfo.lockRelId;
+ partLocks = lappend(partLocks, lockrelid);
+ MemoryContextSwitchTo(old_context);
+
+ relation_close(rel, NoLock);
+ LockRelationIdForSession(lockrelid, lockmode);
+ }
+
+ /*
+ * Process each partition listed in a separate transaction. Note that
+ * this commits and then starts a new transaction immediately.
+ */
+ reindex_multiple_internal(partitions, options, concurrent);
+
+ /* Finally, release the session-level locks */
+ foreach(lc, partLocks)
+ {
+ LockRelId *lockrelid = (LockRelId *) lfirst(lc);
+
+ UnlockRelationIdForSession(lockrelid, lockmode);
+ }
+
+ /*
+ * Clean up working storage --- note we must do this after
+ * StartTransactionCommand, else we might be trying to delete the active
+ * context!
+ */
+ MemoryContextDelete(reindex_context);
+}
+
+/*
+ * reindex_multiple_internal
+ *
+ * Reindex a list of relations, each one being processed in its own
+ * transaction. This commits the existing transaction immediately,
+ * and starts a new transaction when done.
+ */
+static void
+reindex_multiple_internal(List *relids, int options, bool concurrent)
+{
+ ListCell *l;
+
PopActiveSnapshot();
CommitTransactionCommand();
+
foreach(l, relids)
{
Oid relid = lfirst_oid(l);
+ char relkind;
+ char relpersistence;
StartTransactionCommand();
+
/* functions in indexes may want a snapshot set */
PushActiveSnapshot(GetTransactionSnapshot());
+ relkind = get_rel_relkind(relid);
+ relpersistence = get_rel_persistence(relid);
+
+ /*
+ * Partitioned tables and indexes can never be processed directly, and
+ * a list of their leaves should be built first.
+ */
+ Assert(relkind != RELKIND_PARTITIONED_INDEX &&
+ relkind != RELKIND_PARTITIONED_TABLE);
+
if (concurrent && get_rel_persistence(relid) != RELPERSISTENCE_TEMP)
{
(void) ReindexRelationConcurrently(relid, options);
/* ReindexRelationConcurrently() does the verbose output */
}
+ else if (relkind == RELKIND_INDEX)
+ {
+ reindex_index(relid, false, relpersistence,
+ options | REINDEXOPT_REPORT_PROGRESS);
+ PopActiveSnapshot();
+ }
else
{
bool result;
@@ -2791,9 +2912,8 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
CommitTransactionCommand();
}
- StartTransactionCommand();
- MemoryContextDelete(private_context);
+ StartTransactionCommand();
}
@@ -2805,8 +2925,7 @@ ReindexMultipleTables(const char *objectName, ReindexObjectType objectKind,
* view. For tables and materialized views, all its indexes will be rebuilt,
* excluding invalid indexes and any indexes used in exclusion constraints,
* but including its associated toast table indexes. For indexes, the index
- * itself will be rebuilt. If 'relationOid' belongs to a partitioned table
- * then we issue a warning to mention these are not yet supported.
+ * itself will be rebuilt.
*
* The locks taken on parent tables and involved indexes are kept until the
* transaction is committed, at which point a session lock is taken on each
@@ -3010,13 +3129,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
MemoryContextSwitchTo(oldcontext);
break;
}
+
case RELKIND_PARTITIONED_TABLE:
- /* see reindex_relation() */
- ereport(WARNING,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("REINDEX of partitioned tables is not yet implemented, skipping \"%s\"",
- get_rel_name(relationOid))));
- return false;
+ case RELKIND_PARTITIONED_INDEX:
default:
/* Return error if type of relation is not supported */
ereport(ERROR,
@@ -3477,20 +3592,6 @@ ReindexRelationConcurrently(Oid relationOid, int options)
return true;
}
-/*
- * ReindexPartitionedIndex
- * Reindex each child of the given partitioned index.
- *
- * Not yet implemented.
- */
-static void
-ReindexPartitionedIndex(Relation parentIdx)
-{
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("REINDEX is not yet implemented for partitioned indexes")));
-}
-
/*
* Insert or delete an appropriate pg_inherits tuple to make the given index
* be a partition of the indicated parent index.
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 9b0c376c8c..fd6bc65c18 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -926,10 +926,12 @@ standard_ProcessUtility(PlannedStmt *pstmt,
switch (stmt->kind)
{
case REINDEX_OBJECT_INDEX:
- ReindexIndex(stmt->relation, stmt->options, stmt->concurrent);
+ ReindexIndex(stmt->relation, stmt->options,
+ stmt->concurrent, isTopLevel);
break;
case REINDEX_OBJECT_TABLE:
- ReindexTable(stmt->relation, stmt->options, stmt->concurrent);
+ ReindexTable(stmt->relation, stmt->options,
+ stmt->concurrent, isTopLevel);
break;
case REINDEX_OBJECT_SCHEMA:
case REINDEX_OBJECT_SYSTEM:
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index e3e6634d7e..8d9923c7ab 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2196,18 +2196,6 @@ SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_ind
concur_reindex_part_index_0_2 | concur_reindex_part_index_0 | 2
(5 rows)
--- REINDEX fails for partitioned indexes
-REINDEX INDEX concur_reindex_part_index_10;
-ERROR: REINDEX is not yet implemented for partitioned indexes
-REINDEX INDEX CONCURRENTLY concur_reindex_part_index_10;
-ERROR: REINDEX is not yet implemented for partitioned indexes
--- REINDEX is a no-op for partitioned tables
-REINDEX TABLE concur_reindex_part_10;
-WARNING: REINDEX of partitioned tables is not yet implemented, skipping "concur_reindex_part_10"
-NOTICE: table "concur_reindex_part_10" has no indexes to reindex
-REINDEX TABLE CONCURRENTLY concur_reindex_part_10;
-WARNING: REINDEX of partitioned tables is not yet implemented, skipping "concur_reindex_part_10"
-NOTICE: table "concur_reindex_part_10" has no indexes that can be reindexed concurrently
SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_index')
ORDER BY relid, level;
relid | parentrelid | level
@@ -2320,6 +2308,150 @@ SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_ind
concur_reindex_part_index_0_2 | concur_reindex_part_index_0 | 2
(5 rows)
+-- REINDEX for partitioned indexes
+-- REINDEX TABLE fails for partitioned indexes
+-- Top-most parent index
+REINDEX TABLE concur_reindex_part_index;
+ERROR: "concur_reindex_part_index" is not a table or materialized view
+REINDEX TABLE CONCURRENTLY concur_reindex_part_index;
+ERROR: "concur_reindex_part_index" is not a table or materialized view
+-- Partitioned index with no leaves
+REINDEX TABLE concur_reindex_part_index_10;
+ERROR: "concur_reindex_part_index_10" is not a table or materialized view
+REINDEX TABLE CONCURRENTLY concur_reindex_part_index_10;
+ERROR: "concur_reindex_part_index_10" is not a table or materialized view
+-- Cannot run in a transaction block
+BEGIN;
+REINDEX INDEX concur_reindex_part_index;
+ERROR: REINDEX INDEX cannot run inside a transaction block
+ROLLBACK;
+-- Helper functions to track changes of relfilenodes in a partition tree.
+-- Create a table tracking the relfilenode state.
+CREATE OR REPLACE FUNCTION create_relfilenode_part(relname text, indname text)
+ RETURNS VOID AS
+ $func$
+ BEGIN
+ EXECUTE format('
+ CREATE TABLE %I AS
+ SELECT oid, relname, relfilenode, relkind, reltoastrelid
+ FROM pg_class
+ WHERE oid IN
+ (SELECT relid FROM pg_partition_tree(''%I''));',
+ relname, indname);
+ END
+ $func$ LANGUAGE plpgsql;
+CREATE OR REPLACE FUNCTION compare_relfilenode_part(tabname text)
+ RETURNS TABLE (relname name, relkind "char", state text) AS
+ $func$
+ BEGIN
+ RETURN QUERY EXECUTE
+ format(
+ 'SELECT b.relname,
+ b.relkind,
+ CASE WHEN a.relfilenode = b.relfilenode THEN ''relfilenode is unchanged''
+ ELSE ''relfilenode has changed'' END
+ -- Do not join with OID here as CONCURRENTLY changes it.
+ FROM %I b JOIN pg_class a ON b.relname = a.relname
+ ORDER BY 1;', tabname);
+ END
+ $func$ LANGUAGE plpgsql;
+-- Check that expected relfilenodes are changed, non-concurrent case.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+ create_relfilenode_part
+-------------------------
+
+(1 row)
+
+REINDEX INDEX concur_reindex_part_index;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+ relname | relkind | state
+-------------------------------+---------+--------------------------
+ concur_reindex_part_index | I | relfilenode is unchanged
+ concur_reindex_part_index_0 | I | relfilenode is unchanged
+ concur_reindex_part_index_0_1 | i | relfilenode has changed
+ concur_reindex_part_index_0_2 | i | relfilenode has changed
+ concur_reindex_part_index_10 | I | relfilenode is unchanged
+(5 rows)
+
+DROP TABLE reindex_index_status;
+-- concurrent case.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+ create_relfilenode_part
+-------------------------
+
+(1 row)
+
+REINDEX INDEX CONCURRENTLY concur_reindex_part_index;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+ relname | relkind | state
+-------------------------------+---------+--------------------------
+ concur_reindex_part_index | I | relfilenode is unchanged
+ concur_reindex_part_index_0 | I | relfilenode is unchanged
+ concur_reindex_part_index_0_1 | i | relfilenode has changed
+ concur_reindex_part_index_0_2 | i | relfilenode has changed
+ concur_reindex_part_index_10 | I | relfilenode is unchanged
+(5 rows)
+
+DROP TABLE reindex_index_status;
+-- REINDEX for partitioned tables
+-- REINDEX INDEX fails for partitioned tables
+-- Top-most parent
+REINDEX INDEX concur_reindex_part;
+ERROR: "concur_reindex_part" is not an index
+REINDEX INDEX CONCURRENTLY concur_reindex_part;
+ERROR: "concur_reindex_part" is not an index
+-- Partitioned with no leaves
+REINDEX INDEX concur_reindex_part_10;
+ERROR: "concur_reindex_part_10" is not an index
+REINDEX INDEX CONCURRENTLY concur_reindex_part_10;
+ERROR: "concur_reindex_part_10" is not an index
+-- Cannot run in a transaction block
+BEGIN;
+REINDEX TABLE concur_reindex_part;
+ERROR: REINDEX TABLE cannot run inside a transaction block
+ROLLBACK;
+-- Check that expected relfilenodes are changed, non-concurrent case.
+-- Note that the partition tree changes of the *indexes* need to be checked.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+ create_relfilenode_part
+-------------------------
+
+(1 row)
+
+REINDEX TABLE concur_reindex_part;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+ relname | relkind | state
+-------------------------------+---------+--------------------------
+ concur_reindex_part_index | I | relfilenode is unchanged
+ concur_reindex_part_index_0 | I | relfilenode is unchanged
+ concur_reindex_part_index_0_1 | i | relfilenode has changed
+ concur_reindex_part_index_0_2 | i | relfilenode has changed
+ concur_reindex_part_index_10 | I | relfilenode is unchanged
+(5 rows)
+
+DROP TABLE reindex_index_status;
+-- concurrent case.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+ create_relfilenode_part
+-------------------------
+
+(1 row)
+
+REINDEX TABLE CONCURRENTLY concur_reindex_part;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+ relname | relkind | state
+-------------------------------+---------+--------------------------
+ concur_reindex_part_index | I | relfilenode is unchanged
+ concur_reindex_part_index_0 | I | relfilenode is unchanged
+ concur_reindex_part_index_0_1 | i | relfilenode has changed
+ concur_reindex_part_index_0_2 | i | relfilenode has changed
+ concur_reindex_part_index_10 | I | relfilenode is unchanged
+(5 rows)
+
+DROP TABLE reindex_index_status;
+DROP FUNCTION create_relfilenode_part;
+DROP FUNCTION compare_relfilenode_part;
+-- Cleanup of partition tree used for REINDEX test.
DROP TABLE concur_reindex_part;
-- Check errors
-- Cannot run inside a transaction block
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index f3667bacdc..6d98b73365 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -903,12 +903,6 @@ CREATE INDEX concur_reindex_part_index_0_2 ON ONLY concur_reindex_part_0_2 (c1);
ALTER INDEX concur_reindex_part_index_0 ATTACH PARTITION concur_reindex_part_index_0_2;
SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_index')
ORDER BY relid, level;
--- REINDEX fails for partitioned indexes
-REINDEX INDEX concur_reindex_part_index_10;
-REINDEX INDEX CONCURRENTLY concur_reindex_part_index_10;
--- REINDEX is a no-op for partitioned tables
-REINDEX TABLE concur_reindex_part_10;
-REINDEX TABLE CONCURRENTLY concur_reindex_part_10;
SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_index')
ORDER BY relid, level;
-- REINDEX should preserve dependencies of partition tree.
@@ -948,6 +942,88 @@ WHERE classid = 'pg_class'::regclass AND
ORDER BY 1, 2;
SELECT relid, parentrelid, level FROM pg_partition_tree('concur_reindex_part_index')
ORDER BY relid, level;
+
+-- REINDEX for partitioned indexes
+-- REINDEX TABLE fails for partitioned indexes
+-- Top-most parent index
+REINDEX TABLE concur_reindex_part_index;
+REINDEX TABLE CONCURRENTLY concur_reindex_part_index;
+-- Partitioned index with no leaves
+REINDEX TABLE concur_reindex_part_index_10;
+REINDEX TABLE CONCURRENTLY concur_reindex_part_index_10;
+-- Cannot run in a transaction block
+BEGIN;
+REINDEX INDEX concur_reindex_part_index;
+ROLLBACK;
+-- Helper functions to track changes of relfilenodes in a partition tree.
+-- Create a table tracking the relfilenode state.
+CREATE OR REPLACE FUNCTION create_relfilenode_part(relname text, indname text)
+ RETURNS VOID AS
+ $func$
+ BEGIN
+ EXECUTE format('
+ CREATE TABLE %I AS
+ SELECT oid, relname, relfilenode, relkind, reltoastrelid
+ FROM pg_class
+ WHERE oid IN
+ (SELECT relid FROM pg_partition_tree(''%I''));',
+ relname, indname);
+ END
+ $func$ LANGUAGE plpgsql;
+CREATE OR REPLACE FUNCTION compare_relfilenode_part(tabname text)
+ RETURNS TABLE (relname name, relkind "char", state text) AS
+ $func$
+ BEGIN
+ RETURN QUERY EXECUTE
+ format(
+ 'SELECT b.relname,
+ b.relkind,
+ CASE WHEN a.relfilenode = b.relfilenode THEN ''relfilenode is unchanged''
+ ELSE ''relfilenode has changed'' END
+ -- Do not join with OID here as CONCURRENTLY changes it.
+ FROM %I b JOIN pg_class a ON b.relname = a.relname
+ ORDER BY 1;', tabname);
+ END
+ $func$ LANGUAGE plpgsql;
+-- Check that expected relfilenodes are changed, non-concurrent case.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+REINDEX INDEX concur_reindex_part_index;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+DROP TABLE reindex_index_status;
+-- concurrent case.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+REINDEX INDEX CONCURRENTLY concur_reindex_part_index;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+DROP TABLE reindex_index_status;
+
+-- REINDEX for partitioned tables
+-- REINDEX INDEX fails for partitioned tables
+-- Top-most parent
+REINDEX INDEX concur_reindex_part;
+REINDEX INDEX CONCURRENTLY concur_reindex_part;
+-- Partitioned with no leaves
+REINDEX INDEX concur_reindex_part_10;
+REINDEX INDEX CONCURRENTLY concur_reindex_part_10;
+-- Cannot run in a transaction block
+BEGIN;
+REINDEX TABLE concur_reindex_part;
+ROLLBACK;
+-- Check that expected relfilenodes are changed, non-concurrent case.
+-- Note that the partition tree changes of the *indexes* need to be checked.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+REINDEX TABLE concur_reindex_part;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+DROP TABLE reindex_index_status;
+-- concurrent case.
+SELECT create_relfilenode_part('reindex_index_status', 'concur_reindex_part_index');
+REINDEX TABLE CONCURRENTLY concur_reindex_part;
+SELECT * FROM compare_relfilenode_part('reindex_index_status');
+DROP TABLE reindex_index_status;
+
+DROP FUNCTION create_relfilenode_part;
+DROP FUNCTION compare_relfilenode_part;
+
+-- Cleanup of partition tree used for REINDEX test.
DROP TABLE concur_reindex_part;
-- Check errors
diff --git a/doc/src/sgml/ref/reindex.sgml b/doc/src/sgml/ref/reindex.sgml
index aac5d5be23..439e52bbc8 100644
--- a/doc/src/sgml/ref/reindex.sgml
+++ b/doc/src/sgml/ref/reindex.sgml
@@ -88,7 +88,9 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN
<term><literal>INDEX</literal></term>
<listitem>
<para>
- Recreate the specified index.
+ Recreate the specified index. This form of <command>REINDEX</command>
+ cannot be executed inside a transaction block when used with a
+ partitioned index.
</para>
</listitem>
</varlistentry>
@@ -99,6 +101,8 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN
<para>
Recreate all indexes of the specified table. If the table has a
secondary <quote>TOAST</quote> table, that is reindexed as well.
+ This form of <command>REINDEX</command> cannot be executed inside a
+ transaction block when used with a partitioned table.
</para>
</listitem>
</varlistentry>
@@ -259,8 +263,12 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN
</para>
<para>
- Reindexing partitioned tables or partitioned indexes is not supported.
- Each individual partition can be reindexed separately instead.
+ Reindexing partitioned indexes or partitioned tables is supported
+ with respectively <command>REINDEX INDEX</command> or
+ <command>REINDEX TABLE</command>. Each partition of the partitioned
+ relation defined is rebuilt in its own transaction. Those commands
+ cannot be used inside a transaction block when working on a
+ partitioned table or index.
</para>
<refsect2 id="sql-reindex-concurrently" xreflabel="Rebuilding Indexes Concurrently">
signature.asc
Description: PGP signature
