Re: query_id, pg_stat_activity, extended query protocol

> Sounds fine by me (still need to check all three patterns).
>
> + if (list_length(psrc->query_list) > 0)
> + pgstat_report_query_id(linitial_node(Query, psrc->query_list)->queryId, false);
>
> Something that slightly worries me is to assume that the first Query
> in the query_list is fetched. Using a foreach() for all three paths
> may be better, jumping out at the loop when finding a valid query ID.
>
I cannot see how the inital node would not contain the queryId, but
to be on the safe side, your suggestion makes sense.

Are you thinking something like the below? In the foreach,
check for the first queryId != 0, set the queryId and then
break out of the loop

foreach(lc, psrc->query_list)
{
    Query *query = lfirst_node(Query, lc);
    if (query->queryId != UINT64CONST(0))
    {
pgstat_report_query_id(query->queryId, false);
        break;
    }
}
>> We do have to account for the queryId changing after cache revalidation, so
>> we should still set the queryId inside GetCachedPlan in the case the query
>> underwent re-analysis. This means there is a chance that a queryId set at
>> the start of the exec_bind_message may be different by the time we complete
>> the function, in the case the query revalidation results in a different queryId.
> Makes sense to me. I'd rather make that a separate patch, with, if

I will create a separate patch for this.

> possible, its own tests (the case of Andrei with a DROP/CREATE TABLE) .

In terms of testing, there are several options being discussed [1] including
BackgroundPsql and using hooks. I want to add a another idea which
is to rely on compute_plan_id = regress to log if my_query_id is a
non-zero value inside pgstat_report_query_id. Something like below:

@@ -640,6 +641,14 @@ pgstat_report_query_id(uint64 query_id, bool force)
        PGSTAT_BEGIN_WRITE_ACTIVITY(beentry);
        beentry->st_query_id = query_id;
        PGSTAT_END_WRITE_ACTIVITY(beentry);
+
+       if (compute_query_id == COMPUTE_QUERY_ID_REGRESS)
+       {
+               int64 queryId = pgstat_get_my_query_id();
+
+               if (queryId != UINT64CONST(0))
+                       elog(DEBUG3, "queryId value is not zero");
+       }

The number of logs can be counted and compared with what
is expected. For example, in simple query, I expect the queryId to be
set once. Using the \bind, I expect the queryId to be set 3 times (
parse/bind/execute).

Specifically for the DROP/CREATE TABLE test, the \parse and \bindx
being proposed in [2] can be used. The table can be dropped and
recreated after the \parse step. If we count the logs, we would expect
a total of 4 logs to be set (parse/bind/revalidation/execution).

I think the testing discussion should be moved to a different thread.
What do you think?

Regards,

Sami

[1] https://www.postgresql.org/message-id/ZqQk0WHN8EMBEai9%40paquier.xyz
[2] https://www.postgresql.org/message-id/ZqCMCS4HUshUYjGc@paquier.xyz