Обсуждение: 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
Re: Re: [COMMITTERS] pgsql: Ensure age() returns a stable value rather than the latest value
От
Peter Eisentraut
Дата:
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
Re: Re: [COMMITTERS] pgsql: Ensure age() returns a stable value rather than the latest value
От
Peter Eisentraut
Дата:
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