Thread: Re: pgsql: Store 2PC GID in commit/abort WAL recs for logicaldecoding

Re: pgsql: Store 2PC GID in commit/abort WAL recs for logicaldecoding

From
Heikki Linnakangas
Date:
On 28/03/18 19:47, Simon Riggs wrote:
> Store 2PC GID in commit/abort WAL recs for logical decoding

This forgot to update the comments in xl_xact_commit and xl_xact_abort, 
for the new fields.

> +
> +               if (parsed->xinfo & XACT_XINFO_HAS_GID)
> +               {
> +                       int gidlen;
> +                       strcpy(parsed->twophase_gid, data);
> +                       gidlen = strlen(parsed->twophase_gid) + 1;
> +                       data += MAXALIGN(gidlen);
> +               }
> +       }
> +
> +       if (parsed->xinfo & XACT_XINFO_HAS_ORIGIN)
> +       {
> +               xl_xact_origin xl_origin;
> +
> +               /* we're only guaranteed 4 byte alignment, so copy onto stack */
> +               memcpy(&xl_origin, data, sizeof(xl_origin));
> +
> +               parsed->origin_lsn = xl_origin.origin_lsn;
> +               parsed->origin_timestamp = xl_origin.origin_timestamp;
> +
> +               data += sizeof(xl_xact_origin);
>         }

There seems to be some confusion on the padding here. Firstly, what's 
the point of zero-padding the GID length to the next MAXALIGN boundary, 
which would be 8 bytes on 64-bit systems, if the start is only 
guaranteed 4-byte alignment, per the comment at the memcpy() above. 
Secondly, if we're memcpying the fields that follow anyway, why bother 
with any alignment padding at all?

I propose the attached.

- Heikki

Attachment

Re: pgsql: Store 2PC GID in commit/abort WAL recs for logicaldecoding

From
Michael Paquier
Date:
On Mon, Apr 09, 2018 at 03:30:39PM +0300, Heikki Linnakangas wrote:
> There seems to be some confusion on the padding here. Firstly, what's the
> point of zero-padding the GID length to the next MAXALIGN boundary, which
> would be 8 bytes on 64-bit systems, if the start is only guaranteed 4-byte
> alignment, per the comment at the memcpy() above. Secondly, if we're
> memcpying the fields that follow anyway, why bother with any alignment
> padding at all?

It seems to me that you are right here: those complications are not
necessary.

     if (replorigin)
+    {
         /* Move LSNs forward for this replication origin */
         replorigin_session_advance(replorigin_session_origin_lsn,
                                    gxact->prepare_end_lsn);
+    }
This is better style.

+   /* twophase_gid follows if XINFO_HAS_GID. As a null-terminated string. */
+   /* xl_xact_origin follows if XINFO_HAS_ORIGIN, stored unaligned! */

Worth mentioning that the first one is also unaligned with your patch?
And that all the last fields of xl_xact_commit and xl_xact_abort are
kept as such on purpose?
--
Michael

Attachment

Re: pgsql: Store 2PC GID in commit/abort WAL recs for logicaldecoding

From
Heikki Linnakangas
Date:
On 10/04/18 03:24, Michael Paquier wrote:
> +   /* twophase_gid follows if XINFO_HAS_GID. As a null-terminated string. */
> +   /* xl_xact_origin follows if XINFO_HAS_ORIGIN, stored unaligned! */
> 
> Worth mentioning that the first one is also unaligned with your patch?

Hmm. 'twophase_gid' is actually 4-byte aligned here. But it's a string, 
so it doesn't matter whether it it is or not.

> And that all the last fields of xl_xact_commit and xl_xact_abort are
> kept as such on purpose?

I think that's clear without an explicit comment. If it wasn't on 
purpose, we wouldn't have a comment pointing it out (or we would fix it 
so that it was aligned).

Pushed, thanks for the review!

- Heikki


Re: pgsql: Store 2PC GID in commit/abort WAL recs for logical decoding

From
Nikhil Sontakke
Date:
Hi Heikki,


>
> Pushed, thanks for the review!
>

There was a slight oversight in the twophase_gid length calculation in
the XactLogCommitRecord() code path in the cf5a1890592 commit. The
corresponding XactLogAbortRecord() code path was ok. PFA, a small
patch to fix the oversight.

Regards,
Nikhils
-- 
 Nikhil Sontakke                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Attachment

Re: pgsql: Store 2PC GID in commit/abort WAL recs for logicaldecoding

From
Alvaro Herrera
Date:
On 2018-Jun-14, Nikhil Sontakke wrote:

> There was a slight oversight in the twophase_gid length calculation in
> the XactLogCommitRecord() code path in the cf5a1890592 commit. The
> corresponding XactLogAbortRecord() code path was ok. PFA, a small
> patch to fix the oversight.

Thanks, pushed.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Re: pgsql: Store 2PC GID in commit/abort WAL recs for logicaldecoding

From
Alvaro Herrera
Date:
On 2018-Jun-14, Nikhil Sontakke wrote:

> There was a slight oversight in the twophase_gid length calculation in
> the XactLogCommitRecord() code path in the cf5a1890592 commit. The
> corresponding XactLogAbortRecord() code path was ok. PFA, a small
> patch to fix the oversight.

Forgot to add: maybe it would be useful to have tests in core where
these omissions become evident.  Do you have some?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Re: pgsql: Store 2PC GID in commit/abort WAL recs for logicaldecoding

From
Alvaro Herrera
Date:
By the way, why do we need to strlen() the target buffer when strlcpy
already reports the length?  (You could argue that there is a difference
if the string is truncated ... but surely we don't care about that case)
I propose the attached.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachment

Re: pgsql: Store 2PC GID in commit/abort WAL recs for logical decoding

From
Nikhil Sontakke
Date:
Hi Alvaro,


>> There was a slight oversight in the twophase_gid length calculation in
>> the XactLogCommitRecord() code path in the cf5a1890592 commit. The
>> corresponding XactLogAbortRecord() code path was ok. PFA, a small
>> patch to fix the oversight.
>
> Forgot to add: maybe it would be useful to have tests in core where
> these omissions become evident.  Do you have some?
>
Thanks for the commit.

I do have some tests. They are part of the "logical decoding of 2PC"
patch which adds the needed infrastructure to *actually* use this code
present in the core as of now. I am going to submit it in the upcoming
commitfest.

Regards,
Nikhils
-- 
 Nikhil Sontakke                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


Re: pgsql: Store 2PC GID in commit/abort WAL recs for logicaldecoding

From
Alvaro Herrera
Date:
On 2018-Jun-15, Alvaro Herrera wrote:

> By the way, why do we need to strlen() the target buffer when strlcpy
> already reports the length?  (You could argue that there is a difference
> if the string is truncated ... but surely we don't care about that case)
> I propose the attached.

I decided not to push this after all.  Yes, one strlen is saved, but
there is some code clarity lost also, and this is certainly not a
contention point.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services