Horton

Thread

Commits

  1. Improve TransactionIdDidAbort() documentation.

  2. Check that xmax didn't commit in freeze check.

  3. Delay commit status checks until freezing executes.

  4. Add page-level freezing to VACUUM.

  5. Set relfrozenxid to oldest extant XID seen by VACUUM.

  1. pgsql: Delay commit status checks until freezing executes.

    Peter Geoghegan <pg@bowt.ie> — 2023-01-03T19:23:41Z

    Delay commit status checks until freezing executes.

pg_xact lookups are relatively expensive.  Move the xmin/xmax commit
status checks from the point that freeze plans are prepared to the point
that they're actually executed.  Otherwise we'll repeat many commit
status checks whenever multiple successive VACUUM operations scan the
same pages and decide against freezing each time, which is a waste of
cycles.

Oversight in commit 1de58df4, which added page-level freezing.

Author: Peter Geoghegan <pg@bowt.ie>
Discussion: https://postgr.es/m/CAH2-WzkZpe4K6qMfEt8H4qYJCKc2R7TPvKsBva7jc9w7iGXQSw@mail.gmail.com

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/79d4bf4eff14d8967b10ad4c60039c1b9b0cf66e

Modified Files
--------------
src/backend/access/heap/heapam.c | 89 ++++++++++++++++++++++++++++------------
src/include/access/heapam.h      |  9 ++++
2 files changed, 71 insertions(+), 27 deletions(-)

  2. Re: pgsql: Delay commit status checks until freezing executes.

    Andres Freund <andres@anarazel.de> — 2023-01-04T00:54:33Z

    Hi,

On 2023-01-03 19:23:41 +0000, Peter Geoghegan wrote:
> Delay commit status checks until freezing executes.
> 
> pg_xact lookups are relatively expensive.  Move the xmin/xmax commit
> status checks from the point that freeze plans are prepared to the point
> that they're actually executed.  Otherwise we'll repeat many commit
> status checks whenever multiple successive VACUUM operations scan the
> same pages and decide against freezing each time, which is a waste of
> cycles.
> 
> Oversight in commit 1de58df4, which added page-level freezing.
> 
> Author: Peter Geoghegan <pg@bowt.ie>
> Discussion: https://postgr.es/m/CAH2-WzkZpe4K6qMfEt8H4qYJCKc2R7TPvKsBva7jc9w7iGXQSw@mail.gmail.com

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.


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?


Greetings,

Andres Freund

  3. Re: pgsql: Delay commit status checks until freezing executes.

    Peter Geoghegan <pg@bowt.ie> — 2023-01-04T01:15:44Z

    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?

> 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.

-- 
Peter Geoghegan

  4. Re: pgsql: Delay commit status checks until freezing executes.

    Peter Geoghegan <pg@bowt.ie> — 2023-01-04T01:54:37Z

    (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

  5. Re: pgsql: Delay commit status checks until freezing executes.

    Andres Freund <andres@anarazel.de> — 2023-01-04T03:56:36Z

    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

  6. Re: pgsql: Delay commit status checks until freezing executes.

    Peter Geoghegan <pg@bowt.ie> — 2023-01-04T04:29:53Z

    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

  7. Re: pgsql: Delay commit status checks until freezing executes.

    Peter Geoghegan <pg@bowt.ie> — 2023-01-04T05:50:48Z

    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

  8. Re: pgsql: Delay commit status checks until freezing executes.

    Andres Freund <andres@anarazel.de> — 2023-01-04T06:33:25Z

    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

  9. Re: pgsql: Delay commit status checks until freezing executes.

    Peter Geoghegan <pg@bowt.ie> — 2023-01-04T06:41:35Z

    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

  10. Re: pgsql: Delay commit status checks until freezing executes.

    Andres Freund <andres@anarazel.de> — 2023-01-04T06:47:40Z

    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

  11. Re: pgsql: Delay commit status checks until freezing executes.

    Peter Geoghegan <pg@bowt.ie> — 2023-01-04T06:52:51Z

    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

  12. Re: pgsql: Delay commit status checks until freezing executes.

    Robert Haas <robertmhaas@gmail.com> — 2023-01-04T15:02:59Z

    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

  13. Re: pgsql: Delay commit status checks until freezing executes.

    Peter Geoghegan <pg@bowt.ie> — 2023-01-04T17:59:37Z

    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

  14. Re: pgsql: Delay commit status checks until freezing executes.

    Andres Freund <andres@anarazel.de> — 2023-01-04T18:41:10Z

    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

  15. Re: pgsql: Delay commit status checks until freezing executes.

    Peter Geoghegan <pg@bowt.ie> — 2023-01-04T21:05:33Z

    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

  16. Re: pgsql: Delay commit status checks until freezing executes.

    Amit Kapila <amit.kapila16@gmail.com> — 2023-01-05T06:58:50Z

    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.

  17. Re: pgsql: Delay commit status checks until freezing executes.

    Peter Geoghegan <pg@bowt.ie> — 2023-01-05T18:57:05Z

    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

  18. Re: pgsql: Delay commit status checks until freezing executes.

    Peter Geoghegan <pg@bowt.ie> — 2023-01-06T19:16:00Z

    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

  19. Re: pgsql: Delay commit status checks until freezing executes.

    Andres Freund <andres@anarazel.de> — 2023-01-07T21:47:56Z

    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

  20. Re: pgsql: Delay commit status checks until freezing executes.

    Peter Geoghegan <pg@bowt.ie> — 2023-01-07T23:41:29Z

    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

  21. Re: pgsql: Delay commit status checks until freezing executes.

    Andres Freund <andres@anarazel.de> — 2023-01-08T03:25:49Z

    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

  22. Re: pgsql: Delay commit status checks until freezing executes.

    Peter Geoghegan <pg@bowt.ie> — 2023-01-11T22:29:25Z

    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

  23. Re: pgsql: Delay commit status checks until freezing executes.

    Andres Freund <andres@anarazel.de> — 2023-01-11T23:06:56Z

    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

  24. Re: pgsql: Delay commit status checks until freezing executes.

    Peter Geoghegan <pg@bowt.ie> — 2023-01-11T23:32:49Z

    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