On Fri, Jun 28, 2024 at 4:18 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Fri, Jun 28, 2024 at 5:15 AM Peter Smith <smithpb2...@gmail.com> wrote: > > > > On Thu, Jun 27, 2024 at 3:44 PM Michael Paquier <mich...@paquier.xyz> wrote: > > > > > > On Wed, Jun 26, 2024 at 02:30:26PM +0530, vignesh C wrote: > > > > On Mon, 3 Jun 2024 at 11:21, Peter Smith <smithpb2...@gmail.com> wrote: > > > >> Perhaps the comment should say something like it used to: > > > >> /* Fail if there is not enough WAL available. This can happen during > > > >> shutdown. */ > > > > > > > > Agree with this, +1 for this change. > > > > > > That would be an improvement. Would you like to send a patch with all > > > the areas you think could stand for improvements? > > > -- > > > > OK, I attached a patch equivalent of the suggestion in this thread. > > > > Shouldn't the check for flushptr (if (flushptr < targetPagePtr + > reqLen)) be moved immediately after the call to WalSndWaitForWal(). > The comment seems to suggests the same: "Make sure we have enough WAL > available before retrieving the current timeline. .." >
Yes, as I wrote in the first post, those lines did once used to be adjacent in logical_read_xlog_page. I also wondered if they still belonged together, but I opted for the safest option of fixing only the comment instead of refactoring old code when no problem had been reported. AFAICT these lines became separated due to a 2023 patch [1], and you were one of the reviewers of that patch, so I assumed the code separation was deemed OK at that time. Unless it was some mistake that slipped past multiple reviewers? ====== [1] https://github.com/postgres/postgres/commit/0fdab27ad68a059a1663fa5ce48d76333f1bd74c#diff-da7052ce339ec037f5c721e08a9532b1fcfb4405966cf6a78d0c811550fd7b43 Kind Regards, Peter Smith. Fujitsu Australia
