support fix query_id for temp table

On Thu, Feb 01, 2024 at 07:37:32AM +0000, ma lz wrote:

session 1:
create temp table ttt ( a int );
insert into ttt values(3); -- query_id is XXX from pg_stat_activity

session 2:
create temp table ttt ( a int );
insert into ttt values(3); -- query_id is YYY from pg_stat_activity

I know temp table has different oid, so query_id is different, is
there a way to use table name for temp table instead of oid?

The CREATE TABLE statements have indeed the same query ID (in 16~),
and the inserts have a different one as they touch different schemas
and relations. That's quite an old problem, that depends on the
RangeVar attached to an InsertStmt. I don't quite see a way to
directly handle that except by using a custom implementation in query
jumbling for this node and its RangeVar, so there is no "easy" way to
tackle that :/
--
Michael

Re: Michael Paquier

On Thu, Feb 01, 2024 at 07:37:32AM +0000, ma lz wrote:

session 1:
create temp table ttt ( a int );
insert into ttt values(3); -- query_id is XXX from pg_stat_activity

session 2:
create temp table ttt ( a int );
insert into ttt values(3); -- query_id is YYY from pg_stat_activity

I know temp table has different oid, so query_id is different, is
there a way to use table name for temp table instead of oid?

The CREATE TABLE statements have indeed the same query ID (in 16~),
and the inserts have a different one as they touch different schemas
and relations. That's quite an old problem, that depends on the
RangeVar attached to an InsertStmt. I don't quite see a way to
directly handle that except by using a custom implementation in query
jumbling for this node and its RangeVar, so there is no "easy" way to
tackle that :/

A customer reported that pg_stat_statements is not useful for them
because they are seeing 160k different query ids in 6-8 hours. They
also proposed to use the temp table name for query jumbling and wrote
a patch for it, which I would also see as the obvious solution to the
problem.

Here's that patch with regression tests added. I would think changing
this would be a big usability improvement for anyone using temp tables
a lot.

There does not seem to be a performance impact - all test were run
with pg_stat_statements active:

Standard pgbench -S (-s 10):
without patch: tps = 154155.407337 (without initial connection time)
with patch: tps = 154223.966534 (without initial connection time)

pgbench -S on temp tables where each table has just one record:
without patch: tps = 184430.801954 (without initial connection time)
with patch: tps = 185692.602764 (without initial connection time)

Christoph

On Mon, Mar 17, 2025 at 10:38:36PM +0100, Christoph Berg wrote:

Here's that patch with regression tests added. I would think changing
this would be a big usability improvement for anyone using temp tables
a lot.

Not the first time I am seeing this argument this month. It is the
second time.

+    /*
+     * If this is a temp table, jumble the name instead of the table oid.
+     */
+    if (expr->rtekind == RTE_RELATION && isAnyTempNamespace(get_rel_namespace(expr->relid)))
+    {
+        rel_name = get_rel_name(expr->relid);
+        AppendJumble(jstate, (const unsigned char *)rel_name, strlen(rel_name));
+    }
+    else
+        JUMBLE_FIELD(relid);

This is OK on its own, still feels a bit incomplete, as the relid also
includes an assumption about the namespace. I would suggested to add
a hardcoded "pg_temp" here, to keep track of this assumption, at
least.

 typedef struct RangeTblEntry
 {
-    pg_node_attr(custom_read_write)
+    pg_node_attr(custom_read_write, custom_query_jumble)

This structure still includes some query_jumble_ignore, which are not
required once custom_query_jumble is added.

We had better document at the top of RangeTblEntry why we are using a
custom function.
--
Michael

Re: Michael Paquier

This is OK on its own, still feels a bit incomplete, as the relid also
includes an assumption about the namespace. I would suggested to add
a hardcoded "pg_temp" here, to keep track of this assumption, at
least.

I had thought about it, but figured that integers and strings are
already separate namespaces, so hashing them shouldn't have any
conflicts. But it's more clear to do that, so added in the new
version:

AppendJumble(jstate, (const unsigned char *)"pg_temp", sizeof("pg_temp"));
AppendJumble(jstate, (const unsigned char *)rel_name, strlen(rel_name));

typedef struct RangeTblEntry
{
-    pg_node_attr(custom_read_write)
+    pg_node_attr(custom_read_write, custom_query_jumble)

This structure still includes some query_jumble_ignore, which are not
required once custom_query_jumble is added.

I would tend to keep them for documentation purposes. (The other
custom_query_jumble functions have a much more explicit structure so
there it is clear which fields are supposed to be jumbled.)

We had better document at the top of RangeTblEntry why we are using a
custom function.

I added a short comment just above custom_query_jumble.

Christoph

On Tue, Mar 18, 2025 at 05:51:54PM +0100, Christoph Berg wrote:

I had thought about it, but figured that integers and strings are
already separate namespaces, so hashing them shouldn't have any
conflicts. But it's more clear to do that, so added in the new
version:

AppendJumble(jstate, (const unsigned char *)"pg_temp", sizeof("pg_temp"));
AppendJumble(jstate, (const unsigned char *)rel_name, strlen(rel_name));

Yes, I know that it's not a big deal, but it could be confusing if
somebody makes an argument about jumbling more object names for
relations. The problem is not only limited to relations, though, as
there are other object types that can use a temp namespace like
functions, but the case of table entries should cover most of the
complaints I can imagine.

typedef struct RangeTblEntry
{
-    pg_node_attr(custom_read_write)
+    pg_node_attr(custom_read_write, custom_query_jumble)

This structure still includes some query_jumble_ignore, which are not
required once custom_query_jumble is added.

I would tend to keep them for documentation purposes. (The other
custom_query_jumble functions have a much more explicit structure so
there it is clear which fields are supposed to be jumbled.)

Fine by me as well to keep a dependency based on the fact that the
structure is rather complicated, but I'd rather document that as well
in parsenodes.h with a slightly fatter comment. What do you think?
--
Michael

Re: Michael Paquier

Fine by me as well to keep a dependency based on the fact that the
structure is rather complicated, but I'd rather document that as well
in parsenodes.h with a slightly fatter comment. What do you think?

You are of course right, that one-line comment was just snakeoil :D.
Now there are proper ones, thanks.

Christoph

On Wed, Mar 19, 2025 at 01:02:54PM +0100, Christoph Berg wrote:

You are of course right, that one-line comment was just snakeoil :D.
Now there are proper ones, thanks.

I have been thinking about this patch for a couple of days. What
makes me unhappy in this proposal is that RangeTblEntry is a large and
complicated Node, and we only want to force a custom computation for
the "relid" portion of the node, while being able to also take some
decisions for what the parent node has. Leaving the existing
per-field query_jumble_ignore with the custom function is prone to
errors, as well, because we need to maintain some level of consistency
between parsenodes.h and src/backend/nodes/.

Hence here is a counter-proposal, that can bring the best of both
worlds: let's add support for custom_query_jumble at field level.
I've spent some time on that, and some concatenation in a macro used
by gen_node_support.pl to force a policy for the custom function name
and its arguments is proving to be rather straight-forward. This
approach addresses the problem of this thread, while also tackling my
concerns about complex node structures.

The custom functions are named _jumble${node}_${field}, with the field
and the parent node given as arguments. I agree that giving the field
is kind of pointless if you have the parent node, but I think that
this is better so as this forces developers to think about how to use
the field value with the node.

Please see the attached. What do you think?
--
Michael

Re: Michael Paquier

I have been thinking about this patch for a couple of days. What
makes me unhappy in this proposal is that RangeTblEntry is a large and
complicated Node, and we only want to force a custom computation for
the "relid" portion of the node, while being able to also take some
decisions for what the parent node has. Leaving the existing
per-field query_jumble_ignore with the custom function is prone to
errors, as well, because we need to maintain some level of consistency
between parsenodes.h and src/backend/nodes/.

Ack, that was also bothering me, but I didn't think it was so easy to
do it on a per-field level. Thanks!

The custom functions are named _jumble${node}_${field}, with the field
and the parent node given as arguments. I agree that giving the field
is kind of pointless if you have the parent node, but I think that
this is better so as this forces developers to think about how to use
the field value with the node.

Makes sense.

Please see the attached. What do you think?

Just one minor thing, I don't understand what you are trying to say in
this comment:

+/*
+ * Note that the argument types are enforced for the per-field custom
+ * functions.
+ */
+#define JUMBLE_CUSTOM(nodetype, item) \
+	_jumble##nodetype##_##item(jstate, expr, expr->item)

Thanks,
Christoph

On Fri, Mar 21, 2025 at 05:26:20PM +0100, Christoph Berg wrote:

Just one minor thing, I don't understand what you are trying to say in
this comment:

+/*
+ * Note that the argument types are enforced for the per-field custom
+ * functions.
+ */
+#define JUMBLE_CUSTOM(nodetype, item) \
+	_jumble##nodetype##_##item(jstate, expr, expr->item)

In this one, I want to mean that we require a custom per-field
function to look like that:
_jumbleNodefoo_field(JumbleState *jstate, NodeFoo *expr, FieldType field);

Rather than having more generic shape like that:
_jumbleNodefoo_field(JumbleState *jstate, Node *exp,
const unsigned char *item, Size size);

So a custom function is defined so as the node type and field type are
arguments. Perhaps this comment would be better if reworded like
that:
"The arguments of this function use the node type and the field type,
rather than a generic argument like AppendJumble() and the other
_jumble() functions."

If you have a better idea, please feel free..
--
Michael

Re: Michael Paquier

+ * Note that the argument types are enforced for the per-field custom
+ * functions.
+ */
+#define JUMBLE_CUSTOM(nodetype, item) \
+	_jumble##nodetype##_##item(jstate, expr, expr->item)

In this one, I want to mean that we require a custom per-field
function to look like that:
_jumbleNodefoo_field(JumbleState *jstate, NodeFoo *expr, FieldType field);

Rather than having more generic shape like that:
_jumbleNodefoo_field(JumbleState *jstate, Node *exp,
const unsigned char *item, Size size);

So a custom function is defined so as the node type and field type are
arguments. Perhaps this comment would be better if reworded like
that:
"The arguments of this function use the node type and the field type,
rather than a generic argument like AppendJumble() and the other
_jumble() functions."

Perhaps this:

/*
* The per-field custom jumble functions get jstate, the node, and the
* field as arguments.
*/

They are not actually different from _jumbleList and _jumbleA_Const
which also get the node (and just not the field). AppendJumble is a
different layer, the output, so it's not surprising its signature is
different.

Are we at the point where the patch is already Ready for Committer?

Thanks,
Christoph

Re: To Michael Paquier

+#define JUMBLE_CUSTOM(nodetype, item) \
+	_jumble##nodetype##_##item(jstate, expr, expr->item)

In this one, I want to mean that we require a custom per-field
function to look like that:
_jumbleNodefoo_field(JumbleState *jstate, NodeFoo *expr, FieldType field);

Perhaps this:

Or actually more explicit:

/*
* Per-field custom jumble functions have this signature:
* _jumbleNodefoo_field(JumbleState *jstate, NodeFoo *expr, FieldType field);
*/

Christoph

On Sat, Mar 22, 2025 at 10:43:00AM +0100, Christoph Berg wrote:

Are we at the point where the patch is already Ready for Committer?

I'll think a bit more about how to document all that. Anyway, yes,
I'm OK with the per-field custom_query_jumble, so let's move on with
that, so I will do something about that.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

On Sat, Mar 22, 2025 at 10:43:00AM +0100, Christoph Berg wrote:

Are we at the point where the patch is already Ready for Committer?

I'll think a bit more about how to document all that. Anyway, yes,
I'm OK with the per-field custom_query_jumble, so let's move on with
that, so I will do something about that.

I'm not terribly happy with the entire proposal.

(1) I know it was asserted upthread that there was no performance
impact, but I find that hard to believe.

(2) This patch inserts catalog lookups into query ID computation,
which AFAIK there never were before. This means you can't compute a
query ID outside a transaction or in an already-failed transaction.
Surely that's going to bite us eventually.

(3) I think having query jumbling work differently for temp tables
than other tables is a fundamentally bad idea.

So my feeling is: if we think this is the behavior we want, let's do
it across the board. I suggest that we simply drop the relid from the
jumble and use the table alias (that is, eref->aliasname) instead.
ISTM this fits well with the general trend in pg_stat_statements
to merge statements together more aggressively than the original
concept envisioned.

regards, tom lane

I wrote:

So my feeling is: if we think this is the behavior we want, let's do
it across the board. I suggest that we simply drop the relid from the
jumble and use the table alias (that is, eref->aliasname) instead.

I experimented with this trivial fix (shown in-line to keep the cfbot
from thinking this is the patch-of-record):

diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 23c9e3c5abf..a54bbdc18b7 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1051,7 +1051,7 @@ typedef struct RangeTblEntry
 	/* user-written alias clause, if any */
 	Alias	   *alias pg_node_attr(query_jumble_ignore);
 	/* expanded reference names */
-	Alias	   *eref pg_node_attr(query_jumble_ignore);
+	Alias	   *eref;

RTEKind rtekind; /* see above */

@@ -1094,7 +1094,7 @@ typedef struct RangeTblEntry
 	 * tables to be invalidated if the underlying table is altered.
 	 */
 	/* OID of the relation */
-	Oid			relid;
+	Oid			relid pg_node_attr(query_jumble_ignore);
 	/* inheritance requested? */
 	bool		inh;
 	/* relation kind (see pg_class.relkind) */

This caused just one diff in existing regression test cases:

diff --git a/contrib/pg_stat_statements/expected/planning.out b/contrib/pg_stat_statements/expected/planning.out
index 3ee1928cbe9..c25b8b946fd 100644
--- a/contrib/pg_stat_statements/expected/planning.out
+++ b/contrib/pg_stat_statements/expected/planning.out
@@ -75,8 +75,9 @@ SELECT plans >= 2 AND plans <= calls AS plans_ok, calls, rows, query FROM pg_sta
   WHERE query LIKE 'SELECT COUNT%' ORDER BY query COLLATE "C";
  plans_ok | calls | rows |                query                 
 ----------+-------+------+--------------------------------------
- t        |     4 |    4 | SELECT COUNT(*) FROM stats_plan_test
-(1 row)
+ f        |     1 |    1 | SELECT COUNT(*) FROM stats_plan_test
+ f        |     3 |    3 | SELECT COUNT(*) FROM stats_plan_test
+(2 rows)

-- Cleanup
DROP TABLE stats_plan_test;

What's happening there is that there's an ALTER TABLE ADD COLUMN in
the test, so the executions after the first one see more entries
in eref->colnames and come up with a different jumble. I think
we probably don't want that behavior; we only want to jumble the
table name. So we'd still need the v3-0001 patch in some form to
allow annotating RangeTblEntry.eref with a custom jumble method
that'd only jumble the aliasname.

regards, tom lane

On Sat, Mar 22, 2025 at 12:24:43PM -0400, Tom Lane wrote:

I experimented with this trivial fix (shown in-line to keep the cfbot
from thinking this is the patch-of-record):

What's happening there is that there's an ALTER TABLE ADD COLUMN in
the test, so the executions after the first one see more entries
in eref->colnames and come up with a different jumble. I think
we probably don't want that behavior; we only want to jumble the
table name. So we'd still need the v3-0001 patch in some form to
allow annotating RangeTblEntry.eref with a custom jumble method
that'd only jumble the aliasname.

Alias.aliasname is not qualified, so it means that we'd begin to
assign the same query ID even if using two relations from two schemas
depending on what search_path assigns, no? Say:
create schema popo1;
create schema popo2;
create table popo1.aa (a int, b int);
create table popo2.aa (a int, b int);
set search_path = 'popo1';
select count(*) from aa;
set search_path = 'popo2';
select count(*) from aa;

=# select query, calls from pg_stat_statements where
query ~ 'select count';
query | calls
-------------------------+-------
select count(*) from aa | 2
(1 row)

Perhaps that's OK because such queries use the same query string, but
just silencing the relid means that we'd lose the namespace reference
entirely, making the stats potentially fuzzier depending on the
workload. On HEAD, one can guess the query ID with an EXPLAIN and a
search_path, as well, so currently it's possible to cross-check the
contents of pgss. But we'd lose this possibility here..
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

Alias.aliasname is not qualified, so it means that we'd begin to
assign the same query ID even if using two relations from two schemas
depending on what search_path assigns, no?

Right. I'm arguing that that's good. The proposed patch already
obscures the difference between similar table names in different
(temp) schemas, and I'm suggesting that taking that a bit further
would be fine.

Note that if the tables we're considering don't have identical
rowtypes, the queries would likely jumble differently anyway
due to differences in Vars' varattno and vartype.

regards, tom lane

On Sat, Mar 22, 2025 at 09:04:19PM -0400, Tom Lane wrote:

Right. I'm arguing that that's good. The proposed patch already
obscures the difference between similar table names in different
(temp) schemas, and I'm suggesting that taking that a bit further
would be fine.

Note that if the tables we're considering don't have identical
rowtypes, the queries would likely jumble differently anyway
due to differences in Vars' varattno and vartype.

Not for the types AFAIK, the varattnos count in, but perhaps for the
same argument as previously it's just kind of OK? Please see the
tests in the attached about that.

I've spent a few hours looking at the diffs of a pgss dump before and
after the fact. The reduction in the number of entries seem to come
mainly from tests where we do a successive creates and drops of the
same table name. There are quite a few of them for updatable views,
but it's not the only one. A good chunk comes from tables with
simpler and rather generic names.

So your idea to use the relation name in eref while skipping the
column list looks kind of promising. Per se the attached. Thoughts?
--
Michael

Re: Michael Paquier

So your idea to use the relation name in eref while skipping the
column list looks kind of promising. Per se the attached. Thoughts?

Makes sense to me, thanks for digging into it.

+++ b/src/backend/nodes/queryjumblefuncs.c
@@ -33,6 +33,7 @@
#include "postgres.h"

#include "access/transam.h"
+#include "catalog/namespace.h"

No longer needed.

+++ b/contrib/pg_stat_statements/sql/select.sql
+SET search_path = 'pgss_schema_1';
+SELECT count(*) FROM tab_search_same;
+SELECT a, b FROM tab_search_same;
+SELECT count(*) FROM tab_search_diff_1;
+SELECT count(*) FROM tab_search_diff_2;
+SELECT a FROM tab_search_diff_2;
+SET search_path = 'pgss_schema_1';

Should this be pgss_schema_2 ?

Christoph

So your idea to use the relation name in eref while skipping the
column list looks kind of promising. Per se the attached. Thoughts?

I feel really uneasy about this behavior becoming the default.
I can bet there are some users which run common queries across
different schemas ( e.g. multi-tenancy ) will consider this behavior a
regression
in pg_stat_statements as now all their common queries have been merged
into a single entry.

For example, I have seen users add comments to SQLs to differentiate
similar SQLs coming from different tenants. This patch makes this no longer a
somewhat decent workaround to overcome the fact that pg_stat_statements
does not track schemas or search path.

```
select pg_stat_statements_reset();

set search_path = s1;
select /*+ user s1 */ * from foo;

set search_path = s2;
select /*+ user s2 */ * from foo;

reset search_path;
select userid, queryid, query, calls from public.pg_stat_statements;

test=# select userid, queryid, query, calls from public.pg_stat_statements;
userid | queryid | query | calls
--------+----------------------+-----------------------------------+-------
10 | 1788423388555345932 | select /*+ user s1 */ * from foo | 2
10 | -8935568138104064674 | select pg_stat_statements_reset() | 1
10 | -8663970364987885379 | set search_path = $1 | 2
10 | -6563543739552933350 | reset search_path | 1
(4 rows)
```

--
Sami Imseih
Amazon Web Services (AWS)