From: Petr Jelinek <petr.jelinek@2ndquadrant.com>
To: PostgreSQL-development <pgsql-hackers@postgresql.org>
Cc: Andres Freund <andres@anarazel.de>
Subject: Re: snapbuild woes
Date: Sat, 10 Dec 2016 23:49:40 +0100
Message-ID: <aee1d499-e3ca-e091-56da-1ee6a47741c8@2ndquadrant.com> (raw)
In-Reply-To: <f37e975c-908f-858e-707f-058d3b1eb214@2ndquadrant.com>
References: <f37e975c-908f-858e-707f-058d3b1eb214@2ndquadrant.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
On 10/12/16 23:10, Petr Jelinek wrote:
>
> The attached 0002-Skip-unnecessary-snapshot-builds.patch changes this
> behavior so that we don't make snapshots for transactions that we seen
> wholly and know that they didn't make catalog changes even if we didn't
> reach consistency yet.
>
Eh, attached wrong patch. This one is correct.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackersAttachments:
[application/x-patch] 0002-Skip-unnecessary-snapshot-builds.patch (5.9K, ../aee1d499-e3ca-e091-56da-1ee6a47741c8@2ndquadrant.com/2-0002-Skip-unnecessary-snapshot-builds.patch)
download | inline diff:
From 2add068ed38c2887e6652396c280695a7e384fe7 Mon Sep 17 00:00:00 2001
From: Petr Jelinek <pjmodos@pjmodos.net>
Date: Sat, 10 Dec 2016 22:22:13 +0100
Subject: [PATCH 2/2] Skip unnecessary snapshot builds
---
src/backend/replication/logical/snapbuild.c | 82 +++++++++++++++++++----------
1 file changed, 53 insertions(+), 29 deletions(-)
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.cindex 5836d52..ea3f40f 100644--- a/src/backend/replication/logical/snapbuild.c+++ b/src/backend/replication/logical/snapbuild.c@@ -955,6 +955,7 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid,
bool forced_timetravel = false;
bool sub_needs_timetravel = false;
bool top_needs_timetravel = false;
+ bool skip_forced_snapshot = false;
TransactionId xmax = xid;
@@ -976,10 +977,19 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid,
/*
* We could avoid treating !SnapBuildTxnIsRunning transactions as
* timetravel ones, but we want to be able to export a snapshot when
- * we reached consistency.+ * we reached consistency so we need to keep track of them.
*/
forced_timetravel = true;
elog(DEBUG1, "forced to assume catalog changes for xid %u because it was running too early", xid);
++ /*+ * It is however desirable to skip building new snapshot for+ * !SnapBuildTxnIsRunning transactions as otherwise we might end up+ * building thousands of unused snapshots on busy servers which can+ * be very expensive.+ */+ if (!SnapBuildTxnIsRunning(builder, xid))+ skip_forced_snapshot = true;
}
for (nxact = 0; nxact < nsubxacts; nxact++)
@@ -992,21 +1002,10 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid,
SnapBuildEndTxn(builder, lsn, subxid);
/*
- * If we're forcing timetravel we also need visibility information- * about subtransaction, so keep track of subtransaction's state.- */- if (forced_timetravel)- {- SnapBuildAddCommittedTxn(builder, subxid);- if (NormalTransactionIdFollows(subxid, xmax))- xmax = subxid;- }-- /*
* Add subtransaction to base snapshot if it DDL, we don't distinguish
* to toplevel transactions there.
*/
- else if (ReorderBufferXidHasCatalogChanges(builder->reorder, subxid))+ if (ReorderBufferXidHasCatalogChanges(builder->reorder, subxid))
{
sub_needs_timetravel = true;
@@ -1018,6 +1017,16 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid,
if (NormalTransactionIdFollows(subxid, xmax))
xmax = subxid;
}
+ /*+ * If we're forcing timetravel we also need visibility information+ * about subtransaction, so keep track of subtransaction's state.+ */+ else if (forced_timetravel)+ {+ SnapBuildAddCommittedTxn(builder, subxid);+ if (NormalTransactionIdFollows(subxid, xmax))+ xmax = subxid;+ }
}
/*
@@ -1026,14 +1035,8 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid,
*/
SnapBuildEndTxn(builder, lsn, xid);
- if (forced_timetravel)- {- elog(DEBUG2, "forced transaction %u to do timetravel.", xid);-- SnapBuildAddCommittedTxn(builder, xid);- }
/* add toplevel transaction to base snapshot */
- else if (ReorderBufferXidHasCatalogChanges(builder->reorder, xid))+ if (ReorderBufferXidHasCatalogChanges(builder->reorder, xid))
{
elog(DEBUG2, "found top level transaction %u, with catalog changes!",
xid);
@@ -1046,10 +1049,18 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid,
/* mark toplevel txn as timetravel as well */
SnapBuildAddCommittedTxn(builder, xid);
}
+ else if (forced_timetravel)+ {+ elog(DEBUG2, "forced transaction %u to do timetravel.", xid);++ SnapBuildAddCommittedTxn(builder, xid);+ }
/* if there's any reason to build a historic snapshot, do so now */
if (forced_timetravel || top_needs_timetravel || sub_needs_timetravel)
{
+ bool build_snapshot;+
/*
* Adjust xmax of the snapshot builder, we only do that for committed,
* catalog modifying, transactions, everything else isn't interesting
@@ -1070,14 +1081,29 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid,
return;
/*
- * Decrease the snapshot builder's refcount of the old snapshot, note- * that it still will be used if it has been handed out to the- * reorderbuffer earlier.+ * Build snapshot if needed. We need to build it if there isn't one+ * already built, or if the transaction has made catalog changes or+ * when we can't know if transaction made catalog changes.
*/
- if (builder->snapshot)+ build_snapshot = !builder->snapshot || top_needs_timetravel ||+ sub_needs_timetravel || !skip_forced_snapshot;++ /*+ * Decrease the snapshot builder's refcount of the old snapshot if we+ * plan to build new one, note that it still will be used if it has+ * been handed out to the reorderbuffer earlier.+ */+ if (builder->snapshot && build_snapshot)
SnapBuildSnapDecRefcount(builder->snapshot);
- builder->snapshot = SnapBuildBuildSnapshot(builder, xid);+ /* Build new snapshot unless asked not to. */+ if (build_snapshot)+ {+ builder->snapshot = SnapBuildBuildSnapshot(builder, xid);++ /* refcount of the snapshot builder for the new snapshot */+ SnapBuildSnapIncRefcount(builder->snapshot);+ }
/* we might need to execute invalidations, add snapshot */
if (!ReorderBufferXidHasBaseSnapshot(builder->reorder, xid))
@@ -1087,11 +1113,9 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid,
builder->snapshot);
}
- /* refcount of the snapshot builder for the new snapshot */- SnapBuildSnapIncRefcount(builder->snapshot);-
/* add a new Snapshot to all currently running transactions */
- SnapBuildDistributeNewCatalogSnapshot(builder, lsn);+ if (build_snapshot)+ SnapBuildDistributeNewCatalogSnapshot(builder, lsn);
}
else
{
--
2.7.4
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: petr.jelinek@2ndquadrant.com, andres@anarazel.de
Subject: Re: snapbuild woes
In-Reply-To: <aee1d499-e3ca-e091-56da-1ee6a47741c8@2ndquadrant.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox