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

2023-01-11 Thread Peter Geoghegan
On Wed, Jan 11, 2023 at 3:06 PM Andres Freund 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

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

2023-01-11 Thread Andres Freund
Hi, On 2023-01-11 14:29:25 -0800, Peter Geoghegan wrote: > On Sat, Jan 7, 2023 at 7:25 PM Andres Freund 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

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

2023-01-11 Thread Peter Geoghegan
On Sat, Jan 7, 2023 at 7:25 PM Andres Freund 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

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

2023-01-07 Thread Andres Freund
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

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

2023-01-07 Thread Peter Geoghegan
On Sat, Jan 7, 2023 at 1:47 PM Andres Freund 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 >

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

2023-01-07 Thread Andres Freund
Hi, On 2023-01-06 11:16:00 -0800, Peter Geoghegan wrote: > On Tue, Jan 3, 2023 at 10:52 PM Peter Geoghegan wrote: > > > And it'd make sense to have > > > the explanation of why TransactionIdDidAbort() isn't the same as > > > !TransactionIdDidCommit(), even for !TransactionIdIsInProgress() xacts,

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

2023-01-06 Thread Peter Geoghegan
On Tue, Jan 3, 2023 at 10:52 PM Peter Geoghegan 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()

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

2023-01-05 Thread Peter Geoghegan
On Wed, Jan 4, 2023 at 10:59 PM Amit Kapila 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

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

2023-01-04 Thread Amit Kapila
On Wed, Jan 4, 2023 at 11:30 PM Peter Geoghegan wrote: > > On Wed, Jan 4, 2023 at 7:03 AM Robert Haas 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

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

2023-01-04 Thread Peter Geoghegan
On Wed, Jan 4, 2023 at 10:41 AM Andres Freund 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. > >

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

2023-01-04 Thread Andres Freund
Hi, On 2023-01-04 09:59:37 -0800, Peter Geoghegan wrote: > On Wed, Jan 4, 2023 at 7:03 AM Robert Haas 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.

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

2023-01-04 Thread Peter Geoghegan
On Wed, Jan 4, 2023 at 7:03 AM Robert Haas 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,

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

2023-01-04 Thread Robert Haas
On Wed, Jan 4, 2023 at 1:53 AM Peter Geoghegan 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

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

2023-01-04 Thread Peter Geoghegan
On Tue, Jan 3, 2023 at 4:54 PM Andres Freund 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 >

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

2023-01-04 Thread Andres Freund
Hi, On 2023-01-03 19:23:41 +, 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.

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

2023-01-03 Thread Peter Geoghegan
On Tue, Jan 3, 2023 at 10:47 PM Andres Freund 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.

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

2023-01-03 Thread Andres Freund
Hi, On 2023-01-03 22:41:35 -0800, Peter Geoghegan wrote: > On Tue, Jan 3, 2023 at 10:33 PM Andres Freund 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

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

2023-01-03 Thread Peter Geoghegan
On Tue, Jan 3, 2023 at 10:33 PM Andres Freund 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

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

2023-01-03 Thread Andres Freund
Hi, On 2023-01-03 20:29:53 -0800, Peter Geoghegan wrote: > On Tue, Jan 3, 2023 at 7:56 PM Andres Freund 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

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

2023-01-03 Thread Peter Geoghegan
On Tue, Jan 3, 2023 at 8:29 PM Peter Geoghegan 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

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

2023-01-03 Thread Peter Geoghegan
On Tue, Jan 3, 2023 at 7:56 PM Andres Freund 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 >

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

2023-01-03 Thread Andres Freund
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 wrote:

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

2023-01-03 Thread Peter Geoghegan
(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 wrote: > > On Tue, Jan 3, 2023 at 4:54 PM Andres Freund wrote: > > There's some changes from TransactionIdDidCommit() to > >