Re: pg_stash_advice: dump file with overlong stash name crashes worker in a restart loop

Hi,

On Fri, 29 May 2026 at 22:55, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Sun, May 17, 2026 at 2:25 AM Ayush Tiwari
> <ayushtiwari(dot)slg01(at)gmail(dot)com> wrote:
> > While testing pg_stash_advice, I came across a
> > crash that's reachable through the persistence dump file:
>
> Hi,
>
> Thanks for the report and patch. I found it to be more elaborate than
> what I think we need, so I have committed a simplified version. While
> it's good to fix stuff like this, the chances of anyone hitting in
> practice are very low, since it requires the dump file to be
> corrupted. Adding a test that could fail with a different value of
> NAMEDATALEN seems likely to hurt more than it helps, and making the
> test more elaborate to avoid that peril does not seem like the best
> use of time and energy. I don't see this as something that needs to be
> tested on every regression test run from now until forever.
>

Thanks for the update and fix!

While reading through pg_stash_advice I noticed something about
pg_set_stashed_advice(stash_name text, query_id bigint, advice_string text)
that I wanted to ask about, since I might be misunderstanding the intended
design.

The function is (correctly, I think) not marked STRICT, because a NULL
advice_string is meaningful, it clears any existing advice for that
stash/query ID. That part makes sense to me.

What surprised me is the handling of the first two arguments:

if (PG_ARGISNULL(0) || PG_ARGISNULL(1))
PG_RETURN_NULL();

So a NULL stash name or NULL query ID is silently accepted and the call
just becomes a no-op that returns NULL. That felt a little odd next to the
neighboring check, where a query ID of 0 is treated as a hard error:

if (queryId == 0)
ereport(ERROR, ... "cannot set advice string for query ID 0");

A few thing I was wondering about:

1. Is the PG_RETURN_NULL() on NULL stash name / query ID intentional, or is
it a leftover from an earlier shape of the function? It looks a bit like
an attempt to emulate STRICT for just the first two arguments, but the
effect is that an accidental NULL (say, a scalar subquery that returned
no rows) silently does nothing rather than telling the user something
went wrong.

2. While there, I noticed the function's header comment says "If the second
argument is NULL, we delete any existing advice stash entry", but the
code actually keys the delete path off the third argument (advice_string,
PG_ARGISNULL(2)). I think the comment is just stale, does that match
your understanding?

Regards,
Ayush