Обсуждение: Re: pgsql: Delay commit status checks until freezing executes.
(Pruning -committers from the list, since cross-posting to -hackers resulted in this being held up for moderation.) On Tue, Jan 3, 2023 at 5:15 PM Peter Geoghegan <pg@bowt.ie> wrote: > > On Tue, Jan 3, 2023 at 4:54 PM Andres Freund <andres@anarazel.de> wrote: > > There's some changes from TransactionIdDidCommit() to !TransactionIdDidAbort() > > that don't look right to me. If the server crashed while xid X was > > in-progress, TransactionIdDidCommit(X) will return false, but so will > > TransactionIdDidAbort(X). So besides moving when the check happens you also > > changed what's being checked in a more substantial way. > > I did point this out on the thread. I made this change with the > intention of making the check more robust. Apparently this was > misguided. > > Where is the behavior that you describe documented, if anywhere? When the server crashes, and we have a problem case, what does TransactionLogFetch()/TransactionIdGetStatus() (which are the guts of both TransactionIdDidCommit and TransactionIdDidAbort) report about the XID? > > Also, why did you change when MarkBufferDirty() happens? Previously it > > happened before we modify the page contents, now after. That's probably fine > > (it's the order suggested in transam/README), but seems like a mighty subtle > > thing to change at the same time as something unrelated, particularly without > > even mentioning it? > > I changed it because the new order is idiomatic. I didn't think that > this was particularly worth mentioning, or even subtle. The logic from > heap_execute_freeze_tuple() only performs simple in-place > modifications. I'm including this here because presumably -hackers will have missed it due to the moderation hold-up issue. -- Peter Geoghegan
Hi, On 2023-01-03 17:54:37 -0800, Peter Geoghegan wrote: > (Pruning -committers from the list, since cross-posting to -hackers > resulted in this being held up for moderation.) I still think these moderation rules are deeply unhelpful... > On Tue, Jan 3, 2023 at 5:15 PM Peter Geoghegan <pg@bowt.ie> wrote: > > > > On Tue, Jan 3, 2023 at 4:54 PM Andres Freund <andres@anarazel.de> wrote: > > > There's some changes from TransactionIdDidCommit() to !TransactionIdDidAbort() > > > that don't look right to me. If the server crashed while xid X was > > > in-progress, TransactionIdDidCommit(X) will return false, but so will > > > TransactionIdDidAbort(X). So besides moving when the check happens you also > > > changed what's being checked in a more substantial way. > > > > I did point this out on the thread. I made this change with the > > intention of making the check more robust. Apparently this was > > misguided. > > > > Where is the behavior that you describe documented, if anywhere? I don't know - I think there's a explicit comment somewhere, but I couldn't find it immediately. There's a bunch of indirect references to in in heapam_visibility.c, with comments like "it must have aborted or crashed". The reason for the behaviour is that we do not have any mechanism for going through the clog and aborting all in-progress-during-crash transactions. So we'll end up with the clog for all in-progress-during-crash transaction being zero / TRANSACTION_STATUS_IN_PROGRESS. IMO it's almost always wrong to use TransactionIdDidAbort(). > When the server crashes, and we have a problem case, what does > TransactionLogFetch()/TransactionIdGetStatus() (which are the guts of > both TransactionIdDidCommit and TransactionIdDidAbort) report about > the XID? Depends a bit on the specifics, but mostly TRANSACTION_STATUS_IN_PROGRESS. > > > Also, why did you change when MarkBufferDirty() happens? Previously it > > > happened before we modify the page contents, now after. That's probably fine > > > (it's the order suggested in transam/README), but seems like a mighty subtle > > > thing to change at the same time as something unrelated, particularly without > > > even mentioning it? > > > > I changed it because the new order is idiomatic. I didn't think that > > this was particularly worth mentioning, or even subtle. The logic from > > heap_execute_freeze_tuple() only performs simple in-place > > modifications. I think changes in how WAL logging is done are just about always worth mentioning in a commit message... Greetings, Andres Freund
On Tue, Jan 3, 2023 at 7:56 PM Andres Freund <andres@anarazel.de> wrote: > I still think these moderation rules are deeply unhelpful... Yes, it is rather annoying. > I don't know - I think there's a explicit comment somewhere, but I couldn't > find it immediately. There's a bunch of indirect references to in in > heapam_visibility.c, with comments like "it must have aborted or > crashed". I think that that's a far cry from any kind of documentation... > The reason for the behaviour is that we do not have any mechanism for going > through the clog and aborting all in-progress-during-crash transactions. So > we'll end up with the clog for all in-progress-during-crash transaction being > zero / TRANSACTION_STATUS_IN_PROGRESS. I find this astonishing. Why isn't there a prominent comment that advertises that TransactionIdDidAbort() just doesn't work reliably? > IMO it's almost always wrong to use TransactionIdDidAbort(). I didn't think that there was any general guarantee about TransactionIdDidAbort() working after a crash. But this is an on-disk XID, taken from some tuple's xmax, which must have a value < OldestXmin. > I think changes in how WAL logging is done are just about always worth > mentioning in a commit message... I agree with that as a general statement, but I never imagined that this was a case that such a statement could apply to. I will try to remember to put something about similar changes in any future commit messages, in the unlikely event that I ever end up moving MarkBufferDirty() around in some existing critical section in the future. -- Peter Geoghegan
On Tue, Jan 3, 2023 at 8:29 PM Peter Geoghegan <pg@bowt.ie> wrote: > I find this astonishing. Why isn't there a prominent comment that > advertises that TransactionIdDidAbort() just doesn't work reliably? I pushed a fix for this now. We should add a comment about this issue to TransactionIdDidAbort() header comments, but I didn't do that part yet. Thanks for the report. -- Peter Geoghegan
Hi, On 2023-01-03 20:29:53 -0800, Peter Geoghegan wrote: > On Tue, Jan 3, 2023 at 7:56 PM Andres Freund <andres@anarazel.de> wrote: > > I don't know - I think there's a explicit comment somewhere, but I couldn't > > find it immediately. There's a bunch of indirect references to in in > > heapam_visibility.c, with comments like "it must have aborted or > > crashed". > > I think that that's a far cry from any kind of documentation... Agreed - not sure if there never were docs, or whether they were accidentally removed. This stuff has been that way for a long time. I'd say a comment above TransactionIdDidAbort() referencing an overview comment at the top of the file? I think it might be worth moving the comment from heapam_visibility.c to transam.c? > > The reason for the behaviour is that we do not have any mechanism for going > > through the clog and aborting all in-progress-during-crash transactions. So > > we'll end up with the clog for all in-progress-during-crash transaction being > > zero / TRANSACTION_STATUS_IN_PROGRESS. > > I find this astonishing. Why isn't there a prominent comment that > advertises that TransactionIdDidAbort() just doesn't work reliably? Arguably it works reliably, just more narrowly than one might think. Treating "crashed transactions" as a distinct state from explicit aborts. Greetings, Andres Freund
On Tue, Jan 3, 2023 at 10:33 PM Andres Freund <andres@anarazel.de> wrote: > I'd say a comment above TransactionIdDidAbort() referencing an overview > comment at the top of the file? I think it might be worth moving the comment > from heapam_visibility.c to transam.c? What comments in heapam_visibility.c should we be referencing here? I don't see anything about it there. I have long been aware that those routines deduce that a transaction must have aborted, but surely that's not nearly enough. That's merely not being broken, without any explanation given as to why. > > I find this astonishing. Why isn't there a prominent comment that > > advertises that TransactionIdDidAbort() just doesn't work reliably? > > Arguably it works reliably, just more narrowly than one might think. Treating > "crashed transactions" as a distinct state from explicit aborts. That's quite a stretch. There are numerous comments that pretty much imply that TransactionIdDidCommit/TransactionIdDidAbort are very similar, for example any discussion of how you need to call TransactionIdIsInProgress first before calling either of the other two. -- Peter Geoghegan
Hi, On 2023-01-03 22:41:35 -0800, Peter Geoghegan wrote: > On Tue, Jan 3, 2023 at 10:33 PM Andres Freund <andres@anarazel.de> wrote: > > I'd say a comment above TransactionIdDidAbort() referencing an overview > > comment at the top of the file? I think it might be worth moving the comment > > from heapam_visibility.c to transam.c? > > What comments in heapam_visibility.c should we be referencing here? I > don't see anything about it there. I have long been aware that those > routines deduce that a transaction must have aborted, but surely > that's not nearly enough. That's merely not being broken, without any > explanation given as to why. IMO the comment at the top mentioning why the TransactionIdIsInProgress() calls are crucial / need to be done first would be considerably more likely to be found in transam.c than heapam_visibility.c. And it'd make sense to have the explanation of why TransactionIdDidAbort() isn't the same as !TransactionIdDidCommit(), even for !TransactionIdIsInProgress() xacts, near the explanation for doing TransactionIdIsInProgress() first. Greetings, Andres Freund
On Tue, Jan 3, 2023 at 10:47 PM Andres Freund <andres@anarazel.de> wrote: > IMO the comment at the top mentioning why the TransactionIdIsInProgress() > calls are crucial / need to be done first would be considerably more likely to > be found in transam.c than heapam_visibility.c. Yeah, but they're duplicated anyway. For example in the transam README. Plus we have references to these same comments from other files, such as heapam.c, which mentions heapam_visibility.c by name as where you go to learn more about this issue. > And it'd make sense to have > the explanation of why TransactionIdDidAbort() isn't the same as > !TransactionIdDidCommit(), even for !TransactionIdIsInProgress() xacts, near > the explanation for doing TransactionIdIsInProgress() first. I think that we should definitely have a comment directly over TransactionIdDidAbort(). Though I wouldn't mind reorganizing these other comments, or making the comment over TransactionIdDidAbort() mostly just point to the other comments. -- Peter Geoghegan
On Wed, Jan 4, 2023 at 1:53 AM Peter Geoghegan <pg@bowt.ie> wrote: > I think that we should definitely have a comment directly over > TransactionIdDidAbort(). Though I wouldn't mind reorganizing these > other comments, or making the comment over TransactionIdDidAbort() > mostly just point to the other comments. Yeah, I think it would be good to have a comment there. As Andres says, it is almost always wrong to use this function, and we should make that more visible. Possibly we should even rename the function, like TransactionIdKnownToHaveAborted(). But that having been said, I'm kind of astonished that you didn't know about this already. The freezing behavior is in general extremely hard to get right, and I guess I feel if you don't understand how the underlying functions work, including things like performance considerations and which functions return fully reliable results, I do not think you should be committing your own patches in this area. There is probably a lot of potential benefit in improving the way this stuff works, but there is also a heck of a lot of danger of creating subtle data corrupting bugs that could easily take years to find. -- Robert Haas EDB: http://www.enterprisedb.com
On Wed, Jan 4, 2023 at 7:03 AM Robert Haas <robertmhaas@gmail.com> wrote: > But that having been said, I'm kind of astonished that you didn't know > about this already. The freezing behavior is in general extremely hard > to get right, and I guess I feel if you don't understand how the > underlying functions work, including things like performance > considerations I was the one that reported the issue with CLOG lookups in the first place. > and which functions return fully reliable results, I do > not think you should be committing your own patches in this area. My mistake here had nothing to do with my own goals. I was trying to be diligent by hardening an existing check in passing, and it backfired. > There is probably a lot of potential benefit in improving the way this > stuff works, but there is also a heck of a lot of danger of creating > subtle data corrupting bugs that could easily take years to find. It's currently possible for VACUUM to set the all-frozen bit while unsetting the all-visible bit, due to a race condition [1]. This is your long standing bug. So apparently nobody is qualified to commit patches in this area. About a year ago, there was a massive argument over some earlier work in the same general area, by me. Being the subject of a pile-on on this mailing list is something that I find deeply upsetting and demoralizing. I just cannot take much more of it. At the same time, I've made quite an investment in the pending patches, and think that it's something that I have to see through. If I am allowed to finish what I've started, then I will stop all new work on VACUUM. I'll go back to working on B-Tree indexing. Nobody is asking me to focus on VACUUM, and there are plenty of other things that I could be doing that don't seem to lead to these situations. [1] https://postgr.es/m/CAH2-WznuNGSzF8v6OsgjaC5aYsb3cZ6HW6MLm30X0d65cmSH6A@mail.gmail.com -- Peter Geoghegan
Hi, On 2023-01-04 09:59:37 -0800, Peter Geoghegan wrote: > On Wed, Jan 4, 2023 at 7:03 AM Robert Haas <robertmhaas@gmail.com> wrote: > > and which functions return fully reliable results, I do > > not think you should be committing your own patches in this area. > > My mistake here had nothing to do with my own goals. I was trying to > be diligent by hardening an existing check in passing, and it > backfired. When moving code around I strongly suggest to make as much of a diff to be "move only". I find git diff --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space quite helpful for that. Being able to see just the "really changed" lines makes it a lot easier to see the crucial parts of a change. > > There is probably a lot of potential benefit in improving the way this > > stuff works, but there is also a heck of a lot of danger of creating > > subtle data corrupting bugs that could easily take years to find. > > It's currently possible for VACUUM to set the all-frozen bit while > unsetting the all-visible bit, due to a race condition [1]. This is > your long standing bug. So apparently nobody is qualified to commit > patches in this area. That's a non-sequitur. Bugs are a fact of programming. > About a year ago, there was a massive argument over some earlier work > in the same general area, by me. Being the subject of a pile-on on > this mailing list is something that I find deeply upsetting and > demoralizing. I just cannot take much more of it. At the same time, > I've made quite an investment in the pending patches, and think that > it's something that I have to see through. I'm, genuinely!, sorry that you feel piled on. That wasn't, and isn't, my goal. I think the area of code desperately needs work. I complained because I didn't like the process and was afraid of the consequences and the perceived need on my part to do post-commit reviews. Greetings, Andres Freund
On Wed, Jan 4, 2023 at 10:41 AM Andres Freund <andres@anarazel.de> wrote: > > It's currently possible for VACUUM to set the all-frozen bit while > > unsetting the all-visible bit, due to a race condition [1]. This is > > your long standing bug. So apparently nobody is qualified to commit > > patches in this area. > > That's a non-sequitur. Bugs are a fact of programming. I agree. > > About a year ago, there was a massive argument over some earlier work > > in the same general area, by me. Being the subject of a pile-on on > > this mailing list is something that I find deeply upsetting and > > demoralizing. I just cannot take much more of it. At the same time, > > I've made quite an investment in the pending patches, and think that > > it's something that I have to see through. > > I'm, genuinely!, sorry that you feel piled on. That wasn't, and isn't, my > goal. Apology accepted. I am making a simple, practical point here, too: I'm much too selfish a person to continue to put myself in this position. I have nothing to prove, and have little to gain over what I'd get out of working in various other areas. I wasn't hired by my current employer to work on VACUUM in particular. In the recent past I have found ways to be very productive in other areas, without any apparent risk of protracted, stressful fights -- which is something that I plan on getting back to soon. I just don't have the stomach for this. It just isn't worth it to me. > I think the area of code desperately needs work. I complained because I > didn't like the process and was afraid of the consequences and the perceived > need on my part to do post-commit reviews. The work that I did in 15 (in particular commit 0b018fab, the "oldest extant XID" commit) really isn't very useful without the other patches in place -- it was always supposed to be one piece of a larger whole. It enables the freezing stuff because VACUUM now "gets credit" for proactive freezing in a way that it didn't before. The motivating examples wiki page shows examples of this [1]. Once the later patches are in place, the 15/16 work on VACUUM will be complete, and I can walk away from working on VACUUM having delivered a very useful improvement to performance stability -- a good outcome for everybody. If you and Robert can find a way to accommodate that, then in all likelihood we won't need to have any more heated and protracted arguments like the one from early in 2022. I will be quite happy to get back to working on B-Tree, likely the skip scan work. [1] https://wiki.postgresql.org/wiki/Freezing/skipping_strategies_patch:_motivating_examples -- Peter Geoghegan
On Wed, Jan 4, 2023 at 11:30 PM Peter Geoghegan <pg@bowt.ie> wrote: > > On Wed, Jan 4, 2023 at 7:03 AM Robert Haas <robertmhaas@gmail.com> wrote: > > But that having been said, I'm kind of astonished that you didn't know > > about this already. The freezing behavior is in general extremely hard > > to get right, and I guess I feel if you don't understand how the > > underlying functions work, including things like performance > > considerations > > I was the one that reported the issue with CLOG lookups in the first place. > > > and which functions return fully reliable results, I do > > not think you should be committing your own patches in this area. > > My mistake here had nothing to do with my own goals. I was trying to > be diligent by hardening an existing check in passing, and it > backfired. > > > There is probably a lot of potential benefit in improving the way this > > stuff works, but there is also a heck of a lot of danger of creating > > subtle data corrupting bugs that could easily take years to find. > > It's currently possible for VACUUM to set the all-frozen bit while > unsetting the all-visible bit, due to a race condition [1]. This is > your long standing bug. So apparently nobody is qualified to commit > patches in this area. > > About a year ago, there was a massive argument over some earlier work > in the same general area, by me. Being the subject of a pile-on on > this mailing list is something that I find deeply upsetting and > demoralizing. I just cannot take much more of it. At the same time, > I've made quite an investment in the pending patches, and think that > it's something that I have to see through. > > If I am allowed to finish what I've started, then I will stop all new > work on VACUUM. > +1 for you to continue your work in this area. Personally, I don't feel you need to stop working in VACUUM especially now that you have built a good knowledge in this area and have a grip over the improvement areas. AFAICS, the main takeaway is to get a review of one's own work which I see in your case that Jeff is already doing in the main project work. So, continuing with that and having some more reviews should avoid such complaints. It is always possible that you, me, or anyone can miss something important even after detailed reviews by others but I think the chances will be much lower. You are an extremely valuable person for this project and I wish that you continue working with the same enthusiasm. -- With Regards, Amit Kapila.
On Wed, Jan 4, 2023 at 10:59 PM Amit Kapila <amit.kapila16@gmail.com> wrote: > You are an extremely valuable person for this project and I wish that > you continue working with the same enthusiasm. Thank you, Amit. Knowing that my efforts are appreciated by colleagues does make it easier to persevere. -- Peter Geoghegan
On Tue, Jan 3, 2023 at 10:52 PM Peter Geoghegan <pg@bowt.ie> wrote: > > And it'd make sense to have > > the explanation of why TransactionIdDidAbort() isn't the same as > > !TransactionIdDidCommit(), even for !TransactionIdIsInProgress() xacts, near > > the explanation for doing TransactionIdIsInProgress() first. > > I think that we should definitely have a comment directly over > TransactionIdDidAbort(). Though I wouldn't mind reorganizing these > other comments, or making the comment over TransactionIdDidAbort() > mostly just point to the other comments. What do you think of the attached patch, which revises comments over TransactionIdDidAbort, and adds something about it to the top of heapam_visbility.c? -- Peter Geoghegan
Вложения
Hi, On 2023-01-06 11:16:00 -0800, Peter Geoghegan wrote: > On Tue, Jan 3, 2023 at 10:52 PM Peter Geoghegan <pg@bowt.ie> wrote: > > > And it'd make sense to have > > > the explanation of why TransactionIdDidAbort() isn't the same as > > > !TransactionIdDidCommit(), even for !TransactionIdIsInProgress() xacts, near > > > the explanation for doing TransactionIdIsInProgress() first. > > > > I think that we should definitely have a comment directly over > > TransactionIdDidAbort(). Though I wouldn't mind reorganizing these > > other comments, or making the comment over TransactionIdDidAbort() > > mostly just point to the other comments. > > What do you think of the attached patch, which revises comments over > TransactionIdDidAbort, and adds something about it to the top of > heapam_visbility.c? Mostly looks good to me. I think it'd be good to add a reference to the heapam_visbility.c? comment to the top of transam.c (or move it). > * Assumes transaction identifier is valid and exists in clog. > + * > + * Not all transactions that must be treated as aborted will be > + * explicitly marked as such in clog. Transactions that were in > + * progress during a crash are never reported as aborted by us. > */ > bool /* true if given transaction aborted */ > TransactionIdDidAbort(TransactionId transactionId) I think it's currently very likely to be true, but I'd weaken the "never" a bit nonetheless. I think it'd also be good to point to what to do instead. How about: Note that TransactionIdDidAbort() returns true only for explicitly aborted transactions, as transactions implicitly aborted due to a crash will commonly still appear to be in-progress in the clog. Most of the time TransactionIdDidCommit(), with a preceding TransactionIdIsInProgress() check, should be used instead of TransactionIdDidAbort(). Greetings, Andres Freund
On Sat, Jan 7, 2023 at 1:47 PM Andres Freund <andres@anarazel.de> wrote: > > What do you think of the attached patch, which revises comments over > > TransactionIdDidAbort, and adds something about it to the top of > > heapam_visbility.c? > > Mostly looks good to me. I think it'd be good to add a reference to the > heapam_visbility.c? comment to the top of transam.c (or move it). Makes sense. > I think it's currently very likely to be true, but I'd weaken the "never" a > bit nonetheless. I think it'd also be good to point to what to do instead. How > about: > Note that TransactionIdDidAbort() returns true only for explicitly aborted > transactions, as transactions implicitly aborted due to a crash will > commonly still appear to be in-progress in the clog. Most of the time > TransactionIdDidCommit(), with a preceding TransactionIdIsInProgress() > check, should be used instead of TransactionIdDidAbort(). That does seem better. Do we need to do anything about this to the "pg_xact and pg_subtrans" section of the transam README? Also, does amcheck's get_xid_status() need a reference to these rules? FWIW, I found an existing comment about this rule in the call to TransactionIdAbortTree() from RecordTransactionAbort() -- which took me quite a while to find. So you might have been remembering that comment before. -- Peter Geoghegan
Hi, On 2023-01-07 15:41:29 -0800, Peter Geoghegan wrote: > Do we need to do anything about this to the "pg_xact and pg_subtrans" > section of the transam README? Probably a good idea, although it doesn't neatly fit right now. > Also, does amcheck's get_xid_status() need a reference to these rules? Don't think so? Whad made you ask? > FWIW, I found an existing comment about this rule in the call to > TransactionIdAbortTree() from RecordTransactionAbort() -- which took > me quite a while to find. So you might have been remembering that > comment before. Possible, my memory is vague enough that it's hard to be sure... Greetings, Andres Freund
On Sat, Jan 7, 2023 at 7:25 PM Andres Freund <andres@anarazel.de> wrote: > Probably a good idea, although it doesn't neatly fit right now. I'll leave it for now. Attached is v2, which changes things based on your feedback. Would like to get this out of the way soon. > > Also, does amcheck's get_xid_status() need a reference to these rules? > > Don't think so? Whad made you ask? Just the fact that it seems to more or less follow the protocol described at the top of heapam_visibility.c. Not very important, though. -- Peter Geoghegan
Вложения
Hi, On 2023-01-11 14:29:25 -0800, Peter Geoghegan wrote: > On Sat, Jan 7, 2023 at 7:25 PM Andres Freund <andres@anarazel.de> wrote: > > Probably a good idea, although it doesn't neatly fit right now. > > I'll leave it for now. > > Attached is v2, which changes things based on your feedback. Would > like to get this out of the way soon. Makes sense. It's clearly an improvement. > + * We can't use TransactionIdDidAbort here because it won't treat transactions > + * that were in progress during a crash as aborted by now. We determine that > + * transactions aborted/crashed through process of elimination instead. s/by now//? > * When using an MVCC snapshot, we rely on XidInMVCCSnapshot rather than > * TransactionIdIsInProgress, but the logic is otherwise the same: do not > diff --git a/src/backend/access/transam/transam.c b/src/backend/access/transam/transam.c > index 3a28dcc43..7629904bb 100644 > --- a/src/backend/access/transam/transam.c > +++ b/src/backend/access/transam/transam.c > @@ -110,7 +110,8 @@ TransactionLogFetch(TransactionId transactionId) > * transaction tree. > * > * See also TransactionIdIsInProgress, which once was in this module > - * but now lives in procarray.c. > + * but now lives in procarray.c, as well as comments at the top of > + * heapam_visibility.c that explain how everything fits together. > * ---------------------------------------------------------------- > */ +1 Greetings, Andres Freund
On Wed, Jan 11, 2023 at 3:06 PM Andres Freund <andres@anarazel.de> wrote: > > + * We can't use TransactionIdDidAbort here because it won't treat transactions > > + * that were in progress during a crash as aborted by now. We determine that > > + * transactions aborted/crashed through process of elimination instead. > > s/by now//? Did it that way in the commit I pushed just now. Thanks -- Peter Geoghegan