On Wed, Jul 3, 2019 at 5:21 PM Fujii Masao <masao.fujii@gmail.com> wrote:
>
> On Tue, Jul 2, 2019 at 7:16 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> >
> > On Fri, Apr 26, 2019 at 5:38 AM Michael Paquier <michael@paquier.xyz> wrote:
> > >
> > > On Thu, Apr 25, 2019 at 03:08:36PM -0400, Alvaro Herrera wrote:
> > > > On 2019-Apr-26, Fujii Masao wrote:
> > > >> I did that arrangement because the format of PREPARE TRANSACTION record,
> > > >> i.e., that struct, also needs to be accessed in backend and frontend.
> > > >> But, of course, if there is smarter way, I'm happy to adopt that!
> > > >
> > > > I don't know. I spent some time staring at the code involved, and it
> > > > seems it'd be possible to improve just a little bit on cleanliness
> > > > grounds, with a lot of effort, but not enough practical value.
> > >
> > > Describing those records is something we should do. There are other
> > > parsing routines in xactdesc.c for commit and abort records, so having
> > > that extra routine for prepare at the same place does not sound
> > > strange to me.
> > >
> > > +typedef xl_xact_prepare TwoPhaseFileHeader;
> > > I find this mapping implementation a bit lazy, and your
> > > newly-introduced xl_xact_prepare does not count for all the contents
> > > of the actual WAL record for PREPARE TRANSACTION. Wouldn't it be
> > > better to put all the contents of the record in the same structure,
> > > and not only the 2PC header information?
> >
> > This patch doesn't apply anymore, could you send a rebase?
>
> Yes, attached is the updated version of the patch.
Thanks!
So the patch compiles and works as intended. I don't have much to say
about it, it all looks good to me, since the concerns about xactdesc.c
aren't worth the trouble.
I'm not sure that I understand Michael's objection though, as
xl_xact_prepare is not a new definition and AFAICS it couldn't contain
the records anyway. So I'll let him say if he has further objections
or if it's ready for committer!
