Skip to content

Commit cf5a189

Browse files
hlinnakahlinnaka
committed
Fix confusion on the padding of GIDs in on commit and abort records.
Review of commit 1eb6d65: 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
1 parent b7e2cbc commit cf5a189

File tree

5 files changed

+17
-32
lines changed

5 files changed

+17
-32
lines changed

‎src/backend/access/rmgrdesc/xactdesc.c

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -104,18 +104,18 @@ ParseCommitRecord(uint8 info, xl_xact_commit *xlrec, xl_xact_parsed_commit *pars
104104

105105
if (parsed->xinfo & XACT_XINFO_HAS_GID)
106106
{
107-
int gidlen;
108107
strlcpy(parsed->twophase_gid, data, sizeof(parsed->twophase_gid));
109-
gidlen = strlen(data) + 1;
110-
data += MAXALIGN(gidlen);
108+
data += strlen(data) + 1;
111109
}
112110
}
113111

112+
/* Note: no alignment is guaranteed after this point */
113+
114114
if (parsed->xinfo & XACT_XINFO_HAS_ORIGIN)
115115
{
116116
xl_xact_origin xl_origin;
117117

118-
/* we're only guaranteed 4 byte alignment, so copy onto stack */
118+
/* no alignment is guaranteed, so copy onto stack */
119119
memcpy(&xl_origin, data, sizeof(xl_origin));
120120

121121
parsed->origin_lsn = xl_origin.origin_lsn;
@@ -188,18 +188,18 @@ ParseAbortRecord(uint8 info, xl_xact_abort *xlrec, xl_xact_parsed_abort *parsed)
188188

189189
if (parsed->xinfo & XACT_XINFO_HAS_GID)
190190
{
191-
int gidlen;
192191
strlcpy(parsed->twophase_gid, data, sizeof(parsed->twophase_gid));
193-
gidlen = strlen(data) + 1;
194-
data += MAXALIGN(gidlen);
192+
data += strlen(data) + 1;
195193
}
196194
}
197195

196+
/* Note: no alignment is guaranteed after this point */
197+
198198
if (parsed->xinfo & XACT_XINFO_HAS_ORIGIN)
199199
{
200200
xl_xact_origin xl_origin;
201201

202-
/* we're only guaranteed 4 byte alignment, so copy onto stack */
202+
/* no alignment is guaranteed, so copy onto stack */
203203
memcpy(&xl_origin, data, sizeof(xl_origin));
204204

205205
parsed->origin_lsn = xl_origin.origin_lsn;

‎src/backend/access/transam/twophase.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1129,9 +1129,11 @@ EndPrepare(GlobalTransaction gxact)
11291129
gxact->prepare_end_lsn = XLogInsert(RM_XACT_ID, XLOG_XACT_PREPARE);
11301130

11311131
if (replorigin)
1132+
{
11321133
/* Move LSNs forward for this replication origin */
11331134
replorigin_session_advance(replorigin_session_origin_lsn,
11341135
gxact->prepare_end_lsn);
1136+
}
11351137

11361138
XLogFlush(gxact->prepare_end_lsn);
11371139

‎src/backend/access/transam/xact.c

Lines changed: 2 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -5245,9 +5245,7 @@ XactLogCommitRecord(TimestampTz commit_time,
52455245
xl_xact_invals xl_invals;
52465246
xl_xact_twophase xl_twophase;
52475247
xl_xact_origin xl_origin;
5248-
52495248
uint8 info;
5250-
int gidlen = 0;
52515249

52525250
Assert(CritSectionCount > 0);
52535251

@@ -5313,10 +5311,7 @@ XactLogCommitRecord(TimestampTz commit_time,
53135311
Assert(twophase_gid != NULL);
53145312

53155313
if (XLogLogicalInfoActive())
5316-
{
53175314
xl_xinfo.xinfo |= XACT_XINFO_HAS_GID;
5318-
gidlen = strlen(twophase_gid) + 1; /* include '\0' */
5319-
}
53205315
}
53215316

53225317
/* dump transaction origin information */
@@ -5370,12 +5365,7 @@ XactLogCommitRecord(TimestampTz commit_time,
53705365
{
53715366
XLogRegisterData((char *) (&xl_twophase), sizeof(xl_xact_twophase));
53725367
if (xl_xinfo.xinfo & XACT_XINFO_HAS_GID)
5373-
{
5374-
static const char zeroes[MAXIMUM_ALIGNOF] = { 0 };
5375-
XLogRegisterData((char*) twophase_gid, gidlen);
5376-
if (MAXALIGN(gidlen) != gidlen)
5377-
XLogRegisterData((char*) zeroes, MAXALIGN(gidlen) - gidlen);
5378-
}
5368+
XLogRegisterData((char *) twophase_gid, strlen(twophase_gid));
53795369
}
53805370

53815371
if (xl_xinfo.xinfo & XACT_XINFO_HAS_ORIGIN)
@@ -5409,7 +5399,6 @@ XactLogAbortRecord(TimestampTz abort_time,
54095399
xl_xact_origin xl_origin;
54105400

54115401
uint8 info;
5412-
int gidlen = 0;
54135402

54145403
Assert(CritSectionCount > 0);
54155404

@@ -5448,10 +5437,7 @@ XactLogAbortRecord(TimestampTz abort_time,
54485437
Assert(twophase_gid != NULL);
54495438

54505439
if (XLogLogicalInfoActive())
5451-
{
54525440
xl_xinfo.xinfo |= XACT_XINFO_HAS_GID;
5453-
gidlen = strlen(twophase_gid) + 1; /* include '\0' */
5454-
}
54555441
}
54565442

54575443
if (TransactionIdIsValid(twophase_xid) && XLogLogicalInfoActive())
@@ -5487,7 +5473,6 @@ XactLogAbortRecord(TimestampTz abort_time,
54875473
if (xl_xinfo.xinfo & XACT_XINFO_HAS_DBINFO)
54885474
XLogRegisterData((char *) (&xl_dbinfo), sizeof(xl_dbinfo));
54895475

5490-
54915476
if (xl_xinfo.xinfo & XACT_XINFO_HAS_SUBXACTS)
54925477
{
54935478
XLogRegisterData((char *) (&xl_subxacts),
@@ -5508,12 +5493,7 @@ XactLogAbortRecord(TimestampTz abort_time,
55085493
{
55095494
XLogRegisterData((char *) (&xl_twophase), sizeof(xl_xact_twophase));
55105495
if (xl_xinfo.xinfo & XACT_XINFO_HAS_GID)
5511-
{
5512-
static const char zeroes[MAXIMUM_ALIGNOF] = { 0 };
5513-
XLogRegisterData((char*) twophase_gid, gidlen);
5514-
if (MAXALIGN(gidlen) != gidlen)
5515-
XLogRegisterData((char*) zeroes, MAXALIGN(gidlen) - gidlen);
5516-
}
5496+
XLogRegisterData((char *) twophase_gid, strlen(twophase_gid) + 1);
55175497
}
55185498

55195499
if (xl_xinfo.xinfo & XACT_XINFO_HAS_ORIGIN)

‎src/include/access/xact.h

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -269,6 +269,7 @@ typedef struct xl_xact_commit
269269
/* xl_xact_relfilenodes follows if XINFO_HAS_RELFILENODES */
270270
/* xl_xact_invals follows if XINFO_HAS_INVALS */
271271
/* xl_xact_twophase follows if XINFO_HAS_TWOPHASE */
272+
/* twophase_gid follows if XINFO_HAS_GID. As a null-terminated string. */
272273
/* xl_xact_origin follows if XINFO_HAS_ORIGIN, stored unaligned! */
273274
} xl_xact_commit;
274275
#define MinSizeOfXactCommit (offsetof(xl_xact_commit, xact_time) + sizeof(TimestampTz))
@@ -278,11 +279,13 @@ typedef struct xl_xact_abort
278279
TimestampTz xact_time; /* time of abort */
279280

280281
/* xl_xact_xinfo follows if XLOG_XACT_HAS_INFO */
281-
/* No db_info required */
282+
/* xl_xact_dbinfo follows if XINFO_HAS_DBINFO */
282283
/* xl_xact_subxacts follows if HAS_SUBXACT */
283284
/* xl_xact_relfilenodes follows if HAS_RELFILENODES */
284285
/* No invalidation messages needed. */
285286
/* xl_xact_twophase follows if XINFO_HAS_TWOPHASE */
287+
/* twophase_gid follows if XINFO_HAS_GID. As a null-terminated string. */
288+
/* xl_xact_origin follows if XINFO_HAS_ORIGIN, stored unaligned! */
286289
} xl_xact_abort;
287290
#define MinSizeOfXactAbort sizeof(xl_xact_abort)
288291

‎src/include/access/xlog_internal.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@
3131
/*
3232
* Each page of XLOG file has a header like this:
3333
*/
34-
#define XLOG_PAGE_MAGIC 0xD097 /* can be used as WAL version indicator */
34+
#define XLOG_PAGE_MAGIC 0xD098 /* can be used as WAL version indicator */
3535

3636
typedef struct XLogPageHeaderData
3737
{

0 commit comments

Comments
 (0)