Re: Cleanup: Replace sscanf with strtol/strtoul in snapmgr

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)