Re: [PATCH] Query Jumbling for CALL and SET utility statements

Hi,

On 8/31/22 6:08 PM, Andres Freund wrote:
> 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