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