Обсуждение: age(xid) on hot standby
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
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 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?
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 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 <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
tocompute 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?
Вложения
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 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
Simon Riggs <simon@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
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 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