Re: POC: Cleaning up orphaned files using undo logs

On Thu, Aug 8, 2019 at 7:01 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2019-08-07 14:50:17 +0530, Amit Kapila wrote:
> > On Tue, Aug 6, 2019 at 1:26 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > On 2019-08-05 11:29:34 -0700, Andres Freund wrote:
>
> > > > typedef struct TwoPhaseFileHeader
> > > > {
> > > > @@ -927,6 +928,16 @@ typedef struct TwoPhaseFileHeader
> > > > uint16 gidlen; /* length of the GID - GID follows the header */
> > > > XLogRecPtr origin_lsn; /* lsn of this record at origin node */
> > > > TimestampTz origin_timestamp; /* time of prepare at origin node */
> > > > +
> > > > + /*
> > > > + * We need the locations of the start and end undo record pointers when
> > > > + * rollbacks are to be performed for prepared transactions using undo-based
> > > > + * relations. We need to store this information in the file as the user
> > > > + * might rollback the prepared transaction after recovery and for that we
> > > > + * need its start and end undo locations.
> > > > + */
> > > > + UndoRecPtr start_urec_ptr[UndoLogCategories];
> > > > + UndoRecPtr end_urec_ptr[UndoLogCategories];
> > > > } TwoPhaseFileHeader;
> > >
> > > Why do we not need that knowledge for undo processing of a non-prepared
> > > transaction?
>
> > The non-prepared transaction also needs to be aware of that. It is
> > stored in TransactionStateData. I am not sure if I understand your
> > question here.
>
> My concern is that I think it's fairly ugly to store data like this in
> the 2pc state file. And it's not an insubstantial amount of additional
> data either, compared to the current size, even when no undo is in
> use. There's a difference between an unused feature increasing backend
> local memory and increasing the size of WAL logged data. Obviously it's
> not by a huge amount, but still. It also just feels wrong to me.
>
> We don't need the UndoRecPtr's when recovering from a crash/restart to
> process undo. Now we obviously don't want to unnecessarily search for
> data that is expensive to gather, which is a good reason for keeping
> track of this data. But I do wonder if this is the right approach.
>
> I know that Robert is working on a patch that revises the undo request
> layer somewhat, it's possible that this is best discussed afterwards.
>

Okay, we have started working on integrating with Robert's patch. I
think not only this but many of the other things will also change.
So, I will respond to other comments after integrating with Robert's
patch.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com