public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tristan Partin <[email protected]>
To: Amul Sul <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: Cleanup: Replace sscanf with strtol/strtoul in snapmgr
Date: Mon, 04 May 2026 15:49:00 +0000
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAAJ_b97mG=6ybUYE8nQyDfKkhaxTCD8rE8s6L8efNmqHaUYNWQ@mail.gmail.com>
References: <CAAJ_b97mG=6ybUYE8nQyDfKkhaxTCD8rE8s6L8efNmqHaUYNWQ@mail.gmail.com>
On Mon Apr 20, 2026 at 12:07 AM CDT, Amul Sul wrote:
> Hi,
>
> The attached patch replaces sscanf with strtol and strtoul in the
> ImportSnapshot helpers (parseIntFromText, parseXidFromText, and
> parseVxidFromText) to improve reliability and efficiency. By utilizing
> the end pointer, we can locate the next line without re-scanning the
> entire string.
>
> Additionally, this change aligns the snapshot code with the rest of
> the Postgres backend, which already favors these functions for safer
> parsing.
Hey Amul,
The patch generally looks good. One comment:
> @@ -1359,17 +1365,36 @@ parseVxidFromText(const char *prefix, char **s, const char *filename,
> {
> char *ptr = *s;
> int prefixlen = strlen(prefix);
> + long lval;
> + unsigned long ulval;
Perhaps better variable names would be procNumber and
localTransactionId.
> + char *endptr;
>
> if (strncmp(ptr, prefix, prefixlen) != 0)
> ereport(ERROR,
> (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
> errmsg("invalid snapshot data in file \"%s\"", filename)));
> ptr += prefixlen;
> - if (sscanf(ptr, "%d/%u", &vxid->procNumber, &vxid->localTransactionId) != 2)
> +
> + /* Parse procNumber (the signed integer before '/') */
> + errno = 0;
> + lval = strtol(ptr, &endptr, 10);
> + if (endptr == ptr || errno != 0 || lval < INT_MIN || lval > INT_MAX ||
> + *endptr != '/')
> ereport(ERROR,
> (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
> errmsg("invalid snapshot data in file \"%s\"", filename)));
> - ptr = strchr(ptr, '\n');
> + vxid->procNumber = (ProcNumber) lval;
> + ptr = endptr + 1; /* skip the '/' separator */
> +
> + /* Parse localTransactionId (the unsigned integer after '/') */
> + errno = 0;
> + ulval = strtoul(ptr, &endptr, 10);
> + if (endptr == ptr || errno != 0 || ulval > UINT_MAX)
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
> + errmsg("invalid snapshot data in file \"%s\"", filename)));
> + vxid->localTransactionId = (LocalTransactionId) ulval;
> + ptr = strchr(endptr, '\n');
> if (!ptr)
> ereport(ERROR,
> (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
Otherwise, this looks committable to me. In reviewing, I learned that
sscanf() will parse a string like " 45" as 45, so doesn't seem like we
will have any behavioral differences using strto*().
--
Tristan Partin
PostgreSQL Contributors Team
AWS (https://aws.amazon.com)
view thread (5+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Cleanup: Replace sscanf with strtol/strtoul in snapmgr
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox