pg_stat_statements vs. SELECT FOR UPDATE
pg_stat_statements considers a plain select and a select for update to
be equivalent, which seems quite wrong to me as they will have very
different performance characteristics due to locking.
The only comment about it in the code is:
/* we ignore rowMarks */
I propose that it should not ignore rowMarks, per the attached patch or
something similar.
(thanks to Vik Fearing for preliminary testing)
--
Andrew (irc:RhodiumToad)
Attachments:
pgss.patchtext/x-patchDownload
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index f177ebaa2c..080e7591c4 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -333,6 +333,7 @@ static void AppendJumble(pgssJumbleState *jstate,
const unsigned char *item, Size size);
static void JumbleQuery(pgssJumbleState *jstate, Query *query);
static void JumbleRangeTable(pgssJumbleState *jstate, List *rtable);
+static void JumbleRowMarks(pgssJumbleState *jstate, List *rowMarks);
static void JumbleExpr(pgssJumbleState *jstate, Node *node);
static void RecordConstLocation(pgssJumbleState *jstate, int location);
static char *generate_normalized_query(pgssJumbleState *jstate, const char *query,
@@ -2426,7 +2427,7 @@ JumbleQuery(pgssJumbleState *jstate, Query *query)
JumbleExpr(jstate, (Node *) query->sortClause);
JumbleExpr(jstate, query->limitOffset);
JumbleExpr(jstate, query->limitCount);
- /* we ignore rowMarks */
+ JumbleRowMarks(jstate, query->rowMarks);
JumbleExpr(jstate, query->setOperations);
}
@@ -2484,6 +2485,29 @@ JumbleRangeTable(pgssJumbleState *jstate, List *rtable)
}
/*
+ * Jumble a rowMarks list
+ */
+static void
+JumbleRowMarks(pgssJumbleState *jstate, List *rowMarks)
+{
+ ListCell *lc;
+
+ foreach(lc, rowMarks)
+ {
+ RowMarkClause *rowmark = lfirst_node(RowMarkClause, lc);
+ /*
+ * Ignore pushed down clauses as redundant.
+ */
+ if (!rowmark->pushedDown)
+ {
+ APP_JUMB(rowmark->rti);
+ APP_JUMB(rowmark->strength);
+ APP_JUMB(rowmark->waitPolicy);
+ }
+ }
+}
+
+/*
* Jumble an expression tree
*
* In general this function should handle all the same node types that
On 19/01/2019 15:43, Andrew Gierth wrote:
pg_stat_statements considers a plain select and a select for update to
be equivalent, which seems quite wrong to me as they will have very
different performance characteristics due to locking.The only comment about it in the code is:
/* we ignore rowMarks */
I propose that it should not ignore rowMarks, per the attached patch or
something similar.(thanks to Vik Fearing for preliminary testing)
I don't this needs any documentation changes, but some tests would be
nice. I will go add some. Does the extension need a version bump for this?
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
I propose that it should not ignore rowMarks, per the attached patch or
something similar.
+1 for not ignoring rowMarks, but this patch seems like a hack to me.
Why didn't you just add RowMarkClause as one of the known alternative
expression node types? There's no advantage to hard-wiring such
restrictive assumptions about where it can appear.
regards, tom lane
Vik Fearing <vik.fearing@2ndquadrant.com> writes:
Does the extension need a version bump for this?
We don't bump its version when we make any other change that affects
its hash calculation. I don't think this could be back-patched,
but Andrew wasn't proposing to do so (IIUC).
regards, tom lane
"Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
I propose that it should not ignore rowMarks, per the attached patch or
something similar.
Tom> +1 for not ignoring rowMarks, but this patch seems like a hack to
Tom> me. Why didn't you just add RowMarkClause as one of the known
Tom> alternative expression node types?
Because it's not an expression and nothing anywhere else in the backend
treats it as such?
--
Andrew (irc:RhodiumToad)
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
"Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
Tom> +1 for not ignoring rowMarks, but this patch seems like a hack to
Tom> me. Why didn't you just add RowMarkClause as one of the known
Tom> alternative expression node types?
Because it's not an expression and nothing anywhere else in the backend
treats it as such?
Places such as outfuncs.c and copyfuncs.c don't draw a distinction,
and I don't see why pg_stat_statements should either. It would just
be one more place we'd have to fix if we ever allow RowMarkClause in
other places --- or, perhaps more realistically, if the structure of
Query.rowMarks becomes more complex than "flat list of RowMarkClauses".
The other places you mention generally have some specific semantic
knowledge about rowmarks, and would have to be touched anyway if any
changes like that happen. But the jumbling code in pg_stat_statements
has no knowledge of any of that, it's just interested in traversing
the tree. So I'd put it on the same semantic level as outfuncs.c.
regards, tom lane
On 19/01/2019 18:02, Tom Lane wrote:
Vik Fearing <vik.fearing@2ndquadrant.com> writes:
Does the extension need a version bump for this?
We don't bump its version when we make any other change that affects
its hash calculation. I don't think this could be back-patched,
but Andrew wasn't proposing to do so (IIUC).
OK perfect.
Here is Andrew's original patch, but with some tests.
--
Vik Fearing +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Attachments:
pgss.v002.patchtext/x-patch; name=pgss.v002.patchDownload
diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index c759763be2..6001c94ef3 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -354,6 +354,93 @@ SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
SELECT pg_stat_statements_reset() | 1 | 1
(5 rows)
+--
+-- queries with locking clauses
+--
+CREATE TABLE pgss_a (id integer PRIMARY KEY);
+CREATE TABLE pgss_b (id integer PRIMARY KEY, a_id integer REFERENCES pgss_a);
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset
+--------------------------
+
+(1 row)
+
+-- control query
+SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id;
+ id | id | a_id
+----+----+------
+(0 rows)
+
+-- test range tables
+SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR UPDATE;
+ id | id | a_id
+----+----+------
+(0 rows)
+
+SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR UPDATE OF pgss_a;
+ id | id | a_id
+----+----+------
+(0 rows)
+
+SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR UPDATE OF pgss_b;
+ id | id | a_id
+----+----+------
+(0 rows)
+
+SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR UPDATE OF pgss_a, pgss_b; -- should not appear
+ id | id | a_id
+----+----+------
+(0 rows)
+
+SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR UPDATE OF pgss_b, pgss_a;
+ id | id | a_id
+----+----+------
+(0 rows)
+
+-- test strengths
+SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR NO KEY UPDATE;
+ id | id | a_id
+----+----+------
+(0 rows)
+
+SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR SHARE;
+ id | id | a_id
+----+----+------
+(0 rows)
+
+SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR KEY SHARE;
+ id | id | a_id
+----+----+------
+(0 rows)
+
+-- test wait policies
+SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR UPDATE NOWAIT;
+ id | id | a_id
+----+----+------
+(0 rows)
+
+SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR UPDATE SKIP LOCKED;
+ id | id | a_id
+----+----+------
+(0 rows)
+
+SELECT calls, query FROM pg_stat_statements ORDER BY query COLLATE "C";
+ calls | query
+-------+------------------------------------------------------------------------------------------
+ 1 | SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id
+ 1 | SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR KEY SHARE
+ 1 | SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR NO KEY UPDATE
+ 1 | SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR SHARE
+ 2 | SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR UPDATE
+ 1 | SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR UPDATE NOWAIT
+ 1 | SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR UPDATE OF pgss_a
+ 1 | SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR UPDATE OF pgss_b
+ 1 | SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR UPDATE OF pgss_b, pgss_a
+ 1 | SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR UPDATE SKIP LOCKED
+ 1 | SELECT pg_stat_statements_reset()
+(11 rows)
+
+DROP TABLE pgss_a, pgss_b CASCADE;
--
-- utility commands
--
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index f177ebaa2c..137a57e27b 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -333,6 +333,7 @@ static void AppendJumble(pgssJumbleState *jstate,
const unsigned char *item, Size size);
static void JumbleQuery(pgssJumbleState *jstate, Query *query);
static void JumbleRangeTable(pgssJumbleState *jstate, List *rtable);
+static void JumbleRowMarks(pgssJumbleState *jstate, List *rowMarks);
static void JumbleExpr(pgssJumbleState *jstate, Node *node);
static void RecordConstLocation(pgssJumbleState *jstate, int location);
static char *generate_normalized_query(pgssJumbleState *jstate, const char *query,
@@ -2426,7 +2427,7 @@ JumbleQuery(pgssJumbleState *jstate, Query *query)
JumbleExpr(jstate, (Node *) query->sortClause);
JumbleExpr(jstate, query->limitOffset);
JumbleExpr(jstate, query->limitCount);
- /* we ignore rowMarks */
+ JumbleRowMarks(jstate, query->rowMarks);
JumbleExpr(jstate, query->setOperations);
}
@@ -2483,6 +2484,26 @@ JumbleRangeTable(pgssJumbleState *jstate, List *rtable)
}
}
+/*
+ * Jumble a rowMarks list
+ */
+static void
+JumbleRowMarks(pgssJumbleState *jstate, List *rowMarks)
+{
+ ListCell *lc;
+
+ foreach(lc, rowMarks)
+ {
+ RowMarkClause *rowmark = lfirst_node(RowMarkClause, lc);
+ if (!rowmark->pushedDown)
+ {
+ APP_JUMB(rowmark->rti);
+ APP_JUMB(rowmark->strength);
+ APP_JUMB(rowmark->waitPolicy);
+ }
+ }
+}
+
/*
* Jumble an expression tree
*
diff --git a/contrib/pg_stat_statements/sql/pg_stat_statements.sql b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
index aef890d893..3a2b5508ca 100644
--- a/contrib/pg_stat_statements/sql/pg_stat_statements.sql
+++ b/contrib/pg_stat_statements/sql/pg_stat_statements.sql
@@ -177,6 +177,37 @@ SELECT PLUS_ONE(1);
SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
+--
+-- queries with locking clauses
+--
+CREATE TABLE pgss_a (id integer PRIMARY KEY);
+CREATE TABLE pgss_b (id integer PRIMARY KEY, a_id integer REFERENCES pgss_a);
+
+SELECT pg_stat_statements_reset();
+
+-- control query
+SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id;
+
+-- test range tables
+SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR UPDATE;
+SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR UPDATE OF pgss_a;
+SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR UPDATE OF pgss_b;
+SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR UPDATE OF pgss_a, pgss_b; -- should not appear
+SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR UPDATE OF pgss_b, pgss_a;
+
+-- test strengths
+SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR NO KEY UPDATE;
+SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR SHARE;
+SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR KEY SHARE;
+
+-- test wait policies
+SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR UPDATE NOWAIT;
+SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR UPDATE SKIP LOCKED;
+
+SELECT calls, query FROM pg_stat_statements ORDER BY query COLLATE "C";
+
+DROP TABLE pgss_a, pgss_b CASCADE;
+
--
-- utility commands
--
"Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
Tom> +1 for not ignoring rowMarks, but this patch seems like a hack to
Tom> me. Why didn't you just add RowMarkClause as one of the known
Tom> alternative expression node types?
Because it's not an expression and nothing anywhere else in the
backend treats it as such?
Tom> Places such as outfuncs.c and copyfuncs.c don't draw a
Tom> distinction, and I don't see why pg_stat_statements should either.
Tom> It would just be one more place we'd have to fix if we ever allow
Tom> RowMarkClause in other places --- or, perhaps more realistically,
Tom> if the structure of Query.rowMarks becomes more complex than "flat
Tom> list of RowMarkClauses".
None of these possibilities seem even slightly realistic.
Tom> The other places you mention generally have some specific semantic
Tom> knowledge about rowmarks, and would have to be touched anyway if
Tom> any changes like that happen. But the jumbling code in
Tom> pg_stat_statements has no knowledge of any of that, it's just
Tom> interested in traversing the tree. So I'd put it on the same
Tom> semantic level as outfuncs.c.
JumbleExpr's header comments specifically say it's intended to handle
the same kinds of things as expression_tree_walker (barring planner-only
constructs), and RowMarkClause is not one of those things.
If it had been named JumbleNodeTree or whatever then I might agree with
you, but right now the organization of the code is more or less a
straight parallel to query_tree_walker / range_table_walker /
expression_tree_walker, and it seems to me that adding RowMarkClause as
an "expression" node would just distort that parallel for no adequate
reason.
--
Andrew (irc:RhodiumToad)
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested
Hello
Patch is applied cleanly, compiles and pass check-world. Has tests and does not need documentation changes since old behavior was not documented
Well, I can not say something about code.
SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR UPDATE OF pgss_a, pgss_b; -- should not appear
This query is counted as second "SELECT * FROM pgss_a JOIN pgss_b ON pgss_b.a_id = pgss_a.id FOR UPDATE". I prefer a bit more verbose comment here. Firstly I was surprised by both questions "why should not appear?" and "where was the second query call?"
regards, Sergei
Hello
Patch is still applied cleanly on HEAD and passes check-world. I think ignoring FOR UPDATE clause is incorrect behavior.
PS: my note about comments in tests from my previous review is actual too.
regards, Sergei
On Thu, Jul 11, 2019 at 9:08 PM Sergei Kornilov <sk@zsrv.org> wrote:
Patch is still applied cleanly on HEAD and passes check-world. I think ignoring FOR UPDATE clause is incorrect behavior.
With my reviewer hat: I agree.
With my CFM hat: It seems like this patch is ready to land so I have
set it to "Ready for Committer". But don't worry if you were hoping
to review this and missed out, we have two more pg_stat_statements
patches that need your feedback!
https://commitfest.postgresql.org/23/2080/
https://commitfest.postgresql.org/23/1999/
--
Thomas Munro
https://enterprisedb.com
"Sergei" == Sergei Kornilov <sk@zsrv.org> writes:
Sergei> PS: my note about comments in tests from my previous review is
Sergei> actual too.
I changed the comment when committing it.
--
Andrew (irc:RhodiumToad)