Fix confusion on the padding of GIDs in on commit and abort records.

Review of commit 1eb6d652: It's pointless to add padding to the GID fields,
when the code that follows assumes that there is no alignment, and uses
memcpy(). Remove the pointless padding.

Update comments to note the new fields in the WAL records.

Reviewed-by: Michael Paquier
Discussion: https://www.postgresql.org/message-id/33b787bf-dc20-1161-54e9-3f3b607bf59d%40iki.fi

diff --git a/src/backend/access/rmgrdesc/xactdesc.c b/src/backend/access/rmgrdesc/xactdesc.c
index 9b40f447ff183d8883977f7d4fdcca1afb0fb8c7..6d5ebd475b42d63686251b07c874745481ad7817 100644 (file)
--- a/src/backend/access/rmgrdesc/xactdesc.c
+++ b/src/backend/access/rmgrdesc/xactdesc.c
@@ -104,18 +104,18 @@ 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;
                }
        }
 
+       /* Note: no alignment is guaranteed after this point */
+
        if (parsed->xinfo & XACT_XINFO_HAS_ORIGIN)
        {
                xl_xact_origin xl_origin;
 
-               /* we're only guaranteed 4 byte alignment, so copy onto stack */
+               /* no alignment is guaranteed, so copy onto stack */
                memcpy(&xl_origin, data, sizeof(xl_origin));
 
                parsed->origin_lsn = xl_origin.origin_lsn;
@@ -188,18 +188,18 @@ 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;
                }
        }
 
+       /* Note: no alignment is guaranteed after this point */
+
        if (parsed->xinfo & XACT_XINFO_HAS_ORIGIN)
        {
                xl_xact_origin xl_origin;
 
-               /* we're only guaranteed 4 byte alignment, so copy onto stack */
+               /* no alignment is guaranteed, 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 d6e4b7980f3c08be0c193943cff546ee951a98af..5c05d545c46253fcc78ba42ea221ddaa7205690d 100644 (file)
--- 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 948733c1e3dd8d07896c22a1a20b1c0b4511ef82..4747353bb93a06994e6536b679cf272de074f266 100644 (file)
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -5245,9 +5245,7 @@ XactLogCommitRecord(TimestampTz commit_time,
        xl_xact_invals xl_invals;
        xl_xact_twophase xl_twophase;
        xl_xact_origin xl_origin;
-
        uint8           info;
-       int                     gidlen = 0;
 
        Assert(CritSectionCount > 0);
 
@@ -5313,10 +5311,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 */
@@ -5370,12 +5365,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)
@@ -5409,7 +5399,6 @@ XactLogAbortRecord(TimestampTz abort_time,
        xl_xact_origin xl_origin;
 
        uint8           info;
-       int                     gidlen = 0;
 
        Assert(CritSectionCount > 0);
 
@@ -5448,10 +5437,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())
@@ -5487,7 +5473,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),
@@ -5508,12 +5493,7 @@ 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);
        }
 
        if (xl_xinfo.xinfo & XACT_XINFO_HAS_ORIGIN)

diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index a46396f2d92f4e5d3f0ffc48dce3c1148d545bd9..3661b8d09040a706c39c52c49042fa205a7cd5f1 100644 (file)
--- 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)
 

diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h
index a5c074642f6891c957ba9bf955a8b8ac704f1365..7c766836dbaeb270da2ba3fcdc8b3bbb50704c0e 100644 (file)
--- a/src/include/access/xlog_internal.h
+++ b/src/include/access/xlog_internal.h
@@ -31,7 +31,7 @@
 /*
  * Each page of XLOG file has a header like this:
  */
-#define XLOG_PAGE_MAGIC 0xD097 /* can be used as WAL version indicator */
+#define XLOG_PAGE_MAGIC 0xD098 /* can be used as WAL version indicator */
 
 typedef struct XLogPageHeaderData
 {