Thanks for the updated patch, Fabrizio.

On Tue, Nov 11, 2014 at 7:44 AM, Fabrízio de Royes Mello <
fabriziome...@gmail.com> wrote:

> > A couple of minor comments about this patch:
> > 1) Reading it, I am wondering if it would not be finally time to
> > switch to a macro to get a relation's persistence, something like
> > RelationGetPersistence in rel.h... Not related directly to this patch.
>
> Good idea... I'll provide a patch soon.
>
I created a thread dedicated to that:
http://www.postgresql.org/message-id/cab7npqseb+hwitxfwkqypa7+9bjolnxio47qsro3hcbsoq0...@mail.gmail.com
Now few people cared enough to comment :)

> 2) reindex_index has as new argument a relpersislence value for the
> > new index. reindex_relation has differently a new set of flags to
> > enforce the relpersistence of all the underling indexes. Wouldn't it
> > be better for API consistency to pass directly a relpersistence value
> > through reindex_relation? In any case, the comment block of
> > reindex_relation does not contain a description of the new flags.
>
> I did it because the ReindexDatabase build a list of oids to run
> reindex_relation for each item of the list. I can change the list to store
> the relpersistence also, but this can lead us to a concurrency trouble
> because if one session execute REINDEX DATABASE and other session run
> "ALTER TABLE name SET {LOGGED|UNLOGGED}" during the building of the list
> the reindex can lead us to a inconsistent state.
>
Fair point. I forgot this code path.

>
Except a typo with s/rebuilded/rebuilt/ in the patch, corrected in the
patch attached with some extra comments bundled, it seems to be that this
patch can be passed to a committer, so marking it so.

Btw, perhaps this diff should be pushed as a different patch as this is a
rather different thing:
-       if (heapRelation->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED
&&
+       if (indexRelation->rd_rel->relpersistence ==
RELPERSISTENCE_UNLOGGED &&
                !smgrexists(indexRelation->rd_smgr, INIT_FORKNUM)
As this is a correctness fix, it does not seem necessary to back-patch it:
the parent relation always has the same persistence as its indexes.
Regards,
-- 
Michael
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 912038a..a0a81e8 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1980,7 +1980,7 @@ index_build(Relation heapRelation,
 	 * created it, or truncated twice in a subsequent transaction, the
 	 * relfilenode won't change, and nothing needs to be done here.
 	 */
-	if (heapRelation->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED &&
+	if (indexRelation->rd_rel->relpersistence == RELPERSISTENCE_UNLOGGED &&
 		!smgrexists(indexRelation->rd_smgr, INIT_FORKNUM))
 	{
 		RegProcedure ambuildempty = indexRelation->rd_am->ambuildempty;
@@ -3130,7 +3130,7 @@ IndexGetRelation(Oid indexId, bool missing_ok)
  * reindex_index - This routine is used to recreate a single index
  */
 void
-reindex_index(Oid indexId, bool skip_constraint_checks)
+reindex_index(Oid indexId, bool skip_constraint_checks, char relpersistence)
 {
 	Relation	iRel,
 				heapRelation;
@@ -3191,6 +3191,9 @@ reindex_index(Oid indexId, bool skip_constraint_checks)
 			indexInfo->ii_ExclusionStrats = NULL;
 		}
 
+		/* Set the relpersistence of the new index */
+		iRel->rd_rel->relpersistence = relpersistence;
+
 		/* We'll build a new physical relation for the index */
 		RelationSetNewRelfilenode(iRel, InvalidTransactionId,
 								  InvalidMultiXactId);
@@ -3310,6 +3313,12 @@ reindex_index(Oid indexId, bool skip_constraint_checks)
  * performance, other callers should include the flag only after transforming
  * the data in a manner that risks a change in constraint validity.
  *
+ * REINDEX_REL_FORCE_UNLOGGED: if true, set the persistence of the rebuilt
+ * indexes to unlogged.
+ *
+ * REINDEX_REL_FORCE_LOGGED: if true, set the persistence of the rebuilt
+ * indexes to permanent.
+ *
  * Returns true if any indexes were rebuilt (including toast table's index
  * when relevant).  Note that a CommandCounterIncrement will occur after each
  * index rebuild.
@@ -3389,11 +3398,19 @@ reindex_relation(Oid relid, int flags)
 		foreach(indexId, indexIds)
 		{
 			Oid			indexOid = lfirst_oid(indexId);
+			char		relpersistence = rel->rd_rel->relpersistence;
 
 			if (is_pg_class)
 				RelationSetIndexList(rel, doneIndexes, InvalidOid);
 
-			reindex_index(indexOid, !(flags & REINDEX_REL_CHECK_CONSTRAINTS));
+			/* Check for flags to enforce UNLOGGED or PERMANENT persistence */
+			if (flags & REINDEX_REL_FORCE_UNLOGGED)
+				relpersistence = RELPERSISTENCE_UNLOGGED;
+			else if (flags & REINDEX_REL_FORCE_PERMANENT)
+				relpersistence = RELPERSISTENCE_PERMANENT;
+
+			reindex_index(indexOid, !(flags & REINDEX_REL_CHECK_CONSTRAINTS),
+						  relpersistence);
 
 			CommandCounterIncrement();
 
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 6a578ec..e067abc 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -589,7 +589,8 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
 	 */
 	finish_heap_swap(tableOid, OIDNewHeap, is_system_catalog,
 					 swap_toast_by_content, false, true,
-					 frozenXid, cutoffMulti);
+					 frozenXid, cutoffMulti,
+					 OldHeap->rd_rel->relpersistence);
 }
 
 
@@ -1475,7 +1476,8 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
 				 bool check_constraints,
 				 bool is_internal,
 				 TransactionId frozenXid,
-				 MultiXactId cutoffMulti)
+				 MultiXactId cutoffMulti,
+				 char newrelpersistence)
 {
 	ObjectAddress object;
 	Oid			mapped_tables[4];
@@ -1519,6 +1521,16 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
 	reindex_flags = REINDEX_REL_SUPPRESS_INDEX_USE;
 	if (check_constraints)
 		reindex_flags |= REINDEX_REL_CHECK_CONSTRAINTS;
+
+	/*
+	 * Ensure that the indexes of this relation have the same persistence
+	 * as their parent relation.
+	 */
+	if (newrelpersistence == RELPERSISTENCE_UNLOGGED)
+		reindex_flags |= REINDEX_REL_FORCE_UNLOGGED;
+	else if (newrelpersistence == RELPERSISTENCE_PERMANENT)
+		reindex_flags |= REINDEX_REL_FORCE_PERMANENT;
+
 	reindex_relation(OIDOldHeap, reindex_flags);
 
 	/*
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 0205595..12b4ac7 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -1689,7 +1689,7 @@ ReindexIndex(RangeVar *indexRelation)
 									  RangeVarCallbackForReindexIndex,
 									  (void *) &heapOid);
 
-	reindex_index(indOid, false);
+	reindex_index(indOid, false, indexRelation->relpersistence);
 
 	return indOid;
 }
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index 523ba35..b19da4a 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -67,7 +67,7 @@ static void mv_GenerateOper(StringInfo buf, Oid opoid);
 
 static void refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
 						 int save_sec_context);
-static void refresh_by_heap_swap(Oid matviewOid, Oid OIDNewHeap);
+static void refresh_by_heap_swap(Oid matviewOid, Oid OIDNewHeap, char relpersistence);
 
 static void OpenMatViewIncrementalMaintenance(void);
 static void CloseMatViewIncrementalMaintenance(void);
@@ -303,7 +303,7 @@ ExecRefreshMatView(RefreshMatViewStmt *stmt, const char *queryString,
 		Assert(matview_maintenance_depth == old_depth);
 	}
 	else
-		refresh_by_heap_swap(matviewOid, OIDNewHeap);
+		refresh_by_heap_swap(matviewOid, OIDNewHeap, relpersistence);
 
 	/* Roll back any GUC changes */
 	AtEOXact_GUC(false, save_nestlevel);
@@ -759,10 +759,10 @@ refresh_by_match_merge(Oid matviewOid, Oid tempOid, Oid relowner,
  * swapping is handled by the called function, so it is not needed here.
  */
 static void
-refresh_by_heap_swap(Oid matviewOid, Oid OIDNewHeap)
+refresh_by_heap_swap(Oid matviewOid, Oid OIDNewHeap, char relpersistence)
 {
 	finish_heap_swap(matviewOid, OIDNewHeap, false, false, true, true,
-					 RecentXmin, ReadNextMultiXactId());
+					 RecentXmin, ReadNextMultiXactId(), relpersistence);
 }
 
 
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 714a9f1..093224f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -393,7 +393,6 @@ static void ATExecClusterOn(Relation rel, const char *indexName,
 				LOCKMODE lockmode);
 static void ATExecDropCluster(Relation rel, LOCKMODE lockmode);
 static bool ATPrepChangePersistence(Relation rel, bool toLogged);
-static void ATChangeIndexesPersistence(Oid relid, char relpersistence);
 static void ATPrepSetTableSpace(AlteredTableInfo *tab, Relation rel,
 					char *tablespacename, LOCKMODE lockmode);
 static void ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode);
@@ -3735,16 +3734,6 @@ ATRewriteTables(List **wqueue, LOCKMODE lockmode)
 			ATRewriteTable(tab, OIDNewHeap, lockmode);
 
 			/*
-			 * Change the persistence marking of indexes, if necessary.  This
-			 * is so that the new copies are built with the right persistence
-			 * in the reindex step below.  Note we cannot do this earlier,
-			 * because the rewrite step might read the indexes, and that would
-			 * cause buffers for them to have the wrong setting.
-			 */
-			if (tab->chgPersistence)
-				ATChangeIndexesPersistence(tab->relid, tab->newrelpersistence);
-
-			/*
 			 * Swap the physical files of the old and new heaps, then rebuild
 			 * indexes and discard the old heap.  We can use RecentXmin for
 			 * the table's new relfrozenxid because we rewrote all the tuples
@@ -3756,7 +3745,8 @@ ATRewriteTables(List **wqueue, LOCKMODE lockmode)
 							 false, false, true,
 							 !OidIsValid(tab->newTableSpace),
 							 RecentXmin,
-							 ReadNextMultiXactId());
+							 ReadNextMultiXactId(),
+							 persistence);
 		}
 		else
 		{
@@ -10880,48 +10870,6 @@ ATPrepChangePersistence(Relation rel, bool toLogged)
 }
 
 /*
- * Update the pg_class entry of each index for the given relation to the
- * given persistence.
- */
-static void
-ATChangeIndexesPersistence(Oid relid, char relpersistence)
-{
-	Relation	rel;
-	Relation	pg_class;
-	List	   *indexes;
-	ListCell   *cell;
-
-	pg_class = heap_open(RelationRelationId, RowExclusiveLock);
-
-	/* We already have a lock on the table */
-	rel = relation_open(relid, NoLock);
-	indexes = RelationGetIndexList(rel);
-	foreach(cell, indexes)
-	{
-		Oid			indexid = lfirst_oid(cell);
-		HeapTuple	tuple;
-		Form_pg_class pg_class_form;
-
-		tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(indexid));
-		if (!HeapTupleIsValid(tuple))
-			elog(ERROR, "cache lookup failed for relation %u",
-				 indexid);
-
-		pg_class_form = (Form_pg_class) GETSTRUCT(tuple);
-		pg_class_form->relpersistence = relpersistence;
-		simple_heap_update(pg_class, &tuple->t_self, tuple);
-
-		/* keep catalog indexes current */
-		CatalogUpdateIndexes(pg_class, tuple);
-
-		heap_freetuple(tuple);
-	}
-
-	heap_close(pg_class, RowExclusiveLock);
-	heap_close(rel, NoLock);
-}
-
-/*
  * Execute ALTER TABLE SET SCHEMA
  */
 Oid
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index e8ed999..e486007 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -3077,6 +3077,7 @@ RelationSetNewRelfilenode(Relation relation, TransactionId freezeXid,
 	}
 	classform->relfrozenxid = freezeXid;
 	classform->relminmxid = minmulti;
+	classform->relpersistence = relation->rd_rel->relpersistence;
 
 	simple_heap_update(pg_class, &tuple->t_self, tuple);
 	CatalogUpdateIndexes(pg_class, tuple);
diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h
index c36a729..76b5d57 100644
--- a/src/include/catalog/index.h
+++ b/src/include/catalog/index.h
@@ -111,12 +111,15 @@ extern void validate_index(Oid heapId, Oid indexId, Snapshot snapshot);
 
 extern void index_set_state_flags(Oid indexId, IndexStateFlagsAction action);
 
-extern void reindex_index(Oid indexId, bool skip_constraint_checks);
+extern void reindex_index(Oid indexId, bool skip_constraint_checks,
+						  char relpersistence);
 
 /* Flag bits for reindex_relation(): */
 #define REINDEX_REL_PROCESS_TOAST		0x01
 #define REINDEX_REL_SUPPRESS_INDEX_USE	0x02
 #define REINDEX_REL_CHECK_CONSTRAINTS	0x04
+#define REINDEX_REL_FORCE_UNLOGGED		0x08
+#define REINDEX_REL_FORCE_PERMANENT		0x10
 
 extern bool reindex_relation(Oid relid, int flags);
 
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
index f7730a9..0b7e877 100644
--- a/src/include/commands/cluster.h
+++ b/src/include/commands/cluster.h
@@ -33,6 +33,7 @@ extern void finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
 				 bool check_constraints,
 				 bool is_internal,
 				 TransactionId frozenXid,
-				 MultiXactId minMulti);
+				 MultiXactId minMulti,
+				 char newrelpersistence);
 
 #endif   /* CLUSTER_H */
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to