On 2017-05-11 14:54:26 -0700, Andres Freund wrote:
> On 2017-05-11 14:51:55 -0700, wrote:
> > On 2017-05-08 00:10:12 -0700, Andres Freund wrote:
> > > I plan to commit the next pending patch after the back branch releases
> > > are cut - it's an invasive fix and the issue doesn't cause corruption
> > > "just" slow slot creation. So it seems better to wait for a few days,
> > > rather than hurry it into the release.
> >
> > Now that that's done, here's an updated version of that patch. Note the
> > new logic to trigger xl_running_xact's to be logged at the right spot.
> > Works well in my testing.
> >
> > I plan to commit this fairly soon, unless somebody wants a bit more time
> > to look into it.
> >
> > Unless somebody protests, I'd like to slightly revise how the on-disk
> > snapshots are stored on master - given the issues this bug/commit showed
> > with the current method - but I can see one could argue that that should
> > rather be done next release.
>
> As usual I forgot my attachement.
This patch also seems to offer a way to do your 0005 in, possibly, more
efficient manner. We don't ever need to assume a transaction is
timetravelling anymore. Could you check whether the attached, hasty,
patch resolves the performance problems you measured? Also, do you have
a "reference" workload for that?
Regards,
Andres
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index 0f2dcb84be..4ddd10fcf0 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -929,21 +929,31 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid,
{
int nxact;
- bool forced_timetravel = false;
+ bool needs_snapshot = false;
+ bool needs_timetravel = false;
+
bool sub_needs_timetravel = false;
- bool top_needs_timetravel = false;
TransactionId xmax = xid;
+ if (builder->state == SNAPBUILD_START)
+ return;
+
+
/*
- * If we couldn't observe every change of a transaction because it was
- * already running at the point we started to observe we have to assume it
- * made catalog changes.
- *
- * This has the positive benefit that we afterwards have enough
- * information to build an exportable snapshot that's usable by pg_dump et
- * al.
+ * Transactions preceding BUILDING_SNAPSHOT will neither be decoded, nor
+ * will it be part of a snapshot. So we don't even need to record
+ * anything.
*/
+ if (builder->state == SNAPBUILD_BUILDING_SNAPSHOT &&
+ TransactionIdPrecedes(xid, SnapBuildNextPhaseAt(builder)))
+ {
+ /* ensure that only commits after this are getting replayed */
+ if (builder->start_decoding_at <= lsn)
+ builder->start_decoding_at = lsn + 1;
+ return;
+ }
+
if (builder->state < SNAPBUILD_CONSISTENT)
{
/* ensure that only commits after this are getting replayed */
@@ -951,12 +961,13 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid,
builder->start_decoding_at = lsn + 1;
/*
- * We could avoid treating !SnapBuildTxnIsRunning transactions as
- * timetravel ones, but we want to be able to export a snapshot when
- * we reached consistency.
+ * If we're building an exportable snapshot, force recording of the
+ * xid, even if the transaction doesn't modify the catalog.
*/
- forced_timetravel = true;
- elog(DEBUG1, "forced to assume catalog changes for xid %u because it was running too early", xid);
+ if (builder->building_full_snapshot)
+ {
+ needs_timetravel = true;
+ }
}
for (nxact = 0; nxact < nsubxacts; nxact++)
@@ -964,23 +975,13 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid,
TransactionId subxid = subxacts[nxact];
/*
- * 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;
+ needs_snapshot = true;
elog(DEBUG1, "found subtransaction %u:%u with catalog changes.",
xid, subxid);
@@ -990,21 +991,26 @@ 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, even
+ * if not catalog modifying.
+ */
+ else if (needs_timetravel)
+ {
+ SnapBuildAddCommittedTxn(builder, subxid);
+ if (NormalTransactionIdFollows(subxid, xmax))
+ xmax = subxid;
+ }
}
- 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 top-level modifies catalog, it'll need a snapshot */
+ if (ReorderBufferXidHasCatalogChanges(builder->reorder, xid))
{
elog(DEBUG2, "found top level transaction %u, with catalog changes!",
xid);
-
- top_needs_timetravel = true;
+ needs_snapshot = true;
+ needs_timetravel = true;
SnapBuildAddCommittedTxn(builder, xid);
}
else if (sub_needs_timetravel)
@@ -1012,23 +1018,38 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid,
/* mark toplevel txn as timetravel as well */
SnapBuildAddCommittedTxn(builder, xid);
}
+ else if (needs_timetravel)
+ {
+ elog(DEBUG2, "forced transaction %u to do timetravel.", xid);
+
+ SnapBuildAddCommittedTxn(builder, xid);
+ }
+
+ if (!needs_timetravel)
+ {
+ /* record that we cannot export a general snapshot anymore */
+ builder->committed.includes_all_transactions = false;
+ }
+
+ Assert(!needs_snapshot || needs_timetravel);
+
+ /*
+ * Adjust xmax of the snapshot builder, we only do that for committed,
+ * catalog modifying, transactions, everything else isn't interesting
+ * for us since we'll never look at the respective rows.
+ */
+ if (needs_timetravel &&
+ (!TransactionIdIsValid(builder->xmax) ||
+ TransactionIdFollowsOrEquals(xmax, builder->xmax)))
+ {
+ builder->xmax = xmax;
+ TransactionIdAdvance(builder->xmax);
+ }
/* if there's any reason to build a historic snapshot, do so now */
- if (forced_timetravel || top_needs_timetravel || sub_needs_timetravel)
+ if (needs_snapshot)
{
/*
- * Adjust xmax of the snapshot builder, we only do that for committed,
- * catalog modifying, transactions, everything else isn't interesting
- * for us since we'll never look at the respective rows.
- */
- if (!TransactionIdIsValid(builder->xmax) ||
- TransactionIdFollowsOrEquals(xmax, builder->xmax))
- {
- builder->xmax = xmax;
- TransactionIdAdvance(builder->xmax);
- }
-
- /*
* If we haven't built a complete snapshot yet there's no need to hand
* it out, it wouldn't (and couldn't) be used anyway.
*/
@@ -1059,11 +1080,6 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid,
/* add a new Snapshot to all currently running transactions */
SnapBuildDistributeNewCatalogSnapshot(builder, lsn);
}
- else
- {
- /* record that we cannot export a general snapshot anymore */
- builder->committed.includes_all_transactions = false;
- }
}
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
