On 23 March 2017 at 01:41, Robert Haas <robertmh...@gmail.com> wrote:
> On Wed, Mar 22, 2017 at 3:13 AM, Simon Riggs <si...@2ndquadrant.com> wrote:
>>> Changes made per discussion.
>>
>> This looks good to me also. Does what we need it to do.
>>
>> I was a little worried by possible performance of new lock, but its
>> not intended to be run frequently, so its OK.
>
> Agreed.
>
> Reviewing 0002:
>
> + if (!TransactionIdIsValid(xid))
> + {
> + LWLockRelease(XidGenLock);
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("transaction ID " UINT64_FORMAT " is an invalid xid",
> + xid_with_epoch)));
> + }
>
> It's unnecessary to release LWLockRelease() before throwing an error,
> and it's also wrong because we haven't acquired XidGenLock in this
> code path. But maybe it would be better to just remove this entirely
> and instead have TransactionIdInRecentPast return false for
> InvalidTransactionId. Then we'd avoid adding a translatable message
> for a case that is basically harmless to allow.
Agreed, that's better.
> + if (TransactionIdIsCurrentTransactionId(xid))
> + status = gettext_noop("in progress");
> + else if (TransactionIdDidCommit(xid))
> + status = gettext_noop("committed");
> + else if (TransactionIdDidAbort(xid))
> + status = gettext_noop("aborted");
> + else
> +
> + /*
> + * can't test TransactionIdIsInProgress here or we race with
> + * concurrent commit/abort. There's no point anyway, since it
> + * might then commit/abort just after we check.
> + */
> + status = gettext_noop("in progress");
>
> I am not sure this is going to do the right thing for transactions
> that are aborted by a crash without managing to write an abort record.
You're right. It'll report in-progress, since the clog entry will
still be 0. We don't appear to explicitly update the clog during
recovery.
Funny, I got so focused on the clog access safety stuff in the end
that I missed the bigger picture.
Given that part of the point of this is xact status after crash,
that's kind of important. I've added a TAP test for this, as part of a
new test set, "011_crash_recovery.pl". The recovery tests don't really
have much on crash behaviour at the moment so might as well start it
and add more as we go. It doesn't make sense to add a whole new top
level TAP test just for txid_status.
(I *love* the TAP framework now, it took 10 mins to write a test
that's trivial to repeat in 5 seconds per run for this).
> It seems that the first check will say the transaction isn't in
> progress, and the second and third checks will say it isn't either
> committed or aborted since, if I am reading this correctly, they just
> read clog directly.
Right. They're not concerned with subxacts or multixacts.
> Compare HeapTupleSatisfiesMVCC, which assumes
> that a not-in-progress, not-committed transaction must necessarily
> have aborted.
Right.
We don't have a HeapTuple here, of course, so we can't test flags.
Most importantly this means we can't handle multixacts. If we ever did
decide to expose multixacts as bigint epoch-qualified xids for general
users, we'd probably reserve the high bit of the epoch the bigint xid
representation for the equivalent of HEAP_XMAX_IS_MULTI, and maybe
it's worth reserving that bit now and ERROR'ing if we find it set. But
right now we never produce a bigint xid with a multixact.
I wonder if a note in the docs warning not to cast xid to bigint is
worth it. Probably not. You have to cast xid::text::bigint which is
kind of a hint, plus it doesn't make sense to test txid_status() for
tuples' xmin/xmax . I guess you might use it with xids from
pg_stat_activity, but there's not much point when you can just look at
pg_stat_activity again to see if the proc of interest is still there
and has the same xid.
Anyway...
> I think your comment is pointing to a real issue,
> though. It seems like what might be needed is to add one more check.
> Before where you have the "else" clause, check if the XID is old, e.g.
> precedes our snapshot's xmin. If so, it must be committed or aborted
> and, since it didn't commit, it aborted. If not, it must've changed
> from in progress to not-in-progress just as we were in the midst
> checking, so labeling it as in progress is fine.
That seems clear and sensible.
XidInMVCCSnapshot simply tests TransactionIdPrecedes(xid,
snapshot->xmin), as does HeapTupleSatisfiesHistoricMVCC . It seems
reasonable enough to just examine the snapshot xmin directly in
txid_status too; it's already done in replication/logical/snapbuild.c
and lmgr/predicate.c.
We're running in an SQL-called function so we'll have a good snapshot
and GetSnapshotData will have run, so GetActiveSnapshot()->xmin should
be sufficient.
Amended patch attached, with added TAP test included.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
From 25ee92d26ad50a3e62104de37ccdb2bbb670f3fe Mon Sep 17 00:00:00 2001
From: Craig Ringer <cr...@2ndquadrant.com>
Date: Mon, 23 Jan 2017 13:34:02 +0800
Subject: [PATCH] Introduce txid_status(bigint) to get status of an xact
If an application loses its connection while a COMMIT request is in
flight, the backend crashes mid-commit, etc, then an application may
not be sure whether or not a commit completed successfully or was
rolled back. While two-phase commit solves this it does so at a
considerable overhead, so introduce a lighter alternative.
txid_status(bigint) lets an application determine the status of a a
commit based on an xid-with-epoch as returned by txid_current() or
similar. Status may be committed, aborted, in-progress (including
prepared xacts) or null if the xact is too old for its commit status
to still be retained because it has passed the wrap-around epoch
boundary.
Applications must call txid_current() in their transactions to make
much use of this since PostgreSQL does not automatically report an xid
to the client when one is assigned.
Introduces TransactionIdInRecentPast(...) for the use of other
functions that need similar logic in future.
Authors: Craig Ringer, Robert Haas
---
doc/src/sgml/func.sgml | 27 ++++++++
src/backend/utils/adt/txid.c | 132 +++++++++++++++++++++++++++++++++++++
src/include/catalog/pg_proc.h | 2 +
src/test/regress/expected/txid.out | 68 +++++++++++++++++++
src/test/regress/sql/txid.sql | 38 +++++++++++
5 files changed, 267 insertions(+)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 9408a25..c8639b8 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -17523,6 +17523,10 @@ SELECT collation for ('foo' COLLATE "de_DE");
<primary>txid_visible_in_snapshot</primary>
</indexterm>
+ <indexterm>
+ <primary>txid_status</primary>
+ </indexterm>
+
<para>
The functions shown in <xref linkend="functions-txid-snapshot">
provide server transaction information in an exportable form. The main
@@ -17573,6 +17577,11 @@ SELECT collation for ('foo' COLLATE "de_DE");
<entry><type>boolean</type></entry>
<entry>is transaction ID visible in snapshot? (do not use with subtransaction ids)</entry>
</row>
+ <row>
+ <entry><literal><function>txid_status(<parameter>bigint</parameter>)</function></literal></entry>
+ <entry><type>txid_status</type></entry>
+ <entry>report the status of the given xact - <literal>committed</literal>, <literal>aborted</literal>, <literal>in progress</literal>, or NULL if the xid is too old</entry>
+ </row>
</tbody>
</tgroup>
</table>
@@ -17643,6 +17652,24 @@ SELECT collation for ('foo' COLLATE "de_DE");
</para>
<para>
+ <function>txid_status(bigint)</> reports the commit status of a recent
+ transaction. Applications may use it to determine whether a transaction
+ committed or aborted when the application and database server become
+ disconnected while a <literal>COMMIT</literal> is in progress.
+ The status of a transaction will be reported as either
+ <literal>in progress</>,
+ <literal>committed</>, or <literal>aborted</>, provided that the
+ transaction is recent enough that the system retains the commit status
+ of that transaction. If is old enough that no references to that
+ transaction survive in the system and the commit status information has
+ been discarded, this function will return NULL. Note that prepared
+ transactions are reported as <literal>in progress</>; applications must
+ check <link
+ linkend="view-pg-prepared-xacts"><literal>pg_prepared_xacts</></> if they
+ need to determine whether the xid is a prepared transaction.
+ </para>
+
+ <para>
The functions shown in <xref linkend="functions-commit-timestamp">
provide information about transactions that have been already committed.
These functions mainly provide information about when the transactions
diff --git a/src/backend/utils/adt/txid.c b/src/backend/utils/adt/txid.c
index 772d7c7..5c64e32 100644
--- a/src/backend/utils/adt/txid.c
+++ b/src/backend/utils/adt/txid.c
@@ -21,6 +21,7 @@
#include "postgres.h"
+#include "access/clog.h"
#include "access/transam.h"
#include "access/xact.h"
#include "access/xlog.h"
@@ -28,6 +29,7 @@
#include "miscadmin.h"
#include "libpq/pqformat.h"
#include "postmaster/postmaster.h"
+#include "storage/lwlock.h"
#include "utils/builtins.h"
#include "utils/memutils.h"
#include "utils/snapmgr.h"
@@ -93,6 +95,70 @@ load_xid_epoch(TxidEpoch *state)
}
/*
+ * Helper to get a TransactionId from a 64-bit xid with wraparound detection.
+ *
+ * It is an ERROR if the xid is in the future. Otherwise, returns true if
+ * the transaction is still new enough that we can determine whether it
+ * committed and false otherwise. If *extracted_xid is not NULL, it is set
+ * to the low 32 bits of the transaction ID (i.e. the actual XID, without the
+ * epoch).
+ *
+ * The caller must hold CLogTruncationLock since it's dealing with arbitrary
+ * XIDs, and must continue to hold it until it's done with any clog lookups
+ * relating to those XIDs.
+ */
+static bool
+TransactionIdInRecentPast(uint64 xid_with_epoch, TransactionId *extracted_xid)
+{
+ uint32 xid_epoch = (uint32) (xid_with_epoch >> 32);
+ TransactionId xid = (TransactionId) xid_with_epoch;
+ uint32 now_epoch;
+ TransactionId now_epoch_last_xid;
+
+ GetNextXidAndEpoch(&now_epoch_last_xid, &now_epoch);
+
+ if (extracted_xid != NULL)
+ *extracted_xid = xid;
+
+ if (!TransactionIdIsValid(xid))
+ return false;
+
+ /* For non-normal transaction IDs, we can ignore the epoch. */
+ if (!TransactionIdIsNormal(xid))
+ return true;
+
+ /* If the transaction ID is in the future, throw an error. */
+ if (xid_epoch > now_epoch
+ || (xid_epoch == now_epoch && xid > now_epoch_last_xid))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("transaction ID " UINT64_FORMAT " is in the future",
+ xid_with_epoch)));
+
+ /*
+ * ShmemVariableCache->oldestClogXid is protected by CLogTruncationLock,
+ * but we don't acquire that lock here. Instead, we require the caller to
+ * acquire it, because the caller is presumably going to look up the
+ * returned XID. If we took and released the lock within this function, a
+ * CLOG truncation could occur before the caller finished with the XID.
+ */
+ Assert(LWLockHeldByMe(CLogTruncationLock));
+
+ /*
+ * If the transaction ID has wrapped around, it's definitely too old to
+ * determine the commit status. Otherwise, we can compare it to
+ * ShmemVariableCache->oldestClogXid to determine whether the relevant CLOG
+ * entry is guaranteed to still exist.
+ */
+ if (xid_epoch + 1 < now_epoch
+ || (xid_epoch + 1 == now_epoch && xid < now_epoch_last_xid)
+ || TransactionIdPrecedes(xid, ShmemVariableCache->oldestClogXid))
+ return false;
+
+ return true;
+}
+
+/*
* do a TransactionId -> txid conversion for an XID near the given epoch
*/
static txid
@@ -354,6 +420,9 @@ bad_format:
*
* Return the current toplevel transaction ID as TXID
* If the current transaction does not have one, one is assigned.
+ *
+ * This value has the epoch as the high 32 bits and the 32-bit xid
+ * as the low 32 bits.
*/
Datum
txid_current(PG_FUNCTION_ARGS)
@@ -658,3 +727,66 @@ txid_snapshot_xip(PG_FUNCTION_ARGS)
SRF_RETURN_DONE(fctx);
}
}
+
+/*
+ * Report the status of a recent transaction ID, or null for wrapped,
+ * truncated away or otherwise too old XIDs.
+ *
+ * The passed epoch-qualified xid is treated as a normal xid, not a
+ * multixact id.
+ *
+ * If it points to a committed subxact the result is the subxact status even
+ * though the parent xact may still be in progress or may have aborted.
+ */
+Datum
+txid_status(PG_FUNCTION_ARGS)
+{
+ const char *status;
+ uint64 xid_with_epoch = PG_GETARG_INT64(0);
+ TransactionId xid;
+
+ /*
+ * We must protect against concurrent truncation of clog entries to avoid
+ * an I/O error on SLRU lookup.
+ */
+ LWLockAcquire(CLogTruncationLock, LW_SHARED);
+ if (TransactionIdInRecentPast(xid_with_epoch, &xid))
+ {
+ Assert(TransactionIdIsValid(xid));
+
+ if (TransactionIdIsCurrentTransactionId(xid))
+ status = gettext_noop("in progress");
+ else if (TransactionIdDidCommit(xid))
+ status = gettext_noop("committed");
+ else if (TransactionIdDidAbort(xid))
+ status = gettext_noop("aborted");
+ else
+ {
+ /*
+ * The xact is not marked as either committed or aborted in clog.
+ *
+ * It could be a transaction that ended without updating clog or
+ * writing an abort record due to a crash. We can safely assume
+ * it's aborted if it isn't committed and is older than our
+ * snapshot xmin.
+ *
+ * Otherwise it must be in-progress (or have been at the time
+ * we checked commit/abort status).
+ */
+ if (TransactionIdPrecedes(xid, GetActiveSnapshot()->xmin))
+ status = gettext_noop("aborted");
+ else
+ status = gettext_noop("in progress");
+ }
+ }
+ else
+ {
+ status = NULL;
+ }
+ LWLockRelease(CLogTruncationLock);
+
+ if (status == NULL)
+ PG_RETURN_NULL();
+ else
+ PG_RETURN_TEXT_P(cstring_to_text(status));
+}
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 836d6ff..39bd295 100644
--- a/src/include/catalog/pg_proc.h
+++ b/src/include/catalog/pg_proc.h
@@ -4975,6 +4975,8 @@ DATA(insert OID = 2947 ( txid_snapshot_xip PGNSP PGUID 12 1 50 0 0 f f f f t
DESCR("get set of in-progress txids in snapshot");
DATA(insert OID = 2948 ( txid_visible_in_snapshot PGNSP PGUID 12 1 0 0 0 f f f f t f i s 2 0 16 "20 2970" _null_ _null_ _null_ _null_ _null_ txid_visible_in_snapshot _null_ _null_ _null_ ));
DESCR("is txid visible in snapshot?");
+DATA(insert OID = 3360 ( txid_status PGNSP PGUID 12 1 0 0 0 f f f f t f v s 1 0 25 "20" _null_ _null_ _null_ _null_ _null_ txid_status _null_ _null_ _null_ ));
+DESCR("commit status of transaction");
/* record comparison using normal comparison rules */
DATA(insert OID = 2981 ( record_eq PGNSP PGUID 12 1 0 0 0 f f f f t f i s 2 0 16 "2249 2249" _null_ _null_ _null_ _null_ _null_ record_eq _null_ _null_ _null_ ));
diff --git a/src/test/regress/expected/txid.out b/src/test/regress/expected/txid.out
index 802ccb9..015dae3 100644
--- a/src/test/regress/expected/txid.out
+++ b/src/test/regress/expected/txid.out
@@ -254,3 +254,71 @@ SELECT txid_current_if_assigned() IS NOT DISTINCT FROM BIGINT :'txid_current';
(1 row)
COMMIT;
+-- test xid status functions
+BEGIN;
+SELECT txid_current() AS committed \gset
+COMMIT;
+BEGIN;
+SELECT txid_current() AS rolledback \gset
+ROLLBACK;
+BEGIN;
+SELECT txid_current() AS inprogress \gset
+SELECT txid_status(:committed) AS committed;
+ committed
+-----------
+ committed
+(1 row)
+
+SELECT txid_status(:rolledback) AS rolledback;
+ rolledback
+------------
+ aborted
+(1 row)
+
+SELECT txid_status(:inprogress) AS inprogress;
+ inprogress
+-------------
+ in progress
+(1 row)
+
+SELECT txid_status(1); -- BootstrapTransactionId is always committed
+ txid_status
+-------------
+ committed
+(1 row)
+
+SELECT txid_status(2); -- FrozenTransactionId is always committed
+ txid_status
+-------------
+ committed
+(1 row)
+
+SELECT txid_status(3); -- in regress testing FirstNormalTransactionId will always be behind oldestXmin
+ txid_status
+-------------
+
+(1 row)
+
+COMMIT;
+BEGIN;
+CREATE FUNCTION test_future_xid_status(bigint)
+RETURNS void
+LANGUAGE plpgsql
+AS
+$$
+BEGIN
+ PERFORM txid_status($1);
+ RAISE EXCEPTION 'didn''t ERROR at xid in the future as expected';
+EXCEPTION
+ WHEN invalid_parameter_value THEN
+ RAISE NOTICE 'Got expected error for xid in the future';
+END;
+$$;
+SELECT test_future_xid_status(:inprogress + 10000);
+NOTICE: Got expected error for xid in the future
+ test_future_xid_status
+------------------------
+
+(1 row)
+
+ROLLBACK;
diff --git a/src/test/regress/sql/txid.sql b/src/test/regress/sql/txid.sql
index 4aefd9e..bd6decf 100644
--- a/src/test/regress/sql/txid.sql
+++ b/src/test/regress/sql/txid.sql
@@ -59,3 +59,41 @@ SELECT txid_current_if_assigned() IS NULL;
SELECT txid_current() \gset
SELECT txid_current_if_assigned() IS NOT DISTINCT FROM BIGINT :'txid_current';
COMMIT;
+
+-- test xid status functions
+BEGIN;
+SELECT txid_current() AS committed \gset
+COMMIT;
+
+BEGIN;
+SELECT txid_current() AS rolledback \gset
+ROLLBACK;
+
+BEGIN;
+SELECT txid_current() AS inprogress \gset
+
+SELECT txid_status(:committed) AS committed;
+SELECT txid_status(:rolledback) AS rolledback;
+SELECT txid_status(:inprogress) AS inprogress;
+SELECT txid_status(1); -- BootstrapTransactionId is always committed
+SELECT txid_status(2); -- FrozenTransactionId is always committed
+SELECT txid_status(3); -- in regress testing FirstNormalTransactionId will always be behind oldestXmin
+
+COMMIT;
+
+BEGIN;
+CREATE FUNCTION test_future_xid_status(bigint)
+RETURNS void
+LANGUAGE plpgsql
+AS
+$$
+BEGIN
+ PERFORM txid_status($1);
+ RAISE EXCEPTION 'didn''t ERROR at xid in the future as expected';
+EXCEPTION
+ WHEN invalid_parameter_value THEN
+ RAISE NOTICE 'Got expected error for xid in the future';
+END;
+$$;
+SELECT test_future_xid_status(:inprogress + 10000);
+ROLLBACK;
--
2.5.5
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
