Hi,
On 10/6/22 8:39 AM, Michael Paquier wrote:
> On Mon, Sep 19, 2022 at 08:29:22AM +0200, Drouvot, Bertrand wrote:
>> Please find attached v6 taking care of the remarks mentioned above.
>> + case T_VariableSetStmt:
>> + {
>> + VariableSetStmt *stmt = (VariableSetStmt *) node;
>> +
>> + /* stmt->name is NULL for RESET ALL */
>> + if (stmt->name)
>> + {
>> + APP_JUMB(stmt->kind);
>> + APP_JUMB_STRING(stmt->name);
>> + JumbleExpr(jstate, (Node *) stmt->args);
>> + }
>> + }
>> + break;
>
> Hmm. If VariableSetStmt->is_local is not added to the jumble, then
> aren't "SET foo = $1" and "SET LOCAL foo = $1" counted as the same
> query?
>
Good catch, thanks!
While at it let's also jumble "SET SESSION foo =". For this one, we
would need to record another bool in VariableSetStmt: I'll create a
dedicated patch for that.
> I am not seeing SAVEPOINT, RELEASE, ROLLBACK .. TO SAVEPOINT
> mentioned on this thread. Would these be worth considering in what
> gets compiled? That would cover the remaining bits of
> TransactionStmt. The ODBC driver abuses of savepoints, for example,
> so this could be useful for monitoring purposes in such cases.
Agree. I'll look at those too.
>
> As of the code stands, it could be cleaner to check
> IsJumbleUtilityAllowed() in compute_utility_query_id(), falling back
> to a default in JumbleQuery(). Not that what your patch does is
> incorrect, of course.
Will look at it.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
