MIME-Version: 1.0
References: <2a6fe324-3008-4e44-9ff6-b5bba9232490@oss.nttdata.com>
 <ZoJHsmnCyB7Pty1C@paquier.xyz> <CA+TgmobLaJTxCHgdh04rfsUMEhP_ceDbiF0M=gtw5jG4q_zPbg@mail.gmail.com>
 <CA+TgmoY9QC_CUA0F8febGj_5AgnCk8rG7AkX4rhJPvvHeshYtQ@mail.gmail.com>
 <CA+TgmoYW8eR+KN6zhVH0sin7QH6AvENqw_bkN-bB4yLYKAnsew@mail.gmail.com>
 <CA+TgmoZGEsZodXC4f=XZNkAeyuDmWTSkpkjCEOcF19Am0mt_OA@mail.gmail.com>
 <CA+TgmoYry+iPM3DFpD2jy=4afvXm09qPUFiBS6EAY92VzCR8Pg@mail.gmail.com>
 <ZqPGDo2e2y4y_Eao@nathan> <CA+TgmoZz+mBX5F2cg7t2iKXbEakSDVE3rWxW1NRhBphXrO=7TQ@mail.gmail.com>
 <CAPpHfduW3du0W=3noztdaJ6evGP9gqT1AGk_rwXrqDyus1zZoQ@mail.gmail.com> <CA+TgmoZrUjqA0dnLYQ6sunVmBKBAm0OLP49hKvZOuzq7v-0iKg@mail.gmail.com>
In-Reply-To: <CA+TgmoZrUjqA0dnLYQ6sunVmBKBAm0OLP49hKvZOuzq7v-0iKg@mail.gmail.com>
From: Alexander Korotkov <aekorotkov@gmail.com>
Date: Wed, 31 Jul 2024 16:49:54 +0300
Message-ID: <CAPpHfdtT=xAwEQL-fgRLz7vGXABFnnFReienNBJmfPwRZP8-NA@mail.gmail.com>
Subject: Re: Assertion failure with summarize_wal enabled during pg_createsubscriber
To: Robert Haas <robertmhaas@gmail.com>
Cc: Nathan Bossart <nathandbossart@gmail.com>, Michael Paquier <michael@paquier.xyz>, 
	Fujii Masao <masao.fujii@oss.nttdata.com>, 
	Postgres hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CAPpHfdtT%3DxAwEQL-fgRLz7vGXABFnnFReienNBJmfPwRZP8-NA%40mail.gmail.com>
Precedence: bulk

On Mon, Jul 29, 2024 at 7:20=E2=80=AFPM Robert Haas <robertmhaas@gmail.com>=
 wrote:
> On Fri, Jul 26, 2024 at 4:10=E2=80=AFPM Alexander Korotkov <aekorotkov@gm=
ail.com> wrote:
> > 0002 could also use pg_indent and pgperltidy.  And I have couple other
> > notes regarding 0002.
>
> As Tom said elsewhere, we don't have a practice of requiring perltidy
> for every commit, and I normally don't bother. The tree is
> pgindent-clean currently so I believe I got that part right.

Got it, thanks.

> > >    In the process of fixing these bugs, I realized that the logic to =
wait
> > >    for WAL summarization to catch up was spread out in a way that mad=
e
> > >    it difficult to reuse properly, so this code refactors things to m=
ake
> > >    it easier.
> >
> > It would be nice to split refactoring out of material logic changed.
> > This way it would be easier to review and would look cleaner in the
> > git history.
>
> I support that idea in general but felt it was overkill in this case:
> it's new code, and there was only one existing caller of the function
> that got refactored, and I'm not a huge fan of cluttering the git
> history with a bunch of tiny little refactoring commits to fix a
> single bug. I might have changed it if I'd seen this note before
> committing, though.

I understand your point.  I'm also not huge fan of a flood of small
commits.  Nevertheless, I find splitting refactoring from other
changes generally useful.  That could be a single commit of many small
refactorings, not many small commits.  The point for me is easier
review: you can expect refactoring commit to contain "isomorphic"
changes, while other commits implementing material logic changes.  But
that might be a committer preference though.

> > >    To make this fix work, also teach the WAL summarizer that after a
> > >    promotion has occurred, no more WAL can appear on the previous
> > >    timeline: previously, the WAL summarizer wouldn't switch to the ne=
w
> > >    timeline until we actually started writing WAL there, but that mea=
nt
> > >    that when the startup process was waiting for the WAL summarizer, =
it
> > >    was waiting for an action that the summarizer wasn't yet prepared =
to
> > >    take.
> >
> > I think this is a pretty long sentence, and I'm not sure I can
> > understand it correctly.  Does startup process wait for the WAL
> > summarizer without this patch?  If not, it's not clear for me that
> > word "previously" doesn't distribute to this part of sentence.
> > Breaking this into multiple sentences could improve the clarity for
> > me.
>
> Yes, I think that phrasing is muddled. It's too late to amend the
> commit message, but for posterity: I initially thought that I could
> fix this bug by just teaching the startup process to wait for WAL
> summarization before performing the .partial renaming, but that was
> not enough by itself. The problem is that the WAL summarizer would
> read all the records that were present in the final WAL file on the
> old timeline, but it wouldn't actually write out a summary file,
> because that only happens when it reaches XLOG_CHECKPOINT_REDO or a
> timeline switch point. Since no WAL had appeared on the new timeline
> yet, it didn't view the end of the old timeline as a switch point, so
> it just sat there waiting, without ever writing out a file; and the
> startup process sat there waiting for it. So the second part of the
> fix is to make the WAL summarizer realize that once the startup
> process has chosen a new timeline ID, no more WAL is going to appear
> on the old timeline, and thus it can (and should) write out the final
> summary file for the old timeline and prepare to read WAL from the new
> timeline.

Great, thank you for the explanation.  Now that's clear.

------
Regards,
Alexander Korotkov
Supabase