pg_plan_advice: fix parsing underscore in numbers

On 17 Jul 2026, at 11:22, Chao Li <li.evan.chao@gmail.com> wrote:

PG supports underscores between digits; for example, 1_0 is parsed as 10:
```
evantest=# SELECT 1_0;
?column?
----------
10
(1 row)
```

pg_plan_advice seems to intend to support the same syntax, based on pgpa_scanner.l:

Agreed.

The attached patch fixes this by using pg_strtoint32_safe() in the same way as the core scanner.

Yes, but I think we need a bit more since we otherwise accept this case:

postgres=# set pg_plan_advice.advice = 'SEQ_SCAN(x#1_0_)';
SET
postgres=# explain (plan_advice) select 1;
QUERY PLAN
------------------------------------------
Result (cost=0.00..0.01 rows=1 width=4)
Supplied Plan Advice:
SEQ_SCAN(x#10) /* not matched */
SEQ_SCAN(_) /* not matched */
Generated Plan Advice:
NO_GATHER("*RESULT*")
(6 rows)

If we also match on integerjunk like in the attached v2 we instead get the
below which I think is more correct:

postgres=# set pg_plan_advice.advice = 'SEQ_SCAN(x#1_0_)';
ERROR: invalid value for parameter "pg_plan_advice.advice": "SEQ_SCAN(x#1_0_)"
DETAIL: Could not parse advice: trailing junk after numeric literal at or near "1_0_"

AFAICT this shouldn't break parsing for any legitimate cases, but I've only had
a quick look (the tests still pass though).

--
Daniel Gustafsson

On Jul 17, 2026, at 18:16, Daniel Gustafsson <daniel@yesql.se> wrote:

The attached patch fixes this by using pg_strtoint32_safe() in the same way as the core scanner.

Yes, but I think we need a bit more since we otherwise accept this case:

postgres=# set pg_plan_advice.advice = 'SEQ_SCAN(x#1_0_)';
SET
postgres=# explain (plan_advice) select 1;
QUERY PLAN
------------------------------------------
Result (cost=0.00..0.01 rows=1 width=4)
Supplied Plan Advice:
SEQ_SCAN(x#10) /* not matched */
SEQ_SCAN(_) /* not matched */
Generated Plan Advice:
NO_GATHER("*RESULT*")
(6 rows)

If we also match on integerjunk like in the attached v2 we instead get the
below which I think is more correct:

postgres=# set pg_plan_advice.advice = 'SEQ_SCAN(x#1_0_)';
ERROR: invalid value for parameter "pg_plan_advice.advice": "SEQ_SCAN(x#1_0_)"
DETAIL: Could not parse advice: trailing junk after numeric literal at or near "1_0_"

AFAICT this shouldn't break parsing for any legitimate cases, but I've only had
a quick look (the tests still pass though).

--
Daniel Gustafsson

<v2-0001-Fix-parsing-of-underscores-in-pg_plan_advice-occu.patch>

Agreed, v2 is more correct. I missed the integerjunk part.

I also added a test case for 1_0_. There is already a syntax-error section in the test script, so I added the case there.

PFA v3, which only adds this test case.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

On Fri, Jul 17, 2026 at 1:59 PM Chao Li <li.evan.chao@gmail.com> wrote:

On Jul 17, 2026, at 18:16, Daniel Gustafsson <daniel@yesql.se> wrote:

The attached patch fixes this by using pg_strtoint32_safe() in the same way as the core scanner.

FWIW, I've raised this issue at the time during patch review, and
Robert noted "OK" at the time, but I assume this was forgotten in the
final committed version. Excerpt from that thread:

On Mon, Mar 16, 2026 at 1:51 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Fri, Mar 13, 2026 at 4:39 AM Lukas Fittl <lukas@fittl.com> wrote:

- pgpa_scanner accepts integers with underscores, but incorrectly uses
a simple strtoint on them (which would fail), instead of pg_strtoint32
/ pg_strtoint32_safe

OK.

See /messages/by-id/CA+TgmoYMpO=+EQwDhMoH=zmTGbB6TD5-_9bciNEoDArP8JjSMw@mail.gmail.com

PFA v3, which only adds this test case.

I think v3 looks good from a quick look.

Thanks,
Lukas

--
Lukas Fittl

On 18 Jul 2026, at 03:04, Lukas Fittl <lukas@fittl.com> wrote:
On Fri, Jul 17, 2026 at 1:59 PM Chao Li <li.evan.chao@gmail.com> wrote:

On Jul 17, 2026, at 18:16, Daniel Gustafsson <daniel@yesql.se> wrote:

The attached patch fixes this by using pg_strtoint32_safe() in the same way as the core scanner.

FWIW, I've raised this issue at the time during patch review, and
Robert noted "OK" at the time, but I assume this was forgotten in the
final committed version.

Considering the size and complexity of the patchset I am not the least bit
surprised, it's easy to lose track of all the things needing to be fixed,
happens to the best of us. Thanks for the reference to the thread though,
clearly both you and Chao Li should get credited with reporting the bug.

PFA v3, which only adds this test case.

I think v3 looks good from a quick look.

Thanks for the updated patch and the review of that version, I'll have a closer
look at it later tonight.

--
Daniel Gustafsson

On 18 Jul 2026, at 18:47, Daniel Gustafsson <daniel@yesql.se> wrote:

Thanks for the updated patch and the review of that version, I'll have a closer
look at it later tonight.

Looking again I can't see anything that sticks out, so unless objected to I'll
get this applied tomorrow or so. The attached v4 just modifies the commit
message to add Lukas' thread.

--
Daniel Gustafsson

On 18 Jul 2026, at 23:08, Daniel Gustafsson <daniel@yesql.se> wrote:

On 18 Jul 2026, at 18:47, Daniel Gustafsson <daniel@yesql.se> wrote:

Thanks for the updated patch and the review of that version, I'll have a closer
look at it later tonight.

Looking again I can't see anything that sticks out, so unless objected to I'll
get this applied tomorrow or so.

Done.

--
Daniel Gustafsson