Re: query_id, pg_stat_activity, extended query protocol - Mailing list pgsql-hackers

On Wed, Sep 25, 2024 at 05:00:00PM +0300, Alexander Lakhin wrote:
> Please look at the script, which triggers Assert added by 24f520594:
> (assuming shared_preload_libraries=pg_stat_statements)

Or just compute_query_id = on.

> SELECT repeat('x', 100) INTO t FROM generate_series(1, 100000);
> CREATE FUNCTION f() RETURNS int LANGUAGE sql IMMUTABLE RETURN 0;
> CREATE INDEX ON t(f());
>
> TRAP: failed Assert("!IsQueryIdEnabled() || pgstat_get_my_query_id() != 0"), File: "execMain.c", Line: 300, PID: 1288609
> ExceptionalCondition at assert.c:52:13
> ExecutorRun at execMain.c:302:6
> postquel_getnext at functions.c:903:24
> fmgr_sql at functions.c:1198:15
> ExecInterpExpr at execExprInterp.c:746:8
> ExecInterpExprStillValid at execExprInterp.c:2034:1
> ExecEvalExprSwitchContext at executor.h:367:13

And this assertion is doing the job I want it to do, because it is
telling us that we are not setting a query ID when doing a parallel
btree build.  The query string that we would report at the beginning
of _bt_parallel_build_main() is passed down as a parameter, but not
the query ID.  Hence pg_stat_activity would report a NULL query ID
when spawning parallel workers in this cases, even if there is a query
string.

The same can be said for the parallel build for BRIN, that uses a lot
of logic taken from btree for there parallel parameters, and even
vacuum, as it goes through a parse analyze where its query ID would be
set. but that's missed in the workers.

See _bt_parallel_build_main(), _brin_parallel_build_main() and
parallel_vacuum_main() which are the entry point used by the workers
for all of them.  For BRIN, note that I can get the same failure with
the following query, based on the table of your previous test that
would spawn a worker:
CREATE INDEX foo ON t using brin(f());

The recovery test 027_stream_regress.pl not catching these failures
means that we don't have tests with an index expression for such
parallel builds, or the assertion would have triggered.  It looks like
this is just because we don't do a parallel btree build with an index
expression where we need to go through the executor to build its
IndexInfo.

Note that parallel workers launched by execParallel.c pass down the
query ID in a minimal PlannedStmt where we use pgstat_get_my_query_id,
so let's do the same for all these.

Attached is the patch I am finishing with, with some new tests for
BRIN and btree to force parallel builds with immutable expressions
through functions.  These fail the assertions in the recovery TAP
test.  It may be a good idea to keep these tests in the long-term
anyway.  It took me a few minutes to find out that
min_parallel_table_scan_size and max_parallel_maintenance_workers was
enough to force workers to spawn even if tables have no data, to make
the tests cheaper.

Thoughts or comments?
--
Michael