From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: masao.fujii@oss.nttdata.com
Cc: andres@anarazel.de
Cc: pgsql-hackers@lists.postgresql.org
Cc: bossartn@amazon.com
Cc: mengjuan.cmj@alibaba-inc.com
Cc: Jakub.Wartak@tomtom.com
Subject: Re: prevent immature WAL streaming
Date: Fri, 3 Sep 2021 09:59:29 -0400
Message-ID: <202109031359.by2swmsx6ha2@alvherre.pgsql> (raw)
In-Reply-To: <20210903.160904.904317102157226316.horikyota.ntt@gmail.com>
On 2021-Sep-03, Kyotaro Horiguchi wrote:
> At Thu, 2 Sep 2021 18:43:33 -0400, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote in
> 0002:
> > + XLogRecPtr abortedContrecordPtr; /* LSN of incomplete record at end of
> > + * WAL */
>
> The name sounds like the start LSN. doesn't contrecordAbort(ed)Ptr work?
I went over various iterations of the name of this, and still not
entirely happy. I think we need to convey the ideas that
* This is the endptr+1 of the known-good part of the record, that is,
the beginning of the next part of the record. I think "endPtr"
summarizes this well; we use this name elsewhere.
* At some point before recovery, this was the last WAL record that
existed
* there is an invalid contrecord, or we were looking for a contrecord
and found invalid data
* this record is incomplete
So maybe
1. incompleteRecEndPtr
2. finalInvalidRecEndPtr
3. brokenContrecordEndPtr
> > if (!(pageHeader->xlp_info & XLP_FIRST_IS_CONTRECORD))
> > {
> > report_invalid_record(state,
> > "there is no contrecord flag at %X/%X",
> > LSN_FORMAT_ARGS(RecPtr));
> > - goto err;
> > + goto aborted_contrecord;
>
> This loses the exclusion check between XLP_FIRST_IS_CONTRECORD and
> _IS_ABROTED_PARTIAL. Is it okay? (I don't object to remove the check.).
Yeah, I was unsure about that. I think it's good to have it as a
cross-check, though it should never occur. I'll put it back.
Another related point is whether it's a good idea to have the ereport()
about the bit appearing in a not-start-of-page address being a PANIC.
If we degrade to WARNING then it'll be lost in the noise, but I'm not
sure what else can we do. (If it's a PANIC, then you end up with an
unusable database).
> I didin't thought this as an aborted contrecord. but on second
> thought, when we see a record broken in any style, we stop recovery at
> the point. I agree to the change and all the silmiar changes.
>
> + /* XXX should we goto aborted_contrecord here? */
>
> I think it should be aborted_contrecord.
>
> When that happens, the loaded bytes actually looked like the first
> fragment of a continuation record to xlogreader, even if the cause
> were a broken total_len. So if we abort the record there, the next
> time xlogreader will meet XLP_FIRST_IS_ABORTED_PARTIAL at the same
> page, and correctly finds a new record there.
>
> On the other hand if we just errored-out there, we will step-back to
> the beginning of the broken record in the previous page or segment
> then start writing a new record there but that is exactly what we want
> to avoid now.
Hmm, yeah, makes sense.
--
Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
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: pgsql-hackers@postgresql.org
Cc: alvherre@alvh.no-ip.org, horikyota.ntt@gmail.com, masao.fujii@oss.nttdata.com, andres@anarazel.de, pgsql-hackers@lists.postgresql.org, bossartn@amazon.com, mengjuan.cmj@alibaba-inc.com, Jakub.Wartak@tomtom.com
Subject: Re: prevent immature WAL streaming
In-Reply-To: <202109031359.by2swmsx6ha2@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox