On 2014-02-24 17:06:53 -0500, Robert Haas wrote:
> - heap_page_prune_opt(scan->rs_rd, buffer, RecentGlobalXmin);
> + if (IsSystemRelation(scan->rs_rd)
> + || RelationIsAccessibleInLogicalDecoding(scan->rs_rd))
> + heap_page_prune_opt(scan->rs_rd, buffer, RecentGlobalXmin);
> + else
> + heap_page_prune_opt(scan->rs_rd, buffer,
> RecentGlobalDataXmin);
>
> Instead of changing the callers of heap_page_prune_opt() in this way,
> I think it might be better to change heap_page_prune_opt() to take
> only the first two of its current three parameters; everybody's just
> passing RecentGlobalXmin right now anyway.
I've changed stuff this way, and it indeed looks better.
I am wondering about the related situation of GetOldestXmin()
callers. There's a fair bit of duplicated logic in the callers, before
but especially after this patchset. What about adding 'Relation rel'
parameter instead of `allDbs' and `systable'? That keeps the logic
centralized and there's been a fair amount of talk about vacuum
optimizations that could also use it.
It's a bit sad that that requires including rel.h from procarray.h...
What do you think? Isolated patch attached.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
>From b23fe717c699ad5e7cac217c3a5725b57c722ff2 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Wed, 26 Feb 2014 18:23:55 +0100
Subject: [PATCH] fixup! wal_decoding: Introduce logical changeset extraction.
Centralize knowledge from GetOldestXmin() callers into GetOldestXmin()
itself.
---
src/backend/access/transam/xlog.c | 4 +--
src/backend/catalog/index.c | 12 +--------
src/backend/commands/analyze.c | 4 +--
src/backend/commands/cluster.c | 4 +--
src/backend/commands/vacuum.c | 9 +++----
src/backend/commands/vacuumlazy.c | 6 ++---
src/backend/replication/walreceiver.c | 2 +-
src/backend/storage/ipc/procarray.c | 50 +++++++++++++++++++++++------------
src/include/commands/vacuum.h | 4 +--
src/include/storage/procarray.h | 3 ++-
10 files changed, 49 insertions(+), 49 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index b6b2cd4..9f48fa5 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8619,7 +8619,7 @@ CreateCheckPoint(int flags)
* StartupSUBTRANS hasn't been called yet.
*/
if (!RecoveryInProgress())
- TruncateSUBTRANS(GetOldestXmin(true, false, true));
+ TruncateSUBTRANS(GetOldestXmin(NULL, false));
/* Real work is done, but log and update stats before releasing lock. */
LogCheckpointEnd(false);
@@ -8997,7 +8997,7 @@ CreateRestartPoint(int flags)
* this because StartupSUBTRANS hasn't been called yet.
*/
if (EnableHotStandby)
- TruncateSUBTRANS(GetOldestXmin(true, false, true));
+ TruncateSUBTRANS(GetOldestXmin(NULL, false));
/* Real work is done, but log and update before releasing lock. */
LogCheckpointEnd(true);
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 8c1803e..877d767 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -2154,19 +2154,9 @@ IndexBuildHeapScan(Relation heapRelation,
}
else
{
- /*
- * We can ignore a) pegged xmins b) shared relations if we don't scan
- * something acting as a catalog.
- */
- bool include_systables =
- IsCatalogRelation(heapRelation) ||
- RelationIsAccessibleInLogicalDecoding(heapRelation);
-
snapshot = SnapshotAny;
/* okay to ignore lazy VACUUMs here */
- OldestXmin = GetOldestXmin(heapRelation->rd_rel->relisshared,
- true,
- include_systables);
+ OldestXmin = GetOldestXmin(heapRelation, true);
}
scan = heap_beginscan_strat(heapRelation, /* relation */
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index fe569f5..a04adea 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -1082,9 +1082,7 @@ acquire_sample_rows(Relation onerel, int elevel,
totalblocks = RelationGetNumberOfBlocks(onerel);
/* Need a cutoff xmin for HeapTupleSatisfiesVacuum */
- OldestXmin = GetOldestXmin(onerel->rd_rel->relisshared, true,
- IsCatalogRelation(onerel) ||
- RelationIsAccessibleInLogicalDecoding(onerel));
+ OldestXmin = GetOldestXmin(onerel, true);
/* Prepare for sampling block numbers */
BlockSampler_Init(&bs, totalblocks, targrows);
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index fc5c013..b6b40e7 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -850,9 +850,7 @@ copy_heap_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex, bool verbose,
* Since we're going to rewrite the whole table anyway, there's no reason
* not to be aggressive about this.
*/
- vacuum_set_xid_limits(0, 0, 0, 0, OldHeap->rd_rel->relisshared,
- IsCatalogRelation(OldHeap)
- || RelationIsAccessibleInLogicalDecoding(OldHeap),
+ vacuum_set_xid_limits(OldHeap, 0, 0, 0, 0,
&OldestXmin, &FreezeXid, NULL, &MultiXactCutoff,
NULL);
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index ceeac77..ded1841 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -398,12 +398,11 @@ get_rel_oids(Oid relid, const RangeVar *vacrel)
* not interested.
*/
void
-vacuum_set_xid_limits(int freeze_min_age,
+vacuum_set_xid_limits(Relation rel,
+ int freeze_min_age,
int freeze_table_age,
int multixact_freeze_min_age,
int multixact_freeze_table_age,
- bool sharedRel,
- bool catalogRel,
TransactionId *oldestXmin,
TransactionId *freezeLimit,
TransactionId *xidFullScanLimit,
@@ -426,7 +425,7 @@ vacuum_set_xid_limits(int freeze_min_age,
* working on a particular table at any time, and that each vacuum is
* always an independent transaction.
*/
- *oldestXmin = GetOldestXmin(sharedRel, true, catalogRel);
+ *oldestXmin = GetOldestXmin(rel, true);
Assert(TransactionIdIsNormal(*oldestXmin));
@@ -796,7 +795,7 @@ vac_update_datfrozenxid(void)
* committed pg_class entries for new tables; see AddNewRelationTuple().
* So we cannot produce a wrong minimum by starting with this.
*/
- newFrozenXid = GetOldestXmin(true, true, true);
+ newFrozenXid = GetOldestXmin(NULL, true);
/*
* Similarly, initialize the MultiXact "min" with the value that would be
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index bffe153..d5db917 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -205,12 +205,10 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
vac_strategy = bstrategy;
- vacuum_set_xid_limits(vacstmt->freeze_min_age, vacstmt->freeze_table_age,
+ vacuum_set_xid_limits(onerel,
+ vacstmt->freeze_min_age, vacstmt->freeze_table_age,
vacstmt->multixact_freeze_min_age,
vacstmt->multixact_freeze_table_age,
- onerel->rd_rel->relisshared,
- IsCatalogRelation(onerel)
- || RelationIsAccessibleInLogicalDecoding(onerel),
&OldestXmin, &FreezeLimit, &xidFullScanLimit,
&MultiXactCutoff, &mxactFullScanLimit);
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index 3d67333..43db108 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -1147,7 +1147,7 @@ XLogWalRcvSendHSFeedback(bool immed)
* everything else has been checked.
*/
if (hot_standby_feedback)
- xmin = GetOldestXmin(true, false, true);
+ xmin = GetOldestXmin(NULL, false);
else
xmin = InvalidTransactionId;
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 101d47f..060d5f0 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -50,6 +50,7 @@
#include "access/transam.h"
#include "access/xact.h"
#include "access/twophase.h"
+#include "catalog/catalog.h"
#include "miscadmin.h"
#include "storage/proc.h"
#include "storage/procarray.h"
@@ -1110,21 +1111,23 @@ TransactionIdIsActive(TransactionId xid)
* GetOldestXmin -- returns oldest transaction that was running
* when any current transaction was started.
*
- * If allDbs is TRUE then all backends are considered; if allDbs is FALSE
- * then only backends running in my own database are considered.
+ * If rel is NULL or a shared relation, all backends are considered, otherwise
+ * only backends running in this database are considered.
*
* If ignoreVacuum is TRUE then backends with the PROC_IN_VACUUM flag set are
* ignored.
*
- * This is used by VACUUM to decide which deleted tuples must be preserved
- * in a table. allDbs = TRUE is needed for shared relations, but allDbs =
- * FALSE is sufficient for non-shared relations, since only backends in my
- * own database could ever see the tuples in them. Also, we can ignore
- * concurrently running lazy VACUUMs because (a) they must be working on other
- * tables, and (b) they don't need to do snapshot-based lookups.
+ * This is used by VACUUM to decide which deleted tuples must be preserved in
+ * the passed in table. For shared relations backends in all databases must be
+ * considered, but for non-shared non-shared relations that's not required,
+ * since only backends in my own database could ever see the tuples in
+ * them. Also, we can ignore concurrently running lazy VACUUMs because (a)
+ * they must be working on other tables, and (b) they don't need to do
+ * snapshot-based lookups.
*
- * This is also used to determine where to truncate pg_subtrans. allDbs
- * must be TRUE for that case, and ignoreVacuum FALSE.
+ * This is also used to determine where to truncate pg_subtrans. For that
+ * backends in all databases have to be considered, so rel = NULL has to be
+ * passed in.
*
* Note: we include all currently running xids in the set of considered xids.
* This ensures that if a just-started xact has not yet set its snapshot,
@@ -1135,7 +1138,7 @@ TransactionIdIsActive(TransactionId xid)
* backwards on repeated calls. The calculated value is conservative, so that
* anything older is definitely not considered as running by anyone anymore,
* but the exact value calculated depends on a number of things. For example,
- * if allDbs is FALSE and there are no transactions running in the current
+ * if rel = NULL and there are no transactions running in the current
* database, GetOldestXmin() returns latestCompletedXid. If a transaction
* begins after that, its xmin will include in-progress transactions in other
* databases that started earlier, so another call will return a lower value.
@@ -1154,14 +1157,23 @@ TransactionIdIsActive(TransactionId xid)
* GetOldestXmin() move backwards, with no consequences for data integrity.
*/
TransactionId
-GetOldestXmin(bool allDbs, bool ignoreVacuum, bool systable)
+GetOldestXmin(Relation rel, bool ignoreVacuum)
{
ProcArrayStruct *arrayP = procArray;
TransactionId result;
int index;
+ bool allDbs;
+
volatile TransactionId replication_slot_xmin = InvalidTransactionId;
volatile TransactionId replication_slot_catalog_xmin = InvalidTransactionId;
+ /*
+ * If we're not computing a relation specific limit, or if a shared
+ * relation has been passed in, backends in all databases have to be
+ * considered.
+ */
+ allDbs = rel == NULL || rel->rd_rel->relisshared;
+
/* Cannot look for individual databases during recovery */
Assert(allDbs || !RecoveryInProgress());
@@ -1184,8 +1196,8 @@ GetOldestXmin(bool allDbs, bool ignoreVacuum, bool systable)
volatile PGXACT *pgxact = &allPgXact[pgprocno];
/*
- * Backend is doing logical decoding which manages xmin
- * separately, check below.
+ * Backend is doing logical decoding which manages xmin separately,
+ * check below.
*/
if (pgxact->vacuumFlags & PROC_IN_LOGICAL_DECODING)
continue;
@@ -1271,10 +1283,14 @@ GetOldestXmin(bool allDbs, bool ignoreVacuum, bool systable)
result = replication_slot_xmin;
/*
- * after locks are released and defer_cleanup_age has been applied, check
- * whether we need to back up further to make logical decoding possible.
+ * After locks have been released and defer_cleanup_age has been applied,
+ * check whether we need to back up further to make logical decoding
+ * possible. We need to do so if we're computing the global limit (rel =
+ * NULL) or if the passed relation is a catalog relation of some kind.
*/
- if (systable && TransactionIdIsValid(replication_slot_catalog_xmin) &&
+ if ((rel == NULL ||
+ RelationIsAccessibleInLogicalDecoding(rel)) &&
+ TransactionIdIsValid(replication_slot_catalog_xmin) &&
NormalTransactionIdPrecedes(replication_slot_catalog_xmin, result))
result = replication_slot_catalog_xmin;
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 311098a..058dc5f 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -157,10 +157,10 @@ extern void vac_update_relstats(Relation relation,
bool hasindex,
TransactionId frozenxid,
MultiXactId minmulti);
-extern void vacuum_set_xid_limits(int freeze_min_age, int freeze_table_age,
+extern void vacuum_set_xid_limits(Relation rel,
+ int freeze_min_age, int freeze_table_age,
int multixact_freeze_min_age,
int multixact_freeze_table_age,
- bool sharedRel, bool catalogRel,
TransactionId *oldestXmin,
TransactionId *freezeLimit,
TransactionId *xidFullScanLimit,
diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h
index 284d746..c43fd81 100644
--- a/src/include/storage/procarray.h
+++ b/src/include/storage/procarray.h
@@ -15,6 +15,7 @@
#define PROCARRAY_H
#include "storage/standby.h"
+#include "utils/rel.h"
#include "utils/snapshot.h"
@@ -50,7 +51,7 @@ extern RunningTransactions GetRunningTransactionData(void);
extern bool TransactionIdIsInProgress(TransactionId xid);
extern bool TransactionIdIsActive(TransactionId xid);
-extern TransactionId GetOldestXmin(bool allDbs, bool ignoreVacuum, bool systable);
+extern TransactionId GetOldestXmin(Relation rel, bool ignoreVacuum);
extern TransactionId GetOldestActiveTransactionId(void);
extern TransactionId GetOldestSafeDecodingTransactionId(void);
--
1.8.5.rc2.dirty
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
