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

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

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

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

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

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

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

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

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

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