Обсуждение: [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

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

Hi,

On Wed, May 21, 2025 at 9:59 AM Fujii Masao <masao.fujii@gmail.com> wrote:
> I've pushed the patch. Thanks!
>

Glad to hear it, thank you!

--
Best regards,
Daniil Davydov