Thread: Re: pgsql: Store 2PC GID in commit/abort WAL recs for logicaldecoding
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
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
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
Attachment
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
Attachment
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