On Thu, Jul 4, 2019 at 5:23 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Jul 1, 2019 at 1:24 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> >
> > Another small change/review: the function UndoLogGetNextInsertPtr()
> > previously took a transaction ID, but I'm not sure if that made sense,
> > I need to think about it some more.
> >
>
> The changes you have made related to UndoLogGetNextInsertPtr() doesn't
> seem correct to me.
>
> @@ -854,7 +854,9 @@ FindUndoEndLocationAndSize(UndoRecPtr start_urecptr,
> * has already started in this log then lets re-fetch the undo
> * record.
> */
> - next_insert = UndoLogGetNextInsertPtr(slot->logno, uur->uur_xid);
> + next_insert = UndoLogGetNextInsertPtr(slot->logno);
> +
> + /* TODO this can't happen */
> if (!UndoRecPtrIsValid(next_insert))
>
> I think this is a possible case. Say while the discard worker tries
> to register the rollback request from some log and after it fetches
> the undo record corresponding to start location in this function,
> another backend adds the new transaction undo. The same is mentioned
> in comments as well. Can you explain what makes you think that this
> can't happen? If we don't want to pass the xid to
> UndoLogGetNextInsertPtr, then I think we need to get the insert
> location before fetching the record. I will think more on it to see
> if there is any other problem with the same.
>
Pushed the fixed on above lines in the undoprocessing branch. It will
be available in the next set of patches we post.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
