Thanks for the updated patch, Fabrizio.
On Tue, Nov 11, 2014 at 7:44 AM, Fabrízio de Royes Mello <
[email protected]> 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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
