Re: Reorderbuffer crash during recovery

On Mon, Dec 30, 2019 at 11:17 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Fri, Dec 27, 2019 at 8:37 PM Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> >
> > On 2019-Dec-27, vignesh C wrote:
> >
> > > I felt amit solution also solves the problem. Attached patch has the
> > > fix based on the solution proposed.
> > > Thoughts?
> >
> > This seems a sensible fix to me, though I didn't try to reproduce the
> > failure.
> >
> > > @@ -2472,6 +2457,7 @@ ReorderBufferSerializeTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
> > > }
> > >
> > > ReorderBufferSerializeChange(rb, txn, fd, change);
> > > + txn->final_lsn = change->lsn;
> > > dlist_delete(&change->node);
> > > ReorderBufferReturnChange(rb, change);
> >
> > Should this be done insider ReorderBufferSerializeChange itself, instead
> > of in its caller?
> >
>
> makes sense. But, I think we should add a comment specifying the
> reason why it is important to set final_lsn while serializing the
> change.

Fixed

> > Also, would it be sane to verify that the TXN
> > doesn't already have a newer final_lsn? Maybe as an Assert.
> >
>
> I don't think this is a good idea because we update the final_lsn with
> commit_lsn in ReorderBufferCommit after which we can try to serialize
> the remaining changes. Instead, we should update it only if the
> change_lsn value is greater than final_lsn.
>

Fixed.
Thanks Alvaro & Amit for your suggestions. I have made the changes
based on your suggestions. Please find the updated patch for the same.
I have also verified the patch in back branches. Separate patch was
required for Release-10 branch, patch for the same is attached as
0001-Reorder-buffer-crash-while-aborting-old-transactions-REL_10.patch.
Thoughts?

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com