Skip to content

Commit

Permalink
Refactor pgqs_process_opexpr
Browse files Browse the repository at this point in the history 
Previous code wasn't terribly readable, had multiple inconsistencies and failed
to consider some OpExpr.  The changes are:

- we now compute the various qual identifier the same way we store the qual,
  based on the possibly commuted OpExpr
- we now also track OpExpr of the form Constant operator Val where the operator
  doesn't have a commutator
- we now correctly discard quals not referencing a relation

Some regression tests are added to cover those points.
  • Loading branch information
@rjuju
rjuju committed Dec 29, 2019
1 parent 87b452c commit 8d41caf
Show file tree
Hide file tree
Showing 3 changed files with 231 additions and 84 deletions.
74 changes: 73 additions & 1 deletion expected/pg_qualstats.out
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
CREATE EXTENSION pg_qualstats;
-- Make sure sure we'll see at least one qual
SET pg_qualstats.sample_rate = 1;
CREATE TABLE pgqs AS SELECT id FROM generate_series(1, 100) id;
CREATE TABLE pgqs AS SELECT id, 'a' val FROM generate_series(1, 100) id;
SELECT COUNT(*) FROM pgqs WHERE id = 1;
count
-------
Expand Down Expand Up @@ -46,3 +46,75 @@ SELECT COUNT(*) FROM pg_qualstats();
0
(1 row)

-- OpExpr sanity checks
-- subquery_var operator const, shouldn't be tracked
SELECT * FROM (SELECT * FROM pgqs LIMIT 0) pgqs WHERE pgqs.id = 0;
id | val
----+-----
(0 rows)

SELECT COUNT(*) FROM pg_qualstats();
count
-------
0
(1 row)

-- const non_commutable_operator var, should be tracked, var found on RHS
SELECT * FROM pgqs WHERE 'somevalue' ^@ val;
id | val
----+-----
(0 rows)

SELECT lrelid::regclass, lattnum, rrelid::regclass, rattnum FROM pg_qualstats();
lrelid | lattnum | rrelid | rattnum
--------+---------+--------+---------
| | pgqs | 2
(1 row)

SELECT pg_qualstats_reset();
pg_qualstats_reset
--------------------

(1 row)

-- opexpr operator var and commuted, shouldn't be tracked
SELECT * FROM pgqs WHERE id % 2 = 3;
id | val
----+-----
(0 rows)

SELECT * FROM pgqs WHERE 3 = id % 2;
id | val
----+-----
(0 rows)

SELECT COUNT(*) FROM pg_qualstats();
count
-------
0
(1 row)

-- same query with handled commuted qual, which should be found as identical
SELECT * FROM pgqs WHERE id = 0;
id | val
----+-----
(0 rows)

SELECT * FROM pgqs WHERE 0 = id;
id | val
----+-----
(0 rows)

SELECT lrelid::regclass, lattnum, rrelid::regclass, rattnum FROM pg_qualstats();
lrelid | lattnum | rrelid | rattnum
--------+---------+--------+---------
pgqs | 1 | |
pgqs | 1 | |
(2 rows)

SELECT COUNT(DISTINCT qualnodeid) FROM pg_qualstats();
count
-------
1
(1 row)

222 changes: 140 additions & 82 deletions pg_qualstats.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1305,6 +1305,48 @@ get_const_expr(Const *constval, StringInfo buf)

}

/*-----------
* In order to avoid duplicated entries for sementically equivalent OpExpr,
* this function returns a canonical version of the given OpExpr.
*
* For now, the only modification is for OpExpr with a Var and a Const, we
* prefer the form:
* Var operator Const
* with the Var on the LHS. If the expression in the opposite form and the
* operator has a commutator, we'll commute it, otherwise fallback to the
* original OpExpr with the Var on the RHS.
* OpExpr of the form Var operator Var can still be redundant.
*/
static OpExpr *
pgqs_get_canonical_opexpr(OpExpr *expr, bool *commuted)
{
if (commuted)
*commuted = false;

/* Only OpExpr with 2 arguments needs special processing. */
if (list_length(expr->args) != 2)
return expr;

/* If the 1st argument is a Var, nothing is done */
if (IsA(linitial(expr->args), Var))
return expr;

/* If the 2nd argument is a Var, commute the OpExpr if possible */
if (IsA(lsecond(expr->args), Var) && OidIsValid(get_commutator(expr->opno)))
{
OpExpr *new = copyObject(expr);

CommuteOpExpr(new);

if (commuted)
*commuted = true;

return new;
}

return expr;
}

static pgqsEntry *
pgqs_process_opexpr(OpExpr *expr, pgqsWalkerContext *context)
{
Expand All @@ -1314,12 +1356,13 @@ pgqs_process_opexpr(OpExpr *expr, pgqsWalkerContext *context)

if (list_length(expr->args) == 2)
{
Node *node = linitial(expr->args);
Var *var = NULL;
Const *constant = NULL;
bool save_qual;
Node *node;
Var *var;
Const *constant;
bool found;
Oid *sreliddest = NULL;
AttrNumber *sattnumdest = NULL;
Oid *sreliddest;
AttrNumber *sattnumdest;
int position = -1;
StringInfo buf = makeStringInfo();
pgqsHashKey key;
Expand All @@ -1337,75 +1380,92 @@ pgqs_process_opexpr(OpExpr *expr, pgqsWalkerContext *context)
key.queryid = context->queryId;
key.evaltype = context->evaltype;

if (IsA(node, RelabelType))
node = (Node *) ((RelabelType *) node)->arg;
save_qual = false;
var = NULL; /* will store the last Var found, if any */
constant = NULL; /* will store the last Constant found, if any */

if (IsA(node, Var))
node = (Node *) pgqs_resolve_var((Var *) node, context);
/* setup the node and LHS destination fields for the 1st argument */
node = linitial(expr->args);
sreliddest = &(tempentry.lrelid);
sattnumdest = &(tempentry.lattnum);

switch (node->type)
for (int step = 0; step < 2; step++)
{
case T_Var:
var = (Var *) node;
{
RangeTblEntry *rte;
if (IsA(node, RelabelType))
node = (Node *) ((RelabelType *) node)->arg;

rte = list_nth(context->rtable, var->varno - 1);
if (rte->rtekind == RTE_RELATION)
if (IsA(node, Var))
node = (Node *) pgqs_resolve_var((Var *) node, context);

switch (node->type)
{
case T_Var:
var = (Var *) node;
{
tempentry.lrelid = rte->relid;
tempentry.lattnum = var->varattno;
RangeTblEntry *rte;

rte = list_nth(context->rtable, var->varno - 1);
if (rte->rtekind == RTE_RELATION)
{
save_qual = true;
*sreliddest = rte->relid;
*sattnumdest = var->varattno;
}
else
var = NULL;
}
}
break;
case T_Const:
constant = (Const *) node;
break;
default:
break;
}
/* If the operator can be commuted, look at it */
if (var == NULL)
{
if (OidIsValid(get_commutator(expr->opno)))
{
OpExpr *temp = copyObject(expr);

CommuteOpExpr(temp);
node = linitial(temp->args);
sreliddest = &(tempentry.lrelid);
sattnumdest = &(tempentry.lattnum);
break;
case T_Const:
constant = (Const *) node;
break;
default:
break;
}
}
else
{

node = lsecond(expr->args);
sreliddest = &(tempentry.rrelid);
sattnumdest = &(tempentry.rattnum);
}
if (IsA(node, Var))
node = (Node *) pgqs_resolve_var((Var *) node, context);

/* find the node to process for the 2nd pass */
if (step == 0)
{
node = NULL;

switch (node->type)
{
case T_Var:
var = (Var *) node;
if (var == NULL)
{
RangeTblEntry *rte = list_nth(context->rtable, var->varno - 1);
bool commuted;
OpExpr *new = pgqs_get_canonical_opexpr(expr, &commuted);

/*
* If the OpExpr was commuted we have to use the 1st
* argument of the new OpExpr, and keep using the LHS as
* destination fields.
*/
if (commuted)
{
Assert(sreliddest == &(tempentry.lrelid));
Assert(sattnumdest == &(tempentry.lattnum));

*sreliddest = rte->relid;
*sattnumdest = var->varattno;
node = linitial(new->args);
}
}
break;
case T_Const:
constant = (Const *) node;
break;
default:
break;

/*
* If the 1st argument was a var, or if it wasn't and the
* operator couldn't be commuted, use the 2nd argument and the
* RHS as destination fields.
*/
if (node == NULL)
{
/* simply process the next argument */
node = lsecond(expr->args);
/*
* a Var was found and stored on the LHS, so if the next
* node will be stored on the RHS
*/
sreliddest = &(tempentry.rrelid);
sattnumdest = &(tempentry.rattnum);
}
}
}
if (var != NULL)

if (save_qual)
{
pgqsEntry *entry;

Expand All @@ -1415,37 +1475,29 @@ pgqs_process_opexpr(OpExpr *expr, pgqsWalkerContext *context)
*/
if (!pgqs_track_pgcatalog)
{
HeapTuple tp;
Oid nsp;

if (tempentry.lrelid != InvalidOid)
{
tp = SearchSysCache1(RELOID, ObjectIdGetDatum(tempentry.lrelid));
if (!HeapTupleIsValid(tp))
elog(ERROR, "Invalid reloid");
nsp = get_rel_namespace(tempentry.lrelid);

if (((Form_pg_class) GETSTRUCT(tp))->relnamespace == PG_CATALOG_NAMESPACE)
{
ReleaseSysCache(tp);
return NULL;
}
Assert(OidIsValid(nsp));

ReleaseSysCache(tp);
if (nsp == PG_CATALOG_NAMESPACE)
return NULL;
}

if (tempentry.rrelid != InvalidOid)
{
tp = SearchSysCache1(RELOID, ObjectIdGetDatum(tempentry.rrelid));
if (!HeapTupleIsValid(tp))
elog(ERROR, "Invalid reloid");
nsp = get_rel_namespace(tempentry.rrelid);

if (((Form_pg_class) GETSTRUCT(tp))->relnamespace == PG_CATALOG_NAMESPACE)
{
ReleaseSysCache(tp);
return NULL;
}
Assert(OidIsValid(nsp));

ReleaseSysCache(tp);
if (nsp == PG_CATALOG_NAMESPACE)
return NULL;
}
}

if (constant != NULL && pgqs_track_constants)
{
get_const_expr(constant, buf);
Expand Down Expand Up @@ -2122,9 +2174,15 @@ exprRepr(Expr *expr, StringInfo buffer, pgqsWalkerContext *context, bool include

break;
case T_OpExpr:
appendStringInfo(buffer, "%d", ((OpExpr *) expr)->opno);
exprRepr((Expr *) ((OpExpr *) expr)->args, buffer, context, include_const);
{
OpExpr *opexpr;

opexpr = pgqs_get_canonical_opexpr((OpExpr *) expr, NULL);

appendStringInfo(buffer, "%d", opexpr->opno);
exprRepr((Expr *) opexpr->args, buffer, context, include_const);
break;
}
case T_Var:
{
Var *var = (Var *) expr;
Expand Down
19 changes: 18 additions & 1 deletion test/sql/pg_qualstats.sql
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ CREATE EXTENSION pg_qualstats;
-- Make sure sure we'll see at least one qual
SET pg_qualstats.sample_rate = 1;

CREATE TABLE pgqs AS SELECT id FROM generate_series(1, 100) id;
CREATE TABLE pgqs AS SELECT id, 'a' val FROM generate_series(1, 100) id;
SELECT COUNT(*) FROM pgqs WHERE id = 1;
SELECT lrelid::regclass::text, lattnum, occurences, execution_count,
nbfiltered, constvalue, eval_type
Expand All @@ -13,3 +13,20 @@ SELECT COUNT(*) > 0 FROM pg_qualstats();
SELECT COUNT(*) > 0 FROM pg_qualstats_example_queries();
SELECT pg_qualstats_reset();
SELECT COUNT(*) FROM pg_qualstats();
-- OpExpr sanity checks
-- subquery_var operator const, shouldn't be tracked
SELECT * FROM (SELECT * FROM pgqs LIMIT 0) pgqs WHERE pgqs.id = 0;
SELECT COUNT(*) FROM pg_qualstats();
-- const non_commutable_operator var, should be tracked, var found on RHS
SELECT * FROM pgqs WHERE 'somevalue' ^@ val;
SELECT lrelid::regclass, lattnum, rrelid::regclass, rattnum FROM pg_qualstats();
SELECT pg_qualstats_reset();
-- opexpr operator var and commuted, shouldn't be tracked
SELECT * FROM pgqs WHERE id % 2 = 3;
SELECT * FROM pgqs WHERE 3 = id % 2;
SELECT COUNT(*) FROM pg_qualstats();
-- same query with handled commuted qual, which should be found as identical
SELECT * FROM pgqs WHERE id = 0;
SELECT * FROM pgqs WHERE 0 = id;
SELECT lrelid::regclass, lattnum, rrelid::regclass, rattnum FROM pg_qualstats();
SELECT COUNT(DISTINCT qualnodeid) FROM pg_qualstats();

0 comments on commit 8d41caf

Please sign in to comment.