Re: New WAL record to detect the checkpoint redo location

2023-10-19 Thread Robert Haas
On Thu, Oct 19, 2023 at 1:53 AM Michael Paquier wrote: > Seems fine to me. Thanks for considering the idea. I think it was a good idea! I've committed the patch. -- Robert Haas EDB: http://www.enterprisedb.com

Re: New WAL record to detect the checkpoint redo location

2023-10-18 Thread Michael Paquier
On Wed, Oct 18, 2023 at 10:24:50AM -0400, Robert Haas wrote: > I added a variant of this test case. Here's v10. +-- Verify that an XLOG_CHECKPOINT_REDO record begins at precisely the redo LSN +-- of the checkpoint we just performed. +SELECT redo_lsn FROM pg_control_checkpoint() \gset +SELECT

Re: New WAL record to detect the checkpoint redo location

2023-10-18 Thread Robert Haas
On Tue, Oct 17, 2023 at 8:35 PM Michael Paquier wrote: > Suggestion is from here, with a test for pg_walinspect after it runs > its online checkpoint (see the full-page case): > https://www.postgresql.org/message-id/ZOvf1tu6rfL/b...@paquier.xyz > > +-- Check presence of REDO record. > +SELECT

Re: New WAL record to detect the checkpoint redo location

2023-10-17 Thread Michael Paquier
On Tue, Oct 17, 2023 at 12:45:52PM -0400, Robert Haas wrote: > On Fri, Oct 13, 2023 at 3:29 AM Michael Paquier wrote: >> I've mentioned as well a test in pg_walinspect after one of the >> checkpoints generated there, but what you do here is enough for the >> online case. > > I don't quite

Re: New WAL record to detect the checkpoint redo location

2023-10-17 Thread Robert Haas
On Fri, Oct 13, 2023 at 3:29 AM Michael Paquier wrote: > Now looking at 0002, where you should be careful about the code > indentation or koel will complain. Fixed in the attached version. > This makes the new code call LocalSetXLogInsertAllowed() and what we > set for checkPoint.PrevTimeLineID

Re: New WAL record to detect the checkpoint redo location

2023-10-13 Thread Michael Paquier
On Tue, Oct 10, 2023 at 02:43:34PM -0400, Robert Haas wrote: > Here's a new patch set. I think I've incorporated the performance > fixes that you've suggested so far into this version. I also adjusted > a couple of other things: Now looking at 0002, where you should be careful about the code

Re: New WAL record to detect the checkpoint redo location

2023-10-12 Thread Robert Haas
On Thu, Oct 12, 2023 at 3:27 AM Michael Paquier wrote: > I have looked at 0001, for now.. And it looks OK to me. Cool. I've committed that one. Thanks for the review. -- Robert Haas EDB: http://www.enterprisedb.com

Re: New WAL record to detect the checkpoint redo location

2023-10-12 Thread Michael Paquier
On Tue, Oct 10, 2023 at 02:43:34PM -0400, Robert Haas wrote: > - I combined what were previously 0002 and 0003 into a single patch, > since that's how this would get committed. > > - I fixed up some comments. > > - I updated commit messages. > > Hopefully this is getting close to good enough.

Re: New WAL record to detect the checkpoint redo location

2023-10-11 Thread Thomas Munro
On Tue, Oct 10, 2023 at 11:33 AM Robert Haas wrote: > On Mon, Oct 9, 2023 at 4:47 PM Andres Freund wrote: > > I think we might be able to speed some of this up by pre-compute values so > > we > > can implement things like bytesleft / UsableBytesInPage with shifts. IIRC we > > already insist on

Re: New WAL record to detect the checkpoint redo location

2023-10-10 Thread Robert Haas
On Mon, Oct 9, 2023 at 4:47 PM Andres Freund wrote: > As noted in my email from a few minutes ago, I agree that optimizing this > shouldn't be a requirement for merging the patch. Here's a new patch set. I think I've incorporated the performance fixes that you've suggested so far into this

Re: New WAL record to detect the checkpoint redo location

2023-10-09 Thread Andres Freund
Hi, On 2023-10-09 18:31:11 -0400, Robert Haas wrote: > On Mon, Oct 9, 2023 at 4:47 PM Andres Freund wrote: > > I think we might be able to speed some of this up by pre-compute values so > > we > > can implement things like bytesleft / UsableBytesInPage with shifts. IIRC we > > already insist on

Re: New WAL record to detect the checkpoint redo location

2023-10-09 Thread Robert Haas
On Mon, Oct 9, 2023 at 4:47 PM Andres Freund wrote: > I think we might be able to speed some of this up by pre-compute values so we > can implement things like bytesleft / UsableBytesInPage with shifts. IIRC we > already insist on power-of-two segment sizes, so instead of needing to divide > by a

Re: New WAL record to detect the checkpoint redo location

2023-10-09 Thread Andres Freund
Hi, As noted in my email from a few minutes ago, I agree that optimizing this shouldn't be a requirement for merging the patch. On 2023-10-09 15:58:36 -0400, Robert Haas wrote: > 1. The reason why we're doing this multiplication and division is to > make sure that the code in

Re: New WAL record to detect the checkpoint redo location

2023-10-09 Thread Andres Freund
Hi, On 2023-10-06 13:44:55 -0400, Robert Haas wrote: > On Thu, Oct 5, 2023 at 2:34 PM Andres Freund wrote: > > If I add an unlikely around if (rechdr->xl_rmid == RM_XLOG_ID), the > > performance does improve. But that "only" brings it up to 322.406. Not sure > > what the rest is. > > I don't

Re: New WAL record to detect the checkpoint redo location

2023-10-09 Thread Robert Haas
On Thu, Oct 5, 2023 at 2:34 PM Andres Freund wrote: > One thing that's notable, but not related to the patch, is that we waste a > fair bit of cpu time below XLogInsertRecord() with divisions. I think they're > all due to the use of UsableBytesInSegment in >

Re: New WAL record to detect the checkpoint redo location

2023-10-06 Thread Robert Haas
On Thu, Oct 5, 2023 at 2:34 PM Andres Freund wrote: > If I add an unlikely around if (rechdr->xl_rmid == RM_XLOG_ID), the > performance does improve. But that "only" brings it up to 322.406. Not sure > what the rest is. I don't really think this is worth worrying about. A sub-one-percent

Re: New WAL record to detect the checkpoint redo location

2023-10-05 Thread Andres Freund
Hi, On 2023-10-02 10:42:37 -0400, Robert Haas wrote: > I was trying to think of a test case where XLogInsertRecord would be > exercised as heavily as possible, so I really wanted to generate a lot > of WAL while doing as little real work as possible. The best idea that > I had was to run

Re: New WAL record to detect the checkpoint redo location

2023-10-03 Thread Robert Haas
On Wed, Sep 20, 2023 at 4:20 PM Robert Haas wrote: > Here are some patches. Here are some updated patches. Following some off-list conversation with Andres, I restructured 0003 to put the common case first and use likely(), and I fixed the brown-paper-bag noted by Amit. I then turned my

Re: New WAL record to detect the checkpoint redo location

2023-09-22 Thread Amit Kapila
On Thu, Sep 21, 2023 at 9:06 PM Robert Haas wrote: > > On Thu, Sep 21, 2023 at 4:22 AM Amit Kapila wrote: > > After the 0003 patch, do we need acquire exclusive lock via > > WALInsertLockAcquireExclusive() for non-shutdown checkpoints. Even the > > comment "We must block concurrent insertions

Re: New WAL record to detect the checkpoint redo location

2023-09-21 Thread Dilip Kumar
On Thu, Sep 21, 2023 at 1:50 AM Robert Haas wrote: > > On Mon, Sep 18, 2023 at 2:57 PM Robert Haas wrote: > > I've been brainstorming about this today, trying to figure out some > > ideas to make it work. > > Here are some patches. > > 0001 refactors XLogInsertRecord to unify a couple of

Re: New WAL record to detect the checkpoint redo location

2023-09-21 Thread Robert Haas
On Thu, Sep 21, 2023 at 4:22 AM Amit Kapila wrote: > After the 0003 patch, do we need acquire exclusive lock via > WALInsertLockAcquireExclusive() for non-shutdown checkpoints. Even the > comment "We must block concurrent insertions while examining insert > state to determine the checkpoint REDO

Re: New WAL record to detect the checkpoint redo location

2023-09-21 Thread Amit Kapila
On Thu, Sep 21, 2023 at 7:05 AM Robert Haas wrote: > > On Mon, Sep 18, 2023 at 2:57 PM Robert Haas wrote: > > I've been brainstorming about this today, trying to figure out some > > ideas to make it work. > > Here are some patches. > > 0001 refactors XLogInsertRecord to unify a couple of

Re: New WAL record to detect the checkpoint redo location

2023-09-20 Thread Robert Haas
On Mon, Sep 18, 2023 at 2:57 PM Robert Haas wrote: > I've been brainstorming about this today, trying to figure out some > ideas to make it work. Here are some patches. 0001 refactors XLogInsertRecord to unify a couple of separate tests of isLogSwitch, hopefully making it cleaner and cheaper to

Re: New WAL record to detect the checkpoint redo location

2023-09-18 Thread Robert Haas
On Fri, Jul 14, 2023 at 11:16 AM Andres Freund wrote: > I suspect we might be able to get rid of the need for exclusive inserts > here. If we rid of that, we could determine the redoa location based on the > LSN determined by the XLogInsert(). I've been brainstorming about this today, trying to

Re: New WAL record to detect the checkpoint redo location

2023-08-30 Thread Michael Paquier
On Thu, Aug 31, 2023 at 09:55:45AM +0530, Dilip Kumar wrote: > Yeah, good catch. With this, it seems like we can not move this new > WAL Insert out of the Exclusive WAL insertion lock right? Because if > we want to set the LSN of this record as the checkpoint.redo then > there should not be any

Re: New WAL record to detect the checkpoint redo location

2023-08-30 Thread Dilip Kumar
On Thu, Aug 31, 2023 at 9:36 AM Michael Paquier wrote: > > On Wed, Aug 30, 2023 at 04:51:19PM +0530, Dilip Kumar wrote: > > Your suggestions LGTM so modified accordingly > > I have been putting my HEAD on this patch for a few hours, reviewing > the surroundings, and somewhat missed that this

Re: New WAL record to detect the checkpoint redo location

2023-08-30 Thread Michael Paquier
On Wed, Aug 30, 2023 at 04:51:19PM +0530, Dilip Kumar wrote: > Your suggestions LGTM so modified accordingly I have been putting my HEAD on this patch for a few hours, reviewing the surroundings, and somewhat missed that this computation is done while we do not hold the WAL insert locks: +

Re: New WAL record to detect the checkpoint redo location

2023-08-30 Thread Dilip Kumar
On Wed, Aug 30, 2023 at 1:03 PM Michael Paquier wrote: > > On Mon, Aug 28, 2023 at 01:47:18PM +0530, Dilip Kumar wrote: > > I removed this mainly because now in other comments[1] where we are > > introducing this new CHECKPOINT_REDO record we are explaining the > > problem > > that the redo

Re: New WAL record to detect the checkpoint redo location

2023-08-30 Thread Michael Paquier
On Mon, Aug 28, 2023 at 01:47:18PM +0530, Dilip Kumar wrote: > I removed this mainly because now in other comments[1] where we are > introducing this new CHECKPOINT_REDO record we are explaining the > problem > that the redo location and the actual checkpoint records are not at > the same place

Re: New WAL record to detect the checkpoint redo location

2023-08-28 Thread Dilip Kumar
On Mon, Aug 28, 2023 at 5:14 AM Michael Paquier wrote: > > On Fri, Aug 25, 2023 at 11:08:25AM +0530, Dilip Kumar wrote: > > Here is the updated version of the patch. > > The concept of the patch looks sound to me. I have a few comments. Thanks for the review > +* This special record,

Re: New WAL record to detect the checkpoint redo location

2023-08-27 Thread Michael Paquier
On Fri, Aug 25, 2023 at 11:08:25AM +0530, Dilip Kumar wrote: > Here is the updated version of the patch. The concept of the patch looks sound to me. I have a few comments. +* This special record, however, is not required when we doing a shutdown +* checkpoint, as there will be

Re: New WAL record to detect the checkpoint redo location

2023-08-24 Thread Dilip Kumar
On Fri, Aug 18, 2023 at 10:12 AM Dilip Kumar wrote: > > On Fri, Aug 18, 2023 at 5:24 AM Michael Paquier wrote: > > > > On Thu, Aug 17, 2023 at 01:11:50PM +0530, Dilip Kumar wrote: > > > Yeah right, actually I was confused, I assumed it will return the > > > start LSN of the record. And I do not

Re: New WAL record to detect the checkpoint redo location

2023-08-17 Thread Dilip Kumar
On Fri, Aug 18, 2023 at 5:24 AM Michael Paquier wrote: > > On Thu, Aug 17, 2023 at 01:11:50PM +0530, Dilip Kumar wrote: > > Yeah right, actually I was confused, I assumed it will return the > > start LSN of the record. And I do not see any easy way to identify > > the Start LSN of this record so

Re: New WAL record to detect the checkpoint redo location

2023-08-17 Thread Michael Paquier
On Thu, Aug 17, 2023 at 01:11:50PM +0530, Dilip Kumar wrote: > Yeah right, actually I was confused, I assumed it will return the > start LSN of the record. And I do not see any easy way to identify > the Start LSN of this record so maybe this solution will not work. I > will have to think of

Re: New WAL record to detect the checkpoint redo location

2023-08-17 Thread Dilip Kumar
On Thu, Aug 17, 2023 at 10:52 AM Michael Paquier wrote: > > On Tue, Aug 15, 2023 at 02:23:43PM +0530, Dilip Kumar wrote: > > Yeah, good idea, actually we can do this insert outside of the > > exclusive insert lock and set the LSN of this insert as the > > checkpoint. redo location. So now we do

Re: New WAL record to detect the checkpoint redo location

2023-08-16 Thread Michael Paquier
On Tue, Aug 15, 2023 at 02:23:43PM +0530, Dilip Kumar wrote: > Yeah, good idea, actually we can do this insert outside of the > exclusive insert lock and set the LSN of this insert as the > checkpoint. redo location. So now we do not need to compute the > checkpoint. redo based on the current

Re: New WAL record to detect the checkpoint redo location

2023-08-15 Thread Dilip Kumar
On Fri, Jul 14, 2023 at 8:46 PM Andres Freund wrote: > > Hi, > > As I think I mentioned before, I like this idea. However, I don't like the > implementation too much. Thanks for looking into it. > This might work right now, but doesn't really seem maintainable. Nor do I like > adding branches

Re: New WAL record to detect the checkpoint redo location

2023-07-14 Thread Andres Freund
Hi, As I think I mentioned before, I like this idea. However, I don't like the implementation too much. On 2023-06-15 13:11:57 +0530, Dilip Kumar wrote: > diff --git a/src/backend/access/transam/xlog.c > b/src/backend/access/transam/xlog.c > index b2430f617c..a025fb91e2 100644 > ---

New WAL record to detect the checkpoint redo location

2023-06-15 Thread Dilip Kumar
As discussed [1 ][2] currently, the checkpoint-redo LSN can not be accurately detected while processing the WAL. Although we have a checkpoint WAL record containing the exact redo LSN, other WAL records may be inserted between the checkpoint-redo LSN and the actual checkpoint record. If we want to