Re: pg_plan_advice

On Thu, Apr 2, 2026 at 7:15 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Thu, Apr 2, 2026 at 12:15 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > So here's v24, also dropping pg_collect_advice.
>
> That version didn't actually pass CI. Here's v25.

I've reviewed the pg_stash_advice code and documentation, and I think
this is overall sound. As I mentioned previously, I think its a very
important addition to make pg_plan_advice work for practical problems
end users encounter.

To me this looks good to go, with three minor notes below. For context
I've spent a few hours today going through the code manually, and
doing testing.

And thank you for the detailed notes in your earlier email, and
reworking this. Regarding authorship, I'm happy to be listed as
co-author on the persistence part if you want to keep that in the
commit message. Overall I'm also willing to put in work during the
remaining cycle to test/review/address issues in pg_plan_advice or
pg_stash_advice, so we can hopefully sort out the other items being
discussed.

For 0001:

> diff --git a/doc/src/sgml/pgstashadvice.sgml b/doc/src/sgml/pgstashadvice.sgml
> new file mode 100644
> index 00000000000..ec60552a447
> --- /dev/null
> +++ b/doc/src/sgml/pgstashadvice.sgml
> @@ -0,0 +1,216 @@
> ...
> + <varlistentry>
> + <term>
> + <function>pg_create_advice_stash(stash_name text) returns void</function>
> + <indexterm>
> + <primary>pg_create_advice_stash</primary>
> + </indexterm>
> + </term>
> +
> + <listitem>
> + <para>
> + Creates a new, empty advice stash with the given name.
> + </para>
> + </listitem>
> + </varlistentry>
> +

I think we should document the restrictions on advice names here (i.e.
they must be alphanumeric or contain an underscore, not start with a
digit, and maximum NAMEDATLEN).

For 0002:

> diff --git a/contrib/pg_stash_advice/pg_stash_advice.c b/contrib/pg_stash_advice/pg_stash_advice.c
> index 15e7adf849b..1858c6a135a 100644
> --- a/contrib/pg_stash_advice/pg_stash_advice.c
> +++ b/contrib/pg_stash_advice/pg_stash_advice.c
> ...
> @@ -464,6 +522,43 @@ pgsa_drop_stash(char *stash_name)
> }
> }
> dshash_seq_term(&iterator);
> +
> + /* Bump change count. */
> + pg_atomic_add_fetch_u64(&pgsa_state->change_count, 1);
> +}
> +
> +/*
> + * Remove all stashes and entries from shared memory.
> + *
> + * This is intended to be called before reloading from a dump file, so that
> + * a failed previous attempt doesn't leave stale data behind.
> + */
> +void
> +pgsa_reset_all_stashes(void)
> +{

I think this might be good to expose on the SQL level as well - in
case someone accidentally created a lot of stashes it could be tedious
to remove them all, e.g. if they wanted to clear all the memory after
an experiment.

> diff --git a/contrib/pg_stash_advice/stashpersist.c b/contrib/pg_stash_advice/stashpersist.c
> new file mode 100644
> index 00000000000..da96ee0d803
> --- /dev/null
> +++ b/contrib/pg_stash_advice/stashpersist.c
>...
> + /* Parse the query ID. */
> + errno = 0;
> + queryId = strtoll(queryid_str, &endptr, 10);
> + if (*endptr != '\0' || errno != 0 || queryid_str == endptr)
> + ereport(ERROR,
> + (errcode(ERRCODE_DATA_CORRUPTED),
> + errmsg("syntax error in file \"%s\" line %u: invalid query ID \"%s\"",
> + PGSA_DUMP_FILE, lineno, queryid_str)));
> +

It might be worth adding a queryId == 0 check here, since we won't
check it later, and its helpful to avoid unpredictable behavior just
in case someone decided to mess with the file manually.

Thanks,
Lukas

--
Lukas Fittl