On Sun, Dec 8, 2013 at 10:51 AM, Peter Eisentraut <pete...@gmx.net> wrote:
> On Tue, 2013-11-19 at 18:24 +0100, Andres Freund wrote:
>> On 2013-11-19 12:23:30 -0500, Robert Haas wrote:
>> > On Mon, Nov 18, 2013 at 11:45 AM, Andres Freund <and...@2ndquadrant.com>
>> > wrote:
>> > >> Yes, we probably should make a decision, unless Robert's idea can be
>> > >> implemented. We have to balance the ease of debugging MVCC failures
>> > >> with the interface we give to the user community.
>> > >
>> > > Imo that patch really doesn't need too much further work.
>> >
>> > Would you care to submit a version you're happy with?
>>
>> I'll give it a spin sometime this week.
>
> I'm setting the CLUSTER FREEZE patch as returned with feedback, until
> this other patch has been considered.
The other patch in question is now committed. Here's a patch for
this, which does somewhat more extensive surgery than previously
proposed (actually, I wrote it from scratch, before looking at the
prior submission). It's basically the same idea, though. Note that
both versions of the patch affect not only CLUSTER, but also VACUUM
FULL.
I suspect we ought to extend this to rewriting variants of ALTER TABLE
as well, but a little thought is needed there. ATRewriteTables()
appears to just call heap_insert() for each updated row, which if I'm
not mistaken is an MVCC violation - offhand, it seems like a
transaction with an older MVCC snapshot could access the table for
this first time after the rewriter commits, and fail to see rows which
would have appeared to be there before the rewrite. (I haven't
actually tested this, so watch me be wrong.) If we're OK with an MVCC
violation here, we could just pass HEAP_INSERT_FROZEN and have a
slightly different flavor of violation; if not, this needs some kind
of more extensive surgery quite apart from what we do about freezing.
Review of the attached appreciated...
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index eb71581..1e98473 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -102,7 +102,9 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] ANALYZE [ <replaceable class="PARAMETER">
Specifying <literal>FREEZE</literal> is equivalent to performing
<command>VACUUM</command> with the
<xref linkend="guc-vacuum-freeze-min-age"> parameter
- set to zero.
+ set to zero. Aggressive freezing is always performed when the
+ table is rewritten, so this option is redundant when <literal>FULL</>
+ is specified.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index deec77d..634754c 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -345,7 +345,7 @@ rewrite_heap_tuple(RewriteState state,
/*
* While we have our hands on the tuple, we may as well freeze any
- * very-old xmin or xmax, so that future VACUUM effort can be saved.
+ * eligible xmin or xmax, so that future VACUUM effort can be saved.
*/
heap_freeze_tuple(new_tuple->t_data, state->rs_freeze_xid,
state->rs_cutoff_multi);
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 0b8ac8c..9f41278 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -64,12 +64,10 @@ typedef struct
} RelToCluster;
-static void rebuild_relation(Relation OldHeap, Oid indexOid,
- int freeze_min_age, int freeze_table_age, bool verbose);
+static void rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose);
static void copy_heap_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
- int freeze_min_age, int freeze_table_age, bool verbose,
- bool *pSwapToastByContent, TransactionId *pFreezeXid,
- MultiXactId *pCutoffMulti);
+ bool verbose, bool *pSwapToastByContent,
+ TransactionId *pFreezeXid, MultiXactId *pCutoffMulti);
static List *get_tables_to_cluster(MemoryContext cluster_context);
static void reform_and_rewrite_tuple(HeapTuple tuple,
TupleDesc oldTupDesc, TupleDesc newTupDesc,
@@ -176,11 +174,8 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
/* close relation, keep lock till commit */
heap_close(rel, NoLock);
- /*
- * Do the job. We use a -1 freeze_min_age to avoid having CLUSTER
- * freeze tuples earlier than a plain VACUUM would.
- */
- cluster_rel(tableOid, indexOid, false, stmt->verbose, -1, -1);
+ /* Do the job. */
+ cluster_rel(tableOid, indexOid, false, stmt->verbose);
}
else
{
@@ -229,9 +224,8 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
StartTransactionCommand();
/* functions in indexes may want a snapshot set */
PushActiveSnapshot(GetTransactionSnapshot());
- /* Do the job. As above, use a -1 freeze_min_age. */
- cluster_rel(rvtc->tableOid, rvtc->indexOid, true, stmt->verbose,
- -1, -1);
+ /* Do the job. */
+ cluster_rel(rvtc->tableOid, rvtc->indexOid, true, stmt->verbose);
PopActiveSnapshot();
CommitTransactionCommand();
}
@@ -262,8 +256,7 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
* and error messages should refer to the operation as VACUUM not CLUSTER.
*/
void
-cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose,
- int freeze_min_age, int freeze_table_age)
+cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose)
{
Relation OldHeap;
@@ -407,8 +400,7 @@ cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose,
TransferPredicateLocksToHeapRelation(OldHeap);
/* rebuild_relation does all the dirty work */
- rebuild_relation(OldHeap, indexOid, freeze_min_age, freeze_table_age,
- verbose);
+ rebuild_relation(OldHeap, indexOid, verbose);
/* NB: rebuild_relation does heap_close() on OldHeap */
}
@@ -561,8 +553,7 @@ mark_index_clustered(Relation rel, Oid indexOid, bool is_internal)
* NB: this routine closes OldHeap at the right time; caller should not.
*/
static void
-rebuild_relation(Relation OldHeap, Oid indexOid,
- int freeze_min_age, int freeze_table_age, bool verbose)
+rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
{
Oid tableOid = RelationGetRelid(OldHeap);
Oid tableSpace = OldHeap->rd_rel->reltablespace;
@@ -587,8 +578,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid,
AccessExclusiveLock);
/* Copy the heap data into the new table in the desired order */
- copy_heap_data(OIDNewHeap, tableOid, indexOid,
- freeze_min_age, freeze_table_age, verbose,
+ copy_heap_data(OIDNewHeap, tableOid, indexOid, verbose,
&swap_toast_by_content, &frozenXid, &cutoffMulti);
/*
@@ -743,8 +733,7 @@ make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp,
* *pCutoffMulti receives the MultiXactId used as a cutoff point.
*/
static void
-copy_heap_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
- int freeze_min_age, int freeze_table_age, bool verbose,
+copy_heap_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex, bool verbose,
bool *pSwapToastByContent, TransactionId *pFreezeXid,
MultiXactId *pCutoffMulti)
{
@@ -857,10 +846,11 @@ copy_heap_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
*pSwapToastByContent = false;
/*
- * compute xids used to freeze and weed out dead tuples.
+ * Compute xids used to freeze and weed out dead tuples and multixacts.
+ * Since we're going to rewrite the whole table anyway, there's no reason
+ * not to be aggressive about this.
*/
- vacuum_set_xid_limits(freeze_min_age, freeze_table_age,
- OldHeap->rd_rel->relisshared,
+ vacuum_set_xid_limits(0, 0, OldHeap->rd_rel->relisshared,
&OldestXmin, &FreezeXid, NULL, &MultiXactCutoff,
NULL);
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 1199060..ba0e841 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -1149,8 +1149,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
/* VACUUM FULL is now a variant of CLUSTER; see cluster.c */
cluster_rel(relid, InvalidOid, false,
- (vacstmt->options & VACOPT_VERBOSE) != 0,
- vacstmt->freeze_min_age, vacstmt->freeze_table_age);
+ (vacstmt->options & VACOPT_VERBOSE) != 0);
}
else
lazy_vacuum_rel(onerel, vacstmt, vac_strategy);
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
index a887318..5dfa998 100644
--- a/src/include/commands/cluster.h
+++ b/src/include/commands/cluster.h
@@ -20,7 +20,7 @@
extern void cluster(ClusterStmt *stmt, bool isTopLevel);
extern void cluster_rel(Oid tableOid, Oid indexOid, bool recheck,
- bool verbose, int freeze_min_age, int freeze_table_age);
+ bool verbose);
extern void check_index_is_clusterable(Relation OldHeap, Oid indexOid,
bool recheck, LOCKMODE lockmode);
extern void mark_index_clustered(Relation rel, Oid indexOid, bool is_internal);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
