Re: [PATCH] Query Jumbling for CALL and SET utility statements - Mailing list pgsql-hackers

Hi,

Hi,

On 2022-08-31 17:33:44 +0200, Drouvot, Bertrand wrote:

@@ -383,6 +384,30 @@ JumbleExpr(JumbleState *jstate, Node *node)                              APP_JUMB(var->varlevelsup);                      }                      break;
+             case T_CallStmt:
+                     {
+                             CallStmt   *stmt = (CallStmt *) node;
+                             FuncExpr   *expr = stmt->funcexpr;
+
+                             APP_JUMB(expr->funcid);
+                             JumbleExpr(jstate, (Node *) expr->args);
+                     }
+                     break;

Why do we need to take the arguments into account?

Thanks for looking at it!

Agree that It's not needed to "solve" the Lock contention issue, but I think it's needed for the "render".

Without it we would get, things like:

postgres=# call MY_PROC(10);
CALL
postgres=# call MY_PROC(100000000);
CALL
postgres=# SELECT query, calls, rows FROM pg_stat_statements;
               query               | calls | rows
-----------------------------------+-------+------
 select pg_stat_statements_reset() |     1 |    1
 call MY_PROC(10)                  |     2 |    0
(2 rows)

instead of

postgres=# SELECT query, calls, rows FROM pg_stat_statements;
               query               | calls | rows
-----------------------------------+-------+------
 select pg_stat_statements_reset() |     1 |    1
 call MY_PROC($1)                  |     2 |    0
(2 rows)

+             case T_VariableSetStmt:
+                     {
+                             VariableSetStmt *stmt = (VariableSetStmt *) node;
+
+                             APP_JUMB_STRING(stmt->name);
+                             JumbleExpr(jstate, (Node *) stmt->args);
+                     }
+                     break;

Same?

yeah, same reason. Without it we would get things like:

postgres=# set enable_seqscan=false;
SET
postgres=# set enable_seqscan=true;
SET
postgres=# SELECT query, calls, rows FROM pg_stat_statements;
               query               | calls | rows
-----------------------------------+-------+------
 select pg_stat_statements_reset() |     1 |    1
 set enable_seqscan=false          |     2 |    0
(2 rows)

instead of

postgres=# SELECT query, calls, rows FROM pg_stat_statements;
               query               | calls | rows
-----------------------------------+-------+------
 set enable_seqscan=$1             |     2 |    0
 select pg_stat_statements_reset() |     1 |    1
(2 rows)

+             case T_A_Const:
+                     {
+                             int                     loc = ((const A_Const *) node)->location;
+
+                             RecordConstLocation(jstate, loc);
+                     }
+                     break;

I suspect we only need this because of the jumbling of unparsed arguments I
questioned above? 

Right but only for the T_VariableSetStmt case.

 If we do end up needing it, shouldn't we include the type
in the jumbling?

I don't think so as this is only for the T_VariableSetStmt case.

And looking closer I don't see such as thing as "consttype" (that we can find in the Const struct) in the A_Const struct.

Regards,

-- 
Bertrand Drouvot
Amazon Web Services: https://aws.amazon.com