Re: query_id, pg_stat_activity, extended query protocol

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Sami Imseih <samimseih(at)gmail(dot)com>
Cc: Alexander Lakhin <exclusion(at)gmail(dot)com>, jian he <jian(dot)universality(at)gmail(dot)com>, Anthonin Bonnefoy <anthonin(dot)bonnefoy(at)datadoghq(dot)com>, Andrei Lepikhov <lepihov(at)gmail(dot)com>, kaido vaikla <kaido(dot)vaikla(at)gmail(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: query_id, pg_stat_activity, extended query protocol
Date: 2024-09-30 01:07:55
Message-ID: Zvn5616oYXmpXyHI@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Thu, Sep 26, 2024 at 11:01:12PM -0500, Sami Imseih wrote:
>> I am not sure. The GUCs pretty much enforce this behavior and I doubt
>> that these are going to break moving on. Of course they would, but we
>> are usually careful enough about that as long as it is possible to
>> grep for them. For example see the BRIN case in pageinspect.
>
> Yes, I see pageinspect does the same thing for the BRIN case.
> That is probably OK for this case also.

Okay, I've applied this part then to fix the query ID reporting
for these parallel workers. If people would like a backpatch, please
let me know.

While thinking more about the assertion check in the executor over the
weekend, I've found two things that are not right about it, as of:
- It is OK to not set the query ID if we don't have a query string to
map to. This is something that came up to me because of the parallel
VACUUM case, the query string given to the parallel workers is
optional because we don't have a query string in the case of
autovacuum. This is not an issue currently because autovacuum does
not support parallel jobs (see "tab->at_params.nworkers = -1" in
autovacuum.c), but if we support parallel jobs in autovacuum at some
point the assertion would fail. BRIN and btree always expect a query
string, AFAIK.
- The GUC track_activities. We don't really test it in any tests and
it is enabled by default, so that's really easy to miss. I have been
able to trigger an assertion failure with something like that:
SET compute_query_id = on;
SET track_activities = off;
SELECT 1;

The first point is just some prevention for the future. The second
case is something we should fix and test. I am attaching a patch that
addresses both. Note that the test case cannot use a transaction
block as query IDs are only reported for the top queries, and we can
do a scan of pg_stat_activity to see if the query ID is set. The
assertion was getting more complicated, so I have hidden that behind a
macro in execMain.c. All that should complete this project.

Thoughts?
--
Michael

Attachment Content-Type Size
0001-Expand-assertion-check-for-query-ID-in-executor.patch text/x-diff 4.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Takeshi Ideriha 2024-09-30 01:16:00 Re: BUG #18641: Logical decoding of two-phase commit fails with TOASTed default values
Previous Message Thomas Krennwallner 2024-09-30 00:45:50 pg_upgrade check for invalid databases