age(xid) on hot standby
The check_postgres txn_wraparound action[0]http://bucardo.org/check_postgres/check_postgres.pl.html#txn_wraparound runs this query:
SELECT datname, age(datfrozenxid) AS age FROM pg_database WHERE datallowconn ORDER BY 1, 2
On a hot standby, this fails with:
ERROR: cannot assign TransactionIds during recovery
So, a couple of things to wonder about:
Is it unreasonable to check for transaction ID wraparound on a standby?
It should mirror the situation on the primary, shouldn't it?
Should the age(xid) function do something more useful on a standby,
e.g., have a custom error message or return null or use the transaction
ID from the master?
The error message is coded as an elog() call, meaning that users
shouldn't see it, but it can evidently be triggered by a user, so maybe
we should decorate it with some detail, depending on the outcome of the
previous question.
(It looks like age(xid) isn't documented at all. Maybe it should be.)
[0]: http://bucardo.org/check_postgres/check_postgres.pl.html#txn_wraparound
Excerpts from Peter Eisentraut's message of mié dic 28 15:04:09 -0300 2011:
The check_postgres txn_wraparound action[0] runs this query:
SELECT datname, age(datfrozenxid) AS age FROM pg_database WHERE datallowconn ORDER BY 1, 2
On a hot standby, this fails with:
ERROR: cannot assign TransactionIds during recovery
So, a couple of things to wonder about:
Is it unreasonable to check for transaction ID wraparound on a standby?
It should mirror the situation on the primary, shouldn't it?Should the age(xid) function do something more useful on a standby,
e.g., have a custom error message or return null or use the transaction
ID from the master?
I think we could just have the xid_age call
GetCurrentTransactionIdIfAny, and if that returns InvalidXid, use
ReadNewTransactionId instead. That xid_age assigns a transaction seems
more of an accident than really intended.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
Excerpts from Peter Eisentraut's message of mié dic 28 15:04:09 -0300 2011:
On a hot standby, this fails with:
ERROR: cannot assign TransactionIds during recovery
I think we could just have the xid_age call
GetCurrentTransactionIdIfAny, and if that returns InvalidXid, use
ReadNewTransactionId instead. That xid_age assigns a transaction seems
more of an accident than really intended.
The trouble with using ReadNewTransactionId is that it makes the results
volatile, not stable as the function is declared to be.
regards, tom lane
On ons, 2011-12-28 at 14:35 -0500, Tom Lane wrote:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Excerpts from Peter Eisentraut's message of mié dic 28 15:04:09 -0300 2011:
On a hot standby, this fails with:
ERROR: cannot assign TransactionIds during recoveryI think we could just have the xid_age call
GetCurrentTransactionIdIfAny, and if that returns InvalidXid, use
ReadNewTransactionId instead. That xid_age assigns a transaction seems
more of an accident than really intended.The trouble with using ReadNewTransactionId is that it makes the results
volatile, not stable as the function is declared to be.
Could we alleviate that problem with some caching within the function?
Excerpts from Peter Eisentraut's message of dom ene 15 10:00:03 -0300 2012:
On ons, 2011-12-28 at 14:35 -0500, Tom Lane wrote:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Excerpts from Peter Eisentraut's message of mié dic 28 15:04:09 -0300 2011:
On a hot standby, this fails with:
ERROR: cannot assign TransactionIds during recoveryI think we could just have the xid_age call
GetCurrentTransactionIdIfAny, and if that returns InvalidXid, use
ReadNewTransactionId instead. That xid_age assigns a transaction seems
more of an accident than really intended.The trouble with using ReadNewTransactionId is that it makes the results
volatile, not stable as the function is declared to be.Could we alleviate that problem with some caching within the function?
Maybe if we have it be invalidated at transaction end, that could work.
So each new transaction would get a fresh value. If you had a long
running transaction the cached value would get behind, but maybe this is
not a problem or we could design some protection against it.
For the check_postgres case I imagine it opens a new connection on each
round so this would not be a problem.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Alvaro Herrera <alvherre@commandprompt.com> writes:
Excerpts from Peter Eisentraut's message of dom ene 15 10:00:03 -0300 2012:
On ons, 2011-12-28 at 14:35 -0500, Tom Lane wrote:
The trouble with using ReadNewTransactionId is that it makes the results
volatile, not stable as the function is declared to be.
Could we alleviate that problem with some caching within the function?
Maybe if we have it be invalidated at transaction end, that could work.
So each new transaction would get a fresh value.
Yeah, I think that would work.
If you had a long
running transaction the cached value would get behind, but maybe this is
not a problem or we could design some protection against it.
Nobody has complained about the fact that age()'s reference point
remains fixed throughout a transaction on the master, so I don't see why
we'd not be happy with that behavior on a standby.
regards, tom lane
Excerpts from Tom Lane's message of lun ene 16 12:27:03 -0300 2012:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Excerpts from Peter Eisentraut's message of dom ene 15 10:00:03 -0300 2012:
On ons, 2011-12-28 at 14:35 -0500, Tom Lane wrote:
The trouble with using ReadNewTransactionId is that it makes the results
volatile, not stable as the function is declared to be.Could we alleviate that problem with some caching within the function?
Maybe if we have it be invalidated at transaction end, that could work.
So each new transaction would get a fresh value.Yeah, I think that would work.
So who's going to work on a patch? Peter, are you? If not, we should
add it to the TODO list.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On mån, 2012-01-16 at 22:59 -0300, Alvaro Herrera wrote:
Excerpts from Tom Lane's message of lun ene 16 12:27:03 -0300 2012:
Alvaro Herrera <alvherre@commandprompt.com> writes:
Excerpts from Peter Eisentraut's message of dom ene 15 10:00:03 -0300 2012:
On ons, 2011-12-28 at 14:35 -0500, Tom Lane wrote:
The trouble with using ReadNewTransactionId is that it makes the results
volatile, not stable as the function is declared to be.Could we alleviate that problem with some caching within the function?
Maybe if we have it be invalidated at transaction end, that could work.
So each new transaction would get a fresh value.Yeah, I think that would work.
So who's going to work on a patch? Peter, are you? If not, we should
add it to the TODO list.
Not at this very moment, but maybe in a few weeks.
Peter Eisentraut <peter_e@gmx.net> writes:
On mån, 2012-01-16 at 22:59 -0300, Alvaro Herrera wrote:
So who's going to work on a patch? Peter, are you? If not, we should
add it to the TODO list.
Not at this very moment, but maybe in a few weeks.
BTW, it strikes me that maybe the coding should work about like this:
if (!TransactionIdIsValid(age_reference_xid))
{
age_reference_xid = GetTopTransactionIdIfAny();
if (!TransactionIdIsValid(age_reference_xid))
age_reference_xid = ReadNewTransactionId();
}
... use age_reference_xid to compute result ...
and of course code somewhere to reset age_reference_xid at end of xact.
The advantage of this is
(1) same code works on master and standby
(2) calling age() no longer requires an otherwise read-only transaction
to acquire an XID.
regards, tom lane
On Wed, Jan 18, 2012 at 7:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Peter Eisentraut <peter_e@gmx.net> writes:
On mån, 2012-01-16 at 22:59 -0300, Alvaro Herrera wrote:
So who's going to work on a patch? Peter, are you? If not, we should
add it to the TODO list.Not at this very moment, but maybe in a few weeks.
BTW, it strikes me that maybe the coding should work about like this:
if (!TransactionIdIsValid(age_reference_xid))
{
age_reference_xid = GetTopTransactionIdIfAny();
if (!TransactionIdIsValid(age_reference_xid))
age_reference_xid = ReadNewTransactionId();
}
... use age_reference_xid to compute result ...and of course code somewhere to reset age_reference_xid at end of xact.
The advantage of this is
(1) same code works on master and standby
(2) calling age() no longer requires an otherwise read-only transaction
to acquire an XID.
Nice solution.
Also it illustrates that some users write code that tries to do things
on a Hot Standby that are supposed to be illegal.
If the OPs error message had returned the correct SQLCODE then it
would have been better able to handle the message.
I think we should apply the patch to return the correct SQLCODE in all
cases, even if its supposedly not possible.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Simon Riggs <simon@2ndQuadrant.com> writes:
I think we should apply the patch to return the correct SQLCODE in all
cases, even if its supposedly not possible.
[ shrug... ] My opinion about that has not changed. Those are internal
sanity checks, and as such, ERRCODE_INTERNAL_ERROR is exactly the right
thing for them. If there are paths that can reach that code, we need to
find them and plug the holes with appropriate user-facing error checks
that say what it is the user is not supposed to do. In this example,
if we had decided that the right answer should be for age() to not be
allowed on standbys, then an error saying exactly that would be an
appropriate user-facing error. "You're not supposed to acquire a
transaction ID" is not intelligible to the average user, and giving it
another error code doesn't improve that situation.
regards, tom lane
On ons, 2012-01-18 at 14:55 -0500, Tom Lane wrote:
BTW, it strikes me that maybe the coding should work about like this:
if (!TransactionIdIsValid(age_reference_xid))
{
age_reference_xid = GetTopTransactionIdIfAny();
if (!TransactionIdIsValid(age_reference_xid))
age_reference_xid = ReadNewTransactionId();
}
... use age_reference_xid to compute result ...and of course code somewhere to reset age_reference_xid at end of xact.
How about this patch?
Attachments:
xid-age-hot-standby.patchtext/x-patch; charset=UTF-8; name=xid-age-hot-standby.patchDownload
diff --git i/src/backend/access/transam/xact.c w/src/backend/access/transam/xact.c
index 7f412b1..25fbd05 100644
--- i/src/backend/access/transam/xact.c
+++ w/src/backend/access/transam/xact.c
@@ -43,6 +43,7 @@
#include "storage/procarray.h"
#include "storage/sinvaladt.h"
#include "storage/smgr.h"
+#include "utils/builtins.h"
#include "utils/combocid.h"
#include "utils/guc.h"
#include "utils/inval.h"
@@ -1938,6 +1939,7 @@ CommitTransaction(void)
AtCommit_Notify();
AtEOXact_GUC(true, 1);
+ AtEOXact_xid();
AtEOXact_SPI(true);
AtEOXact_on_commit_actions(true);
AtEOXact_Namespace(true);
@@ -2191,6 +2193,7 @@ PrepareTransaction(void)
/* PREPARE acts the same as COMMIT as far as GUC is concerned */
AtEOXact_GUC(true, 1);
+ AtEOXact_xid();
AtEOXact_SPI(true);
AtEOXact_on_commit_actions(true);
AtEOXact_Namespace(true);
@@ -2337,6 +2340,7 @@ AbortTransaction(void)
AtEOXact_CatCache(false);
AtEOXact_GUC(false, 1);
+ AtEOXact_xid();
AtEOXact_SPI(false);
AtEOXact_on_commit_actions(false);
AtEOXact_Namespace(false);
diff --git i/src/backend/utils/adt/xid.c w/src/backend/utils/adt/xid.c
index 1829b82..9cf6256 100644
--- i/src/backend/utils/adt/xid.c
+++ w/src/backend/utils/adt/xid.c
@@ -86,22 +86,45 @@ xideq(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(TransactionIdEquals(xid1, xid2));
}
+
/*
* xid_age - compute age of an XID (relative to current xact)
+ *
+ * We don't want to allocate a new transaction ID just for calling age(),
+ * because it should be avoided in an otherwise read-only transaction, and to
+ * make this work under hot standby. But calling ReadNewTransactionId() would
+ * make this function volatile (instead of stable), so we cache the reference
+ * point for the duration of the transaction.
*/
+
+static TransactionId age_reference_xid = InvalidTransactionId;
+
Datum
xid_age(PG_FUNCTION_ARGS)
{
TransactionId xid = PG_GETARG_TRANSACTIONID(0);
- TransactionId now = GetTopTransactionId();
+
+ if (!TransactionIdIsValid(age_reference_xid))
+ {
+ age_reference_xid = GetTopTransactionIdIfAny();
+ if (!TransactionIdIsValid(age_reference_xid))
+ age_reference_xid = ReadNewTransactionId();
+ }
/* Permanent XIDs are always infinitely old */
if (!TransactionIdIsNormal(xid))
PG_RETURN_INT32(INT_MAX);
- PG_RETURN_INT32((int32) (now - xid));
+ PG_RETURN_INT32((int32) (age_reference_xid - xid));
}
+void
+AtEOXact_xid(void)
+{
+ age_reference_xid = InvalidTransactionId;
+}
+
+
/*
* xidComparator
* qsort comparison function for XIDs
diff --git i/src/include/utils/builtins.h w/src/include/utils/builtins.h
index f246f11..b7906b8 100644
--- i/src/include/utils/builtins.h
+++ w/src/include/utils/builtins.h
@@ -795,6 +795,7 @@ extern Datum xidrecv(PG_FUNCTION_ARGS);
extern Datum xidsend(PG_FUNCTION_ARGS);
extern Datum xideq(PG_FUNCTION_ARGS);
extern Datum xid_age(PG_FUNCTION_ARGS);
+extern void AtEOXact_xid(void);
extern int xidComparator(const void *arg1, const void *arg2);
extern Datum cidin(PG_FUNCTION_ARGS);
extern Datum cidout(PG_FUNCTION_ARGS);
On 8 May 2012 20:01, Peter Eisentraut <peter_e@gmx.net> wrote:
On ons, 2012-01-18 at 14:55 -0500, Tom Lane wrote:
BTW, it strikes me that maybe the coding should work about like this:
if (!TransactionIdIsValid(age_reference_xid))
{
age_reference_xid = GetTopTransactionIdIfAny();
if (!TransactionIdIsValid(age_reference_xid))
age_reference_xid = ReadNewTransactionId();
}
... use age_reference_xid to compute result ...and of course code somewhere to reset age_reference_xid at end of xact.
How about this patch?
I think we should fix this, but not with this exact patch.
We should just use MyPgXact->xid
rather than add more to the transaction path
I'll simplify the patch and commit.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On 9 May 2012 00:55, Simon Riggs <simon@2ndquadrant.com> wrote:
On 8 May 2012 20:01, Peter Eisentraut <peter_e@gmx.net> wrote:
On ons, 2012-01-18 at 14:55 -0500, Tom Lane wrote:
BTW, it strikes me that maybe the coding should work about like this:
if (!TransactionIdIsValid(age_reference_xid))
{
age_reference_xid = GetTopTransactionIdIfAny();
if (!TransactionIdIsValid(age_reference_xid))
age_reference_xid = ReadNewTransactionId();
}
... use age_reference_xid to compute result ...and of course code somewhere to reset age_reference_xid at end of xact.
How about this patch?
I think we should fix this, but not with this exact patch.
We should just use MyPgXact->xid
rather than add more to the transaction pathI'll simplify the patch and commit.
Committed, but forgot to give appropriate credit. Sorry about that.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Simon Riggs <simon@2ndQuadrant.com> writes:
We should just use MyPgXact->xid
rather than add more to the transaction pathI'll simplify the patch and commit.
Committed, but forgot to give appropriate credit. Sorry about that.
This patch didn't fix things, it broke things. The former guarantee
that age's reference point would hold still throughout a transaction
just disappeared.
What I read your previous suggestion to be was that age() would keep
local state and use inspection of the current VXID to tell if its
cache was stale. That would keep the fix localized (which I agree
is a good idea) without losing the stability guarantee.
regards, tom lane
On 9 May 2012 15:34, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Simon Riggs <simon@2ndQuadrant.com> writes:
We should just use MyPgXact->xid
rather than add more to the transaction pathI'll simplify the patch and commit.
Committed, but forgot to give appropriate credit. Sorry about that.
This patch didn't fix things, it broke things. The former guarantee
that age's reference point would hold still throughout a transaction
just disappeared.What I read your previous suggestion to be was that age() would keep
local state and use inspection of the current VXID to tell if its
cache was stale. That would keep the fix localized (which I agree
is a good idea) without losing the stability guarantee.
Gotcha. Will fix.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services