Re: [HACKERS] age(xid) on hot standby

2012-05-09 Thread Simon Riggs
On 9 May 2012 00:55, Simon Riggs si...@2ndquadrant.com wrote:
 On 8 May 2012 20:01, Peter Eisentraut pete...@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.

Committed, but forgot to give appropriate credit. Sorry about that.

-- 
 Simon Riggs   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-hackers


Re: [HACKERS] age(xid) on hot standby

2012-05-09 Thread Tom Lane
Simon Riggs si...@2ndquadrant.com writes:
 We should just use MyPgXact-xid
 rather than add more to the transaction path
 
 I'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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers



Re: [HACKERS] age(xid) on hot standby

2012-05-09 Thread Simon Riggs
On 9 May 2012 15:34, Tom Lane t...@sss.pgh.pa.us wrote:
 Simon Riggs si...@2ndquadrant.com writes:
 We should just use MyPgXact-xid
 rather than add more to the transaction path

 I'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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] age(xid) on hot standby

2012-05-08 Thread Peter Eisentraut
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?

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);

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] age(xid) on hot standby

2012-05-08 Thread Simon Riggs
On 8 May 2012 20:01, Peter Eisentraut pete...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] age(xid) on hot standby

2012-01-18 Thread Peter Eisentraut
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 alvhe...@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.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] age(xid) on hot standby

2012-01-18 Thread Tom Lane
Peter Eisentraut pete...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] age(xid) on hot standby

2012-01-18 Thread Simon Riggs
On Wed, Jan 18, 2012 at 7:55 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Peter Eisentraut pete...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] age(xid) on hot standby

2012-01-18 Thread Tom Lane
Simon Riggs si...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] age(xid) on hot standby

2012-01-16 Thread Alvaro Herrera

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 alvhe...@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.
 
 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 alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] age(xid) on hot standby

2012-01-16 Thread Tom Lane
Alvaro Herrera alvhe...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] age(xid) on hot standby

2012-01-16 Thread Alvaro Herrera

Excerpts from Tom Lane's message of lun ene 16 12:27:03 -0300 2012:
 
 Alvaro Herrera alvhe...@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 alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] age(xid) on hot standby

2012-01-15 Thread Peter Eisentraut
On ons, 2011-12-28 at 14:35 -0500, Tom Lane wrote:
 Alvaro Herrera alvhe...@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.

Could we alleviate that problem with some caching within the function?


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] age(xid) on hot standby

2011-12-28 Thread Peter Eisentraut
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?

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] age(xid) on hot standby

2011-12-28 Thread Alvaro Herrera

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 alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] age(xid) on hot standby

2011-12-28 Thread Tom Lane
Alvaro Herrera alvhe...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers