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

Hi,

On 2022-08-31 17:33:44 +0200, Drouvot, Bertrand wrote:
> While query jumbling is provided for function calls that’s currently not the
> case for procedures calls.
> The reason behind this is that all utility statements are currently
> discarded for jumbling.
> [...]
> That’s why we think it would make sense to allow jumbling for those 2
> utility statements: CALL and SET.

Yes, I've seen this be an issue repeatedly. Although more heavily for PREPARE
/ EXECUTE than either of the two cases you handle here. IME not tracking
PREPARE / EXECUTE can distort statistics substantially - there's appears to be
a surprising number of applications / frameworks resorting to them. Basically
requiring that track utility is turned on.

I suspect we should carve out things like CALL, PREPARE, EXECUTE from
track_utility - it's more or less an architectural accident that they're
utility statements. It's a bit less clear that SET should be dealt with that
way.

> @@ -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?

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

Same?

> + 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? If we do end up needing it, shouldn't we include the type
in the jumbling?

Greetings,

Andres Freund