[BUG] Skipped initialization of some xl_xact_parsed_prepare fields
Hi,
I noticed that inside ParsePrepareRecord function we are missing
initialization of `nstats` and `nabortstats` fields of
xl_xact_parsed_prepare structure:
*** (current code in master)
memset(parsed, 0, sizeof(*parsed));
parsed->xact_time = xlrec->prepared_at;
parsed->origin_lsn = xlrec->origin_lsn;
parsed->origin_timestamp = xlrec->origin_timestamp;
parsed->twophase_xid = xlrec->xid;
parsed->dbId = xlrec->database;
parsed->nsubxacts = xlrec->nsubxacts;
parsed->nrels = xlrec->ncommitrels;
parsed->nabortrels = xlrec->nabortrels;
parsed->nmsgs = xlrec->ninvalmsgs;
***
Thus, they will always be 0 after calling the ParsePrepareRecord
function, but `stats` and `abortstats` pointers are set correctly:
*** (current code in master)
parsed->stats = (xl_xact_stats_item *) bufptr;
bufptr += MAXALIGN(xlrec->ncommitstats * sizeof(xl_xact_stats_item));
parsed->abortstats = (xl_xact_stats_item *) bufptr;
bufptr += MAXALIGN(xlrec->nabortstats * sizeof(xl_xact_stats_item));
***
If we look (for example) on parsing of a commit record, we could see
that both `nstats` and `stats` fields are set inside the
ParseCommitRecord function.
For now, zeroed number of stats lead to invalid output of
`xact_desc_prepare`, because it relies on values of `nstats` and
`nabortstats` fields:
*** (current code in master)
xact_desc_stats(buf, "commit ", parsed.nstats, parsed.stats);
xact_desc_stats(buf, "abort ", parsed.nabortstats, parsed.abortstats);
***
I suppose to add initialization of `nstats` and `nabortstats` to
ParsePrepareRecord (see attached patch).
--
Best regards,
Daniil Davydov
Attachments:
v1-0001-Add-initialization-of-nstats-and-nabortstats-fiel.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Add-initialization-of-nstats-and-nabortstats-fiel.patchDownload
From 6338b32aeb170dfd23ae6d313ab2fb77058e0645 Mon Sep 17 00:00:00 2001
From: Daniil Davidov <d.davydov@postgrespro.ru>
Date: Thu, 15 May 2025 12:19:03 +0700
Subject: [PATCH v1] Add initialization of nstats and nabortstats fields inside
ParsePrepareRecord
---
src/backend/access/rmgrdesc/xactdesc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/backend/access/rmgrdesc/xactdesc.c b/src/backend/access/rmgrdesc/xactdesc.c
index 715cc1f7bad..305598e2865 100644
--- a/src/backend/access/rmgrdesc/xactdesc.c
+++ b/src/backend/access/rmgrdesc/xactdesc.c
@@ -252,6 +252,8 @@ ParsePrepareRecord(uint8 info, xl_xact_prepare *xlrec, xl_xact_parsed_prepare *p
parsed->nsubxacts = xlrec->nsubxacts;
parsed->nrels = xlrec->ncommitrels;
parsed->nabortrels = xlrec->nabortrels;
+ parsed->nstats = xlrec->ncommitstats;
+ parsed->nabortstats = xlrec->nabortstats;
parsed->nmsgs = xlrec->ninvalmsgs;
strncpy(parsed->twophase_gid, bufptr, xlrec->gidlen);
--
2.43.0
On 2025/05/15 14:39, Daniil Davydov wrote:
Hi,
I noticed that inside ParsePrepareRecord function we are missing
initialization of `nstats` and `nabortstats` fields of
xl_xact_parsed_prepare structure:
*** (current code in master)
memset(parsed, 0, sizeof(*parsed));parsed->xact_time = xlrec->prepared_at;
parsed->origin_lsn = xlrec->origin_lsn;
parsed->origin_timestamp = xlrec->origin_timestamp;
parsed->twophase_xid = xlrec->xid;
parsed->dbId = xlrec->database;
parsed->nsubxacts = xlrec->nsubxacts;
parsed->nrels = xlrec->ncommitrels;
parsed->nabortrels = xlrec->nabortrels;
parsed->nmsgs = xlrec->ninvalmsgs;
***Thus, they will always be 0 after calling the ParsePrepareRecord
function, but `stats` and `abortstats` pointers are set correctly:
*** (current code in master)
parsed->stats = (xl_xact_stats_item *) bufptr;
bufptr += MAXALIGN(xlrec->ncommitstats * sizeof(xl_xact_stats_item));parsed->abortstats = (xl_xact_stats_item *) bufptr;
bufptr += MAXALIGN(xlrec->nabortstats * sizeof(xl_xact_stats_item));
***If we look (for example) on parsing of a commit record, we could see
that both `nstats` and `stats` fields are set inside the
ParseCommitRecord function.
For now, zeroed number of stats lead to invalid output of
`xact_desc_prepare`, because it relies on values of `nstats` and
`nabortstats` fields:
*** (current code in master)
xact_desc_stats(buf, "commit ", parsed.nstats, parsed.stats);
xact_desc_stats(buf, "abort ", parsed.nabortstats, parsed.abortstats);
***
Thanks for the report! You're right.
I suppose to add initialization of `nstats` and `nabortstats` to
ParsePrepareRecord (see attached patch).
LGTM. Barring any objection, I will commit this patch.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Thu, May 15, 2025 at 8:30 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
I suppose to add initialization of `nstats` and `nabortstats` to
ParsePrepareRecord (see attached patch).LGTM. Barring any objection, I will commit this patch.
I've pushed the patch. Thanks!
Regards,
--
Fujii Masao