Обсуждение: Cleanup: Replace sscanf with strtol/strtoul in snapmgr
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. -- Regards, Amul Sul EDB: http://www.enterprisedb.com
Вложения
On 4/20/26 07:06, Amul Sul wrote: > 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. I personally prefer this safer and easier to verify parsing so from me this is a +1. I also reviewed the patch and it is simple, looks like it handles errors correctly and matches code we have in other parts of our code so I am all for merging it in its current shape. It also preserves the old behavior of ignoring random stuff at the end of each line, for good and bad. Looks good to me! Andreas
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)
On Mon, May 4, 2026 at 9:19 PM Tristan Partin <tristan@partin.io> wrote:
>
> On Mon Apr 20, 2026 at 12:07 AM CDT, Amul Sul wrote:
> 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.
>
Thanks, Andreas and Tristan, for the review !
I have renamed the variables as suggested but used the shorter forms
procno and xid instead of procNumber and localTransactionId. I also
applied similar changes to parseXidFromText (changing val to xid), but
kept val in parseIntFromText since it seems to be more appropriate for
a generic integer value.
Updated patch attached.
Regards,
Amul
Вложения
On Tue May 5, 2026 at 2:24 AM CDT, Amul Sul wrote:
> On Mon, May 4, 2026 at 9:19 PM Tristan Partin <tristan@partin.io> wrote:
>>
>> On Mon Apr 20, 2026 at 12:07 AM CDT, Amul Sul wrote:
>> 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.
>>
>
> Thanks, Andreas and Tristan, for the review !
>
> I have renamed the variables as suggested but used the shorter forms
> procno and xid instead of procNumber and localTransactionId. I also
> applied similar changes to parseXidFromText (changing val to xid), but
> kept val in parseIntFromText since it seems to be more appropriate for
> a generic integer value.
>
> Updated patch attached.
New patch looks good to me. I can confirm that the only changes in the
new version of the patch are the variable names.
--
Tristan Partin
PostgreSQL Contributors Team
AWS (https://aws.amazon.com)