Hi, On Fri, 29 May 2026 at 22:55, Robert Haas <[email protected]> wrote:
> On Sun, May 17, 2026 at 2:25 AM Ayush Tiwari > <[email protected]> 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
