On Fri, Jun 28, 2024 at 12:55 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Fri, Jun 28, 2024 at 4:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Jun 28, 2024 at 5:15 AM Peter Smith <smithpb2250@gmail.com> wrote:
> > >
> > > On Thu, Jun 27, 2024 at 3:44 PM Michael Paquier <michael@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 <smithpb2250@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?
>
I don't know whether your assumption is correct. AFAICS, those two
lines should be together. Let us ee if Bertrand remembers anything?
--
With Regards,
Amit Kapila.
