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
diff --git a/src/backend/access/rmgrdesc/xactdesc.c b/src/backend/access/rmgrdesc/xactdesc.c
index 3b3c95f810..2e4ea62e62 100644
--- a/src/backend/access/rmgrdesc/xactdesc.c
+++ b/src/backend/access/rmgrdesc/xactdesc.c
@@ -105,10 +105,8 @@ ParseCommitRecord(uint8 info, xl_xact_commit *xlrec, xl_xact_parsed_commit *pars
if (parsed->xinfo & XACT_XINFO_HAS_GID)
{
- int gidlen;
strlcpy(parsed->twophase_gid, data, sizeof(parsed->twophase_gid));
- gidlen = strlen(data) + 1;
- data += MAXALIGN(gidlen);
+ data += strlen(data) + 1;
}
}
@@ -116,7 +114,7 @@ ParseCommitRecord(uint8 info, xl_xact_commit *xlrec, xl_xact_parsed_commit *pars
{
xl_xact_origin xl_origin;
- /* we're only guaranteed 4 byte alignment, so copy onto stack */
+ /* we're not guaranteed any alignment, so copy onto stack */
memcpy(&xl_origin, data, sizeof(xl_origin));
parsed->origin_lsn = xl_origin.origin_lsn;
@@ -189,10 +187,8 @@ ParseAbortRecord(uint8 info, xl_xact_abort *xlrec, xl_xact_parsed_abort *parsed)
if (parsed->xinfo & XACT_XINFO_HAS_GID)
{
- int gidlen;
strlcpy(parsed->twophase_gid, data, sizeof(parsed->twophase_gid));
- gidlen = strlen(data) + 1;
- data += MAXALIGN(gidlen);
+ data += strlen(data) + 1;
}
}
@@ -200,7 +196,7 @@ ParseAbortRecord(uint8 info, xl_xact_abort *xlrec, xl_xact_parsed_abort *parsed)
{
xl_xact_origin xl_origin;
- /* we're only guaranteed 4 byte alignment, so copy onto stack */
+ /* we're not guaranteed any alignment, so copy onto stack */
memcpy(&xl_origin, data, sizeof(xl_origin));
parsed->origin_lsn = xl_origin.origin_lsn;
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index d6e4b7980f..5c05d545c4 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1129,9 +1129,11 @@ EndPrepare(GlobalTransaction gxact)
gxact->prepare_end_lsn = XLogInsert(RM_XACT_ID, XLOG_XACT_PREPARE);
if (replorigin)
+ {
/* Move LSNs forward for this replication origin */
replorigin_session_advance(replorigin_session_origin_lsn,
gxact->prepare_end_lsn);
+ }
XLogFlush(gxact->prepare_end_lsn);
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index b88d4ccf74..1dcf825625 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -5248,7 +5248,6 @@ XactLogCommitRecord(TimestampTz commit_time,
xl_xact_origin xl_origin;
uint8 info;
- int gidlen = 0;
Assert(CritSectionCount > 0);
@@ -5314,10 +5313,7 @@ XactLogCommitRecord(TimestampTz commit_time,
Assert(twophase_gid != NULL);
if (XLogLogicalInfoActive())
- {
xl_xinfo.xinfo |= XACT_XINFO_HAS_GID;
- gidlen = strlen(twophase_gid) + 1; /* include '\0' */
- }
}
/* dump transaction origin information */
@@ -5371,12 +5367,7 @@ XactLogCommitRecord(TimestampTz commit_time,
{
XLogRegisterData((char *) (&xl_twophase), sizeof(xl_xact_twophase));
if (xl_xinfo.xinfo & XACT_XINFO_HAS_GID)
- {
- static const char zeroes[MAXIMUM_ALIGNOF] = { 0 };
- XLogRegisterData((char*) twophase_gid, gidlen);
- if (MAXALIGN(gidlen) != gidlen)
- XLogRegisterData((char*) zeroes, MAXALIGN(gidlen) - gidlen);
- }
+ XLogRegisterData((char *) twophase_gid, strlen(twophase_gid));
}
if (xl_xinfo.xinfo & XACT_XINFO_HAS_ORIGIN)
@@ -5410,7 +5401,6 @@ XactLogAbortRecord(TimestampTz abort_time,
xl_xact_origin xl_origin;
uint8 info;
- int gidlen = 0;
Assert(CritSectionCount > 0);
@@ -5449,10 +5439,7 @@ XactLogAbortRecord(TimestampTz abort_time,
Assert(twophase_gid != NULL);
if (XLogLogicalInfoActive())
- {
xl_xinfo.xinfo |= XACT_XINFO_HAS_GID;
- gidlen = strlen(twophase_gid) + 1; /* include '\0' */
- }
}
if (TransactionIdIsValid(twophase_xid) && XLogLogicalInfoActive())
@@ -5488,7 +5475,6 @@ XactLogAbortRecord(TimestampTz abort_time,
if (xl_xinfo.xinfo & XACT_XINFO_HAS_DBINFO)
XLogRegisterData((char *) (&xl_dbinfo), sizeof(xl_dbinfo));
-
if (xl_xinfo.xinfo & XACT_XINFO_HAS_SUBXACTS)
{
XLogRegisterData((char *) (&xl_subxacts),
@@ -5509,14 +5495,14 @@ XactLogAbortRecord(TimestampTz abort_time,
{
XLogRegisterData((char *) (&xl_twophase), sizeof(xl_xact_twophase));
if (xl_xinfo.xinfo & XACT_XINFO_HAS_GID)
- {
- static const char zeroes[MAXIMUM_ALIGNOF] = { 0 };
- XLogRegisterData((char*) twophase_gid, gidlen);
- if (MAXALIGN(gidlen) != gidlen)
- XLogRegisterData((char*) zeroes, MAXALIGN(gidlen) - gidlen);
- }
+ XLogRegisterData((char *) twophase_gid, strlen(twophase_gid) + 1);
}
+ /*
+ * NOTE: twophase_gid is variable-length, so no alignment is guaranteed
+ * after this point.
+ */
+
if (xl_xinfo.xinfo & XACT_XINFO_HAS_ORIGIN)
XLogRegisterData((char *) (&xl_origin), sizeof(xl_xact_origin));
diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index a46396f2d9..3661b8d090 100644
--- a/src/include/access/xact.h
+++ b/src/include/access/xact.h
@@ -269,6 +269,7 @@ typedef struct xl_xact_commit
/* xl_xact_relfilenodes follows if XINFO_HAS_RELFILENODES */
/* xl_xact_invals follows if XINFO_HAS_INVALS */
/* xl_xact_twophase follows if XINFO_HAS_TWOPHASE */
+ /* twophase_gid follows if XINFO_HAS_GID. As a null-terminated string. */
/* xl_xact_origin follows if XINFO_HAS_ORIGIN, stored unaligned! */
} xl_xact_commit;
#define MinSizeOfXactCommit (offsetof(xl_xact_commit, xact_time) + sizeof(TimestampTz))
@@ -278,11 +279,13 @@ typedef struct xl_xact_abort
TimestampTz xact_time; /* time of abort */
/* xl_xact_xinfo follows if XLOG_XACT_HAS_INFO */
- /* No db_info required */
+ /* xl_xact_dbinfo follows if XINFO_HAS_DBINFO */
/* xl_xact_subxacts follows if HAS_SUBXACT */
/* xl_xact_relfilenodes follows if HAS_RELFILENODES */
/* No invalidation messages needed. */
/* xl_xact_twophase follows if XINFO_HAS_TWOPHASE */
+ /* twophase_gid follows if XINFO_HAS_GID. As a null-terminated string. */
+ /* xl_xact_origin follows if XINFO_HAS_ORIGIN, stored unaligned! */
} xl_xact_abort;
#define MinSizeOfXactAbort sizeof(xl_xact_abort)
