Re: prevent immature WAL streaming

At Tue, 14 Sep 2021 22:32:04 -0300, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote in
> On 2021-Sep-14, Alvaro Herrera wrote:
>
> > On 2021-Sep-08, Kyotaro Horiguchi wrote:
> >
> > > Thanks! As my understanding the new record add the ability to
> > > cross-check between a teard-off contrecord and the new record inserted
> > > after the teard-off record. I didn't test the version by myself but
> > > the previous version implemented the essential machinery and that
> > > won't change fundamentally by the new record.
> > >
> > > So I think the current patch deserves to see the algorithm actually
> > > works against the problem.
> >
> > Here's a version with the new record type. It passes check-world, and
> > it seems to work correctly to prevent overwrite of the tail end of a
> > segment containing a broken record. This is very much WIP still;
> > comments are missing and I haven't tried to implement any sort of
> > verification that the record being aborted is the right one.
>
> Here's the attachment I forgot earlier.

(I missed the chance to complain about that:p)

Tnaks for the patch!

- CopyXLogRecordToWAL(rechdr->xl_tot_len, isLogSwitch, rdata,
- StartPos, EndPos);
+ CopyXLogRecordToWAL(rechdr->xl_tot_len, isLogSwitch,
+ flags & XLOG_SET_ABORTED_PARTIAL,
+ rdata, StartPos, EndPos);

The new xlog flag XLOG_SET_ABORTED_PARTIAL is used only by
RM_XLOG_ID/XLOG_OVERWRITE_CONTRECORD records, so the flag value is the
equivalent of the record type. We might instead want a new flag
XLOG_SPECIAL_TREATED_RECORD or something to quickly distinguish
records that need a special treat like XLOG_SWITCH.

if (flags & XLOG_SPECIAL_TREATED_RECORD)
{
if (rechdr->xl_rmid == RM_XLOG_ID)
{
if (info == XLOG_SWITCH)
isLogSwitch = true;
if (info == XLOG_OVERWRITE_CONTRECORD)
isOverwrite = true;
}
}
..
CopyXLogRecrodToWAL(.., isLogSwitch, isOverwrite, rdata, StartPos, EndPos);

+ /* XXX can only happen once in the loop. Verify? */
+ if (set_aborted_partial)
+ pagehdr->xlp_info |= XLP_FIRST_IS_ABORTED_PARTIAL;
+

I'm not sure about the reason for the change from the previous patch
(I might be missing something), this sets the flag on the *next* page
of the page where the record starts. So in the first place we
shouldn't set the flag there. The page header flags of the first page
is set by AdvanceXLInsertBuffer. If we want to set the flag in the
function, we need to find the page header for the beginning of the
record and make sure that the record is placed at the beginning of the
page. (It is the reason that I did that in AdvanceXLInsertBuffer..)

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center