Обсуждение: Re: [COMMITTERS] pgsql: Ensure age() returns a stable value rather than the latest value

Simon Riggs <simon@2ndQuadrant.com> writes:
> Ensure age() returns a stable value rather than the latest value

Hm.  This fixes the stability-within-transaction problem, but it's also
introduced a change in the definition of age(), no?  Previously, in an
xact that had an XID, the age was measured relative to that XID.
I'm not sure that we should lightly abandon that correspondence.
At the very least we would need to update the user-facing documentation,
not only the function's header comment.  So far as I can find, the only
such documentation is the pg_description entry:
DESCR("age of a transaction ID, in transactions before current transaction");
but that's still wrong now.

The definition I was thinking of was "if xact has an XID use that, else
do ReadNewTransactionId, and in either case save the value for later
calls during the current virtual xact."  This is more complicated than
what you implemented, and maybe we shouldn't be quite that tense about
backwards-compatibility.  But I don't think we should be changing the
function's definition like you've done in back branches.
        regards, tom lane

On 11 May 2012 17:13, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
>> Ensure age() returns a stable value rather than the latest value
>
> Hm.  This fixes the stability-within-transaction problem, but it's also
> introduced a change in the definition of age(), no?  Previously, in an
> xact that had an XID, the age was measured relative to that XID.
> I'm not sure that we should lightly abandon that correspondence.
> At the very least we would need to update the user-facing documentation,
> not only the function's header comment.  So far as I can find, the only
> such documentation is the pg_description entry:
> DESCR("age of a transaction ID, in transactions before current transaction");
> but that's still wrong now.
>
> The definition I was thinking of was "if xact has an XID use that, else
> do ReadNewTransactionId, and in either case save the value for later
> calls during the current virtual xact."  This is more complicated than
> what you implemented, and maybe we shouldn't be quite that tense about
> backwards-compatibility.  But I don't think we should be changing the
> function's definition like you've done in back branches.

Yeh, I thought about that.

The likely difference between the old and the new result is likely to
be small, especially in the main intended use case. The previous
definition was fairly weird, since if you executed it in a long
running transaction it would give a false reading of the actual age,
which ISTM was a bug in itself.

What would be more confusing would be to have age() return a different
result on standby and master.

At present the back branches just throw ERROR, so some change is
needed there at least, given our earlier policy of keeping that ERROR
as a backstop rather than as an acceptable return (re: SQLcode
discussions).

I've no objection to further change, but I think I've done the best
thing out of the various options.

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Simon Riggs <simon@2ndQuadrant.com> writes:
> On 11 May 2012 17:13, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Hm.  This fixes the stability-within-transaction problem, but it's also
>> introduced a change in the definition of age(), no?

> Yeh, I thought about that.

> The likely difference between the old and the new result is likely to
> be small, especially in the main intended use case. The previous
> definition was fairly weird, since if you executed it in a long
> running transaction it would give a false reading of the actual age,
> which ISTM was a bug in itself.

Well, this definition could also give unexpected readings, depending on
when the first call to age() occurred within the long-running xact.
I'm not sure there's any fix for that unless you want to redefine age()
as a volatile function that uses ReadNewTransactionId on *each* call;
and that doesn't sound like a more useful definition to me.

> What would be more confusing would be to have age() return a different
> result on standby and master.

But surely you can get that anyway, since ReadNewTransactionId is
unlikely to give the same results on standby and master.

> At present the back branches just throw ERROR, so some change is
> needed there at least, given our earlier policy of keeping that ERROR
> as a backstop rather than as an acceptable return (re: SQLcode
> discussions).

> I've no objection to further change, but I think I've done the best
> thing out of the various options.

I'm not convinced this is the best thing, and I'm definitely not happy
with changing the behavior of working cases (ie, behavior on the master)
in the back branches.

Anybody else have an opinion on this?
        regards, tom lane

On Fri, May 11, 2012 at 1:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm not convinced this is the best thing, and I'm definitely not happy
> with changing the behavior of working cases (ie, behavior on the master)
> in the back branches.
>
> Anybody else have an opinion on this?

I agree with you.  Essentially, if we want age() to be stable, and
nobody's argued against that, we have to fix a point in the XID space
and do all of our computations relative to that point.  The original
coding did that be using our XID, and I think what we ought to do is
use either (1) our XID or (2) the next XID as of the first point in
time at which age() is called, if we don't have an XID yet then.
That's a slight behavior change even when not in HS mode, because a
read-only transaction won't acquire an XID just by virtue of using
age(), so I would probably have chosen to fix this only in master and
to not back-patch anything, but in practice I think the downside of
that behavior change is very minimal (and it might even be an equally
minimal improvement for some people) so I think a back-patch is fine.
However, I don't really see any particular merit in removing our own
XID from the picture entirely: that changes the behavior more
significantly for no particular benefit.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> ... I don't really see any particular merit in removing our own
> XID from the picture entirely: that changes the behavior more
> significantly for no particular benefit.

The merit would be in keeping the function's definition simple.

Anyway, let's see if breaking this down by cases clarifies anything.
As I see it, there are three possible cases:

1. On master, xact already has an XID.  The longstanding behavior is
to use that XID as reference.  The committed patch changes this to
reference whatever is next-to-assign XID at first call of age(), but
it's far from clear to me that that's better for this case in isolation.

2. On master, xact does not (yet) have an XID.  The previous behavior
is to force XID assignment at first call of age().  However, if we
capture ReadNewTransactionId as per patch then we give the same answer
as we would have done before, only without assigning the xact an XID.
It could be argued that this can yield inconsistent results if the xact
later does something that forces XID assignment anyway, but surely
that's a pretty narrow corner case.

3. On slave, so xact cannot have an XID.  Previous behavior is to fail
which we all agree is unhelpful.  Capturing ReadNewTransactionId
provides behavior somewhat similar to patched case #2, though it's
unclear to me exactly how compatible it is given the likely skew between
master and slave notions of the next XID.

It's arguable that what we should do is "use XID if on master, capture
ReadNewTransactionId if on slave", which would avoid any backwards
incompatibility for the first two cases while still fixing the case that
everybody agrees is a problem.  Simon argues that this gives a weird
variance in the master vs slave behavior, but I'm not sure I believe
that's an issue.  In case 2, the only way that the user can tell the
difference between force-XID-assignment and capture-ReadNewTransactionId
is if the transaction later does something requiring an XID, which
cannot happen anyway on the slave.  So from here the difference in these
behaviors seems minimal and not worth creating incompatibility in the
first two cases for.
        regards, tom lane

On Fri, May 11, 2012 at 2:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The merit would be in keeping the function's definition simple.

True.  It's not *much* simpler, but it is simpler.

> Anyway, let's see if breaking this down by cases clarifies anything.
> As I see it, there are three possible cases:
>
> 1. On master, xact already has an XID.  The longstanding behavior is
> to use that XID as reference.  The committed patch changes this to
> reference whatever is next-to-assign XID at first call of age(), but
> it's far from clear to me that that's better for this case in isolation.
>
> 2. On master, xact does not (yet) have an XID.  The previous behavior
> is to force XID assignment at first call of age().  However, if we
> capture ReadNewTransactionId as per patch then we give the same answer
> as we would have done before, only without assigning the xact an XID.
> It could be argued that this can yield inconsistent results if the xact
> later does something that forces XID assignment anyway, but surely
> that's a pretty narrow corner case.
>
> 3. On slave, so xact cannot have an XID.  Previous behavior is to fail
> which we all agree is unhelpful.  Capturing ReadNewTransactionId
> provides behavior somewhat similar to patched case #2, though it's
> unclear to me exactly how compatible it is given the likely skew between
> master and slave notions of the next XID.
>
> It's arguable that what we should do is "use XID if on master, capture
> ReadNewTransactionId if on slave", which would avoid any backwards
> incompatibility for the first two cases while still fixing the case that
> everybody agrees is a problem.  Simon argues that this gives a weird
> variance in the master vs slave behavior, but I'm not sure I believe
> that's an issue.  In case 2, the only way that the user can tell the
> difference between force-XID-assignment and capture-ReadNewTransactionId
> is if the transaction later does something requiring an XID, which
> cannot happen anyway on the slave.  So from here the difference in these
> behaviors seems minimal and not worth creating incompatibility in the
> first two cases for.

Yeah.  I don't think I particularly care what we do in HEAD, but it
sure seems like it would be nice to change the back-branch behavior as
little as possible.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 11 May 2012 19:17, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>
> It's arguable that what we should do is "use XID if on master, capture
> ReadNewTransactionId if on slave", which would avoid any backwards
> incompatibility for the first two cases while still fixing the case that
> everybody agrees is a problem.  Simon argues that this gives a weird
> variance in the master vs slave behavior, but I'm not sure I believe
> that's an issue.  In case 2, the only way that the user can tell the
> difference between force-XID-assignment and capture-ReadNewTransactionId
> is if the transaction later does something requiring an XID, which
> cannot happen anyway on the slave.  So from here the difference in these
> behaviors seems minimal and not worth creating incompatibility in the
> first two cases for.

Case (2) is more complex than described. If we use XID always, then
the so-say stable value could change mid way through a scan when the
XID is assigned and would provide neither a stable, sensible nor a
backwards compatible response.

We can only use XID if it already exists AND age() has not yet been
executed by this transaction. Which is case (1).

Case (1) does show changed behaviour. I didn't regard that as
important because the normal use case for this is a short read only
request, so case (1) is not a typical usage of the function. Given
solving case (1) means breaking case (3), I'm not convinced it is
sensible, but as you say it would be incompatible and so I'll change
it to use XID according to my stated rule above.

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Simon Riggs <simon@2ndQuadrant.com> writes:
> Case (2) is more complex than described. If we use XID always, then
> the so-say stable value could change mid way through a scan when the
> XID is assigned and would provide neither a stable, sensible nor a
> backwards compatible response.

No, that's entirely wrong.  The original behavior of the function
for case 2, which I am proposing we revert to,  is that it would
forcibly assign an XID when the transaction didn't already have one.
Subsequently, that value would be stable for the duraction of the xact.
        regards, tom lane

On 12 May 2012 15:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
>> Case (2) is more complex than described. If we use XID always, then
>> the so-say stable value could change mid way through a scan when the
>> XID is assigned and would provide neither a stable, sensible nor a
>> backwards compatible response.
>
> No, that's entirely wrong.  The original behavior of the function
> for case 2, which I am proposing we revert to,  is that it would
> forcibly assign an XID when the transaction didn't already have one.
> Subsequently, that value would be stable for the duraction of the xact.

As you said yourself, assigning an XID is exactly the same as using
ReadNewTransactionId(). There is no difference in behaviour for case
2.

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Simon Riggs <simon@2ndQuadrant.com> writes:
> On 12 May 2012 15:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Simon Riggs <simon@2ndQuadrant.com> writes:
>>> Case (2) is more complex than described. If we use XID always, then
>>> the so-say stable value could change mid way through a scan when the
>>> XID is assigned and would provide neither a stable, sensible nor a
>>> backwards compatible response.

>> No, that's entirely wrong.  The original behavior of the function
>> for case 2, which I am proposing we revert to,  is that it would
>> forcibly assign an XID when the transaction didn't already have one.
>> Subsequently, that value would be stable for the duraction of the xact.

> As you said yourself, assigning an XID is exactly the same as using
> ReadNewTransactionId(). There is no difference in behaviour for case
> 2.

What I said was that there is no difference in behavior with respect to
the value returned by age() itself.  There *is* a difference in overall
behavior if the transaction calling age() does something that requires
an XID later on; which is more or less what I thought you were pointing
out in the first snippet quoted above.  Consider a transaction
consisting of
INSERT INTO log_table SELECT ..., age(xmin), ... FROM some_table;

In all previous releases of PG, the rows inserted into log_table will
have xmin equal to the reference XID used by the age() calculation,
so that it would be possible to cross-check those rows against the xmins
of the source rows in some_table.  With the behavior you're arguing for,
this stops working, because (at least for the first row) age() is
executed before an XID has been acquired by the INSERT.

Now it's entirely likely that there is nobody out there relying on such
a thing, but nonetheless this is a compatibility break, and an
unnecessary one IMO.  You haven't shown any convincing reason why we
need to change the behavior of age() on master servers at all.

To put it a different way: the argument I was trying to illustrate by
breaking down the different cases is that "use XID on master and capture
ReadNewTransactionId on slave" is close enough to being the same
behavior for each that it shouldn't be a problem.  The value that a hot
standby transaction will get is indistinguishable from what it would
have gotten if running on the master, and the fact that the underlying
implementation is entirely different could only be detected by a
transaction that could write, which the HS transaction cannot.  So
I think we should do that, and thereby not create any change at all
in the behavior of age() on master servers.
        regards, tom lane

On 12 May 2012 17:59, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
>> On 12 May 2012 15:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Simon Riggs <simon@2ndQuadrant.com> writes:
>>>> Case (2) is more complex than described. If we use XID always, then
>>>> the so-say stable value could change mid way through a scan when the
>>>> XID is assigned and would provide neither a stable, sensible nor a
>>>> backwards compatible response.
>
>>> No, that's entirely wrong.  The original behavior of the function
>>> for case 2, which I am proposing we revert to,  is that it would
>>> forcibly assign an XID when the transaction didn't already have one.
>>> Subsequently, that value would be stable for the duraction of the xact.
>
>> As you said yourself, assigning an XID is exactly the same as using
>> ReadNewTransactionId(). There is no difference in behaviour for case
>> 2.
>
> What I said was that there is no difference in behavior with respect to
> the value returned by age() itself.  There *is* a difference in overall
> behavior if the transaction calling age() does something that requires
> an XID later on; which is more or less what I thought you were pointing
> out in the first snippet quoted above.  Consider a transaction
> consisting of
>
>        INSERT INTO log_table SELECT ..., age(xmin), ... FROM some_table;
>
> In all previous releases of PG, the rows inserted into log_table will
> have xmin equal to the reference XID used by the age() calculation,
> so that it would be possible to cross-check those rows against the xmins
> of the source rows in some_table.  With the behavior you're arguing for,
> this stops working, because (at least for the first row) age() is
> executed before an XID has been acquired by the INSERT.
>
> Now it's entirely likely that there is nobody out there relying on such
> a thing, but nonetheless this is a compatibility break, and an
> unnecessary one IMO.  You haven't shown any convincing reason why we
> need to change the behavior of age() on master servers at all.
>
> To put it a different way: the argument I was trying to illustrate by
> breaking down the different cases is that "use XID on master and capture
> ReadNewTransactionId on slave" is close enough to being the same
> behavior for each that it shouldn't be a problem.  The value that a hot
> standby transaction will get is indistinguishable from what it would
> have gotten if running on the master, and the fact that the underlying
> implementation is entirely different could only be detected by a
> transaction that could write, which the HS transaction cannot.  So
> I think we should do that, and thereby not create any change at all
> in the behavior of age() on master servers.

As of my last commit, each of these three transactions return exactly
the same answers at each point that they always did:

SELECT age(relfrozenxid) from pg_class;

INSERT INTO foo
SELECT age(relfrozenxid) from pg_class;

BEGIN;
SELECT age(relfrozenxid) from pg_class;
SELECT txid_current();
SELECT age(relfrozenxid) from pg_class;
COMMIT;

but now the first works on Hot Standby too. Isn't that what we want?

I have done as you requested and ensured no change of behaviour occurs.

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On lör, 2012-05-12 at 12:59 -0400, Tom Lane wrote:
> Now it's entirely likely that there is nobody out there relying on
> such a thing, but nonetheless this is a compatibility break, and an
> unnecessary one IMO.  You haven't shown any convincing reason why we
> need to change the behavior of age() on master servers at all.

Recall that this thread originally arose out of age() being called by a
monitoring tool.  It would be nice if repeatedly calling age() on an
otherwise idle database would not change the result.  Currently, you
would never get a "stable" state on such a check, and moreover, you
would not only get different results but different long-term behavior
between master and standby.  Now this is not that important and can be
accommodated in the respective tools, but it is kind of weird.  It would
be like a check for disk space losing one byte at every check, even if
you got it back later.

Peter Eisentraut <peter_e@gmx.net> writes:
> On lör, 2012-05-12 at 12:59 -0400, Tom Lane wrote:
>> Now it's entirely likely that there is nobody out there relying on
>> such a thing, but nonetheless this is a compatibility break, and an
>> unnecessary one IMO.  You haven't shown any convincing reason why we
>> need to change the behavior of age() on master servers at all.

> Recall that this thread originally arose out of age() being called by a
> monitoring tool.  It would be nice if repeatedly calling age() on an
> otherwise idle database would not change the result.  Currently, you
> would never get a "stable" state on such a check, and moreover, you
> would not only get different results but different long-term behavior
> between master and standby.

Hm.  Interesting argument, but why exactly would you expect that age()
would work differently from, say, wall clock time?  And how likely is it
that a database that requires monitoring is going to have exactly zero
transactions over a significant length of time?

(In any case, my primary beef at the moment is not with whether it's a
good idea to change age()'s behavior going forward, but rather with
having back-patched such a change.)
        regards, tom lane

On mån, 2012-05-14 at 15:11 -0400, Tom Lane wrote:
> Hm.  Interesting argument, but why exactly would you expect that age()
> would work differently from, say, wall clock time?  And how likely is
> it that a database that requires monitoring is going to have exactly
> zero transactions over a significant length of time?

Yes, it will be a marginal case in practice, but it's something that a
curious DBA might wonder about.  But I think your example how age()
behaves relative to an INSERT statement is more important.
> 
> (In any case, my primary beef at the moment is not with whether it's a
> good idea to change age()'s behavior going forward, but rather with
> having back-patched such a change.)

Certainly we should leave it alone there.

On 14 May 2012 20:05, Peter Eisentraut <peter_e@gmx.net> wrote:
> On lör, 2012-05-12 at 12:59 -0400, Tom Lane wrote:
>> Now it's entirely likely that there is nobody out there relying on
>> such a thing, but nonetheless this is a compatibility break, and an
>> unnecessary one IMO.  You haven't shown any convincing reason why we
>> need to change the behavior of age() on master servers at all.
>
> Recall that this thread originally arose out of age() being called by a
> monitoring tool.  It would be nice if repeatedly calling age() on an
> otherwise idle database would not change the result.  Currently, you
> would never get a "stable" state on such a check, and moreover, you
> would not only get different results but different long-term behavior
> between master and standby.

That's how it works now.

--
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Peter Eisentraut <peter_e@gmx.net> writes:
> On mån, 2012-05-14 at 15:11 -0400, Tom Lane wrote:
>> (In any case, my primary beef at the moment is not with whether it's a
>> good idea to change age()'s behavior going forward, but rather with
>> having back-patched such a change.)

> Certainly we should leave it alone there.

With back-branch update releases due to be made this week, we need to
decide what if anything we're going to do about changing this.
I have no particular complaint with what Simon's done in HEAD, but
back-patching it was not wise IMO.  I think we should just revert the
patches in the back branches and go back to the way it was before
(complete with failures on slave servers).
        regards, tom lane