public inbox for [email protected]
help / color / mirror / Atom feedFrom: Alexander Korotkov <[email protected]>
To: Robert Haas <[email protected]>
Cc: Nathan Bossart <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Fujii Masao <[email protected]>
Cc: Postgres hackers <[email protected]>
Subject: Re: Assertion failure with summarize_wal enabled during pg_createsubscriber
Date: Wed, 31 Jul 2024 16:49:54 +0300
Message-ID: <CAPpHfdtT=xAwEQL-fgRLz7vGXABFnnFReienNBJmfPwRZP8-NA@mail.gmail.com> (raw)
In-Reply-To: <CA+TgmoZrUjqA0dnLYQ6sunVmBKBAm0OLP49hKvZOuzq7v-0iKg@mail.gmail.com>
References: <[email protected]>
<[email protected]>
<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>
On Mon, Jul 29, 2024 at 7:20 PM Robert Haas <[email protected]> wrote:
> On Fri, Jul 26, 2024 at 4:10 PM Alexander Korotkov <[email protected]> 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 made
> > > it difficult to reuse properly, so this code refactors things to make
> > > 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 new
> > > timeline until we actually started writing WAL there, but that meant
> > > 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
view thread (3+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Assertion failure with summarize_wal enabled during pg_createsubscriber
In-Reply-To: <CAPpHfdtT=xAwEQL-fgRLz7vGXABFnnFReienNBJmfPwRZP8-NA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox