public inbox for [email protected]
help / color / mirror / Atom feedCleanup: Replace sscanf with strtol/strtoul in snapmgr
5+ messages / 3 participants
[nested] [flat]
* Cleanup: Replace sscanf with strtol/strtoul in snapmgr
@ 2026-04-20 05:06 Amul Sul <[email protected]>
2026-05-01 08:08 ` Re: Cleanup: Replace sscanf with strtol/strtoul in snapmgr Andreas Karlsson <[email protected]>
2026-05-04 15:49 ` Re: Cleanup: Replace sscanf with strtol/strtoul in snapmgr Tristan Partin <[email protected]>
0 siblings, 2 replies; 5+ messages in thread
From: Amul Sul @ 2026-04-20 05:06 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>
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
Attachments:
[application/x-patch] 0001-snapmgr-replace-sscanf-with-strtol-strtoul-in-snapsh.patch (4.1K, 2-0001-snapmgr-replace-sscanf-with-strtol-strtoul-in-snapsh.patch)
download | inline diff:
From 9869b9f19775de7d13f1bada5db5d8f14bf20859 Mon Sep 17 00:00:00 2001
From: Amul Sul <[email protected]>
Date: Mon, 20 Apr 2026 10:21:28 +0530
Subject: [PATCH] snapmgr: replace sscanf with strtol/strtoul in snapshot parse
helpers
parseIntFromText, parseXidFromText, and parseVxidFromText used sscanf
to convert digit strings to integers. Replace with strtol/strtoul,
which provide explicit overflow detection and advance the end-pointer
directly, avoiding a redundant strchr scan over the already-parsed
digits.
---
src/backend/utils/time/snapmgr.c | 45 +++++++++++++++++++++++++-------
1 file changed, 35 insertions(+), 10 deletions(-)
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 10fe18df2e7..20b8421b881 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -1308,24 +1308,27 @@ parseIntFromText(const char *prefix, char **s, const char *filename)
{
char *ptr = *s;
int prefixlen = strlen(prefix);
- int val;
+ long val;
+ 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", &val) != 1)
+ errno = 0;
+ val = strtol(ptr, &endptr, 10);
+ if (endptr == ptr || errno != 0 || val < INT_MIN || val > INT_MAX)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
errmsg("invalid snapshot data in file \"%s\"", filename)));
- ptr = strchr(ptr, '\n');
+ ptr = strchr(endptr, '\n');
if (!ptr)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
errmsg("invalid snapshot data in file \"%s\"", filename)));
*s = ptr + 1;
- return val;
+ return (int) val;
}
static TransactionId
@@ -1333,24 +1336,27 @@ parseXidFromText(const char *prefix, char **s, const char *filename)
{
char *ptr = *s;
int prefixlen = strlen(prefix);
- TransactionId val;
+ unsigned long val;
+ 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, "%u", &val) != 1)
+ errno = 0;
+ val = strtoul(ptr, &endptr, 10);
+ if (endptr == ptr || errno != 0 || val > UINT_MAX)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
errmsg("invalid snapshot data in file \"%s\"", filename)));
- ptr = strchr(ptr, '\n');
+ ptr = strchr(endptr, '\n');
if (!ptr)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
errmsg("invalid snapshot data in file \"%s\"", filename)));
*s = ptr + 1;
- return val;
+ return (TransactionId) val;
}
static void
@@ -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;
+ 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),
--
2.47.1
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Cleanup: Replace sscanf with strtol/strtoul in snapmgr
2026-04-20 05:06 Cleanup: Replace sscanf with strtol/strtoul in snapmgr Amul Sul <[email protected]>
@ 2026-05-01 08:08 ` Andreas Karlsson <[email protected]>
1 sibling, 0 replies; 5+ messages in thread
From: Andreas Karlsson @ 2026-05-01 08:08 UTC (permalink / raw)
To: Amul Sul <[email protected]>; PostgreSQL Hackers <[email protected]>
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
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Cleanup: Replace sscanf with strtol/strtoul in snapmgr
2026-04-20 05:06 Cleanup: Replace sscanf with strtol/strtoul in snapmgr Amul Sul <[email protected]>
@ 2026-05-04 15:49 ` Tristan Partin <[email protected]>
2026-05-05 07:23 ` Re: Cleanup: Replace sscanf with strtol/strtoul in snapmgr Amul Sul <[email protected]>
1 sibling, 1 reply; 5+ messages in thread
From: Tristan Partin @ 2026-05-04 15:49 UTC (permalink / raw)
To: Amul Sul <[email protected]>; +Cc: pgsql-hackers <[email protected]>
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)
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Cleanup: Replace sscanf with strtol/strtoul in snapmgr
2026-04-20 05:06 Cleanup: Replace sscanf with strtol/strtoul in snapmgr Amul Sul <[email protected]>
2026-05-04 15:49 ` Re: Cleanup: Replace sscanf with strtol/strtoul in snapmgr Tristan Partin <[email protected]>
@ 2026-05-05 07:23 ` Amul Sul <[email protected]>
2026-05-05 16:01 ` Re: Cleanup: Replace sscanf with strtol/strtoul in snapmgr Tristan Partin <[email protected]>
0 siblings, 1 reply; 5+ messages in thread
From: Amul Sul @ 2026-05-05 07:23 UTC (permalink / raw)
To: Tristan Partin <[email protected]>; +Cc: pgsql-hackers <[email protected]>
On Mon, May 4, 2026 at 9:19 PM Tristan Partin <[email protected]> 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
Attachments:
[application/octet-stream] v2-0001-snapmgr-replace-sscanf-with-strtol-strtoul-in-sna.patch (4.1K, 2-v2-0001-snapmgr-replace-sscanf-with-strtol-strtoul-in-sna.patch)
download | inline diff:
From 583dc36426508b73364a681be6ed29e1a4d87808 Mon Sep 17 00:00:00 2001
From: Amul Sul <[email protected]>
Date: Mon, 20 Apr 2026 10:21:28 +0530
Subject: [PATCH v2] snapmgr: replace sscanf with strtol/strtoul in snapshot
parse helpers
parseIntFromText, parseXidFromText, and parseVxidFromText used sscanf
to convert digit strings to integers. Replace with strtol/strtoul,
which provide explicit overflow detection and advance the end-pointer
directly, avoiding a redundant strchr scan over the already-parsed
digits.
---
src/backend/utils/time/snapmgr.c | 47 +++++++++++++++++++++++++-------
1 file changed, 37 insertions(+), 10 deletions(-)
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 10fe18df2e7..27a0ec835e8 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -1308,24 +1308,28 @@ parseIntFromText(const char *prefix, char **s, const char *filename)
{
char *ptr = *s;
int prefixlen = strlen(prefix);
- int val;
+ long val;
+ 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", &val) != 1)
+ errno = 0;
+ val = strtol(ptr, &endptr, 10);
+ if (endptr == ptr || errno != 0 ||
+ val < INT_MIN || val > INT_MAX)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
errmsg("invalid snapshot data in file \"%s\"", filename)));
- ptr = strchr(ptr, '\n');
+ ptr = strchr(endptr, '\n');
if (!ptr)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
errmsg("invalid snapshot data in file \"%s\"", filename)));
*s = ptr + 1;
- return val;
+ return (int) val;
}
static TransactionId
@@ -1333,24 +1337,27 @@ parseXidFromText(const char *prefix, char **s, const char *filename)
{
char *ptr = *s;
int prefixlen = strlen(prefix);
- TransactionId val;
+ unsigned long xid;
+ 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, "%u", &val) != 1)
+ errno = 0;
+ xid = strtoul(ptr, &endptr, 10);
+ if (endptr == ptr || errno != 0 || xid > UINT_MAX)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
errmsg("invalid snapshot data in file \"%s\"", filename)));
- ptr = strchr(ptr, '\n');
+ ptr = strchr(endptr, '\n');
if (!ptr)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
errmsg("invalid snapshot data in file \"%s\"", filename)));
*s = ptr + 1;
- return val;
+ return (TransactionId) xid;
}
static void
@@ -1359,17 +1366,37 @@ parseVxidFromText(const char *prefix, char **s, const char *filename,
{
char *ptr = *s;
int prefixlen = strlen(prefix);
+ long procno;
+ unsigned long xid;
+ 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;
+ procno = strtol(ptr, &endptr, 10);
+ if (endptr == ptr || errno != 0 ||
+ procno < INT_MIN || procno > 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) procno;
+ ptr = endptr + 1; /* skip the '/' separator */
+
+ /* Parse localTransactionId (the unsigned integer after '/') */
+ errno = 0;
+ xid = strtoul(ptr, &endptr, 10);
+ if (endptr == ptr || errno != 0 || xid > UINT_MAX)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ errmsg("invalid snapshot data in file \"%s\"", filename)));
+ vxid->localTransactionId = (LocalTransactionId) xid;
+ ptr = strchr(endptr, '\n');
if (!ptr)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
--
2.47.1
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Cleanup: Replace sscanf with strtol/strtoul in snapmgr
2026-04-20 05:06 Cleanup: Replace sscanf with strtol/strtoul in snapmgr Amul Sul <[email protected]>
2026-05-04 15:49 ` Re: Cleanup: Replace sscanf with strtol/strtoul in snapmgr Tristan Partin <[email protected]>
2026-05-05 07:23 ` Re: Cleanup: Replace sscanf with strtol/strtoul in snapmgr Amul Sul <[email protected]>
@ 2026-05-05 16:01 ` Tristan Partin <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Tristan Partin @ 2026-05-05 16:01 UTC (permalink / raw)
To: Amul Sul <[email protected]>; +Cc: pgsql-hackers <[email protected]>
On Tue May 5, 2026 at 2:24 AM CDT, Amul Sul wrote:
> On Mon, May 4, 2026 at 9:19 PM Tristan Partin <[email protected]> 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)
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2026-05-05 16:01 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-04-20 05:06 Cleanup: Replace sscanf with strtol/strtoul in snapmgr Amul Sul <[email protected]>
2026-05-01 08:08 ` Andreas Karlsson <[email protected]>
2026-05-04 15:49 ` Tristan Partin <[email protected]>
2026-05-05 07:23 ` Amul Sul <[email protected]>
2026-05-05 16:01 ` Tristan Partin <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox