At Fri, 8 Nov 2019 09:53:07 +0900, Fujii Masao <masao.fujii@gmail.com> wrote in
> On Fri, Nov 8, 2019 at 9:41 AM Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Tue, Sep 03, 2019 at 10:00:08AM +0900, Fujii Masao wrote:
> > > Sorry for the long delay... Yes, I will update the patch if necessary.
> >
> > Fujii-san, are you planning to update this patch then? I have
> > switched it as waiting on author.
>
> No because there has been nothing to update in the latest patch for now
> unless I'm missing something. So I'm just waiting for some new review
> comments against the latest patch to come :)
> Can I switch the status back to "Needs review"?
On Fri, Apr 26, 2019 at 5:38 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> +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?
I agree to this in principle, but I'm afraid that we cannot do that
actually. Doing it straight way would result in something like this.
typedef struct xl_xact_prepare
{
uint32 magic;
...
TimestampTz origin_timestamp;
/* correct alignment here */
+ char gid[FLEXIBLE_ARRAY_MEMBER]; /* the GID of the prepred xact */
+ /* subxacts, xnodes, msgs and sentinel follow the gid[] array */
} xl_xact_prepare;
I don't think this is meaningful..
After all, the new xlog record struct is used only at one place.
xlog_redo is the correspondent of xact_desc, but it is not aware of
the stuct and PrepareRedoAdd decodes it using TwoPhaseFileHeader. In
that sense, the details of the record is a secret of twophase.c. What
is worse, apparently TwoPhaseFileHeader is a *subset* of
xl_xact_prepare but what we want to expose is the super set. Thus I
porpose to add a comment instead of exposing the full structure in
xl_xact_prepare definition.
typedef struct xl_xact_prepare
{
uint32 magic;
...
TimestampTz origin_timestamp;
+ /*
+ * This record has multiple trailing data sections with variable
+ * length. See twophase.c for the details.
+ */
} xl_xact_prepare;
Then, teach xlog_redo to resolve the record pointer to this type
before passing it to PrepareRedoAdd.
Does it make sense?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
