Thread: 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)
Attachment
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
Attachment
>>>>> "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 behaviorwas 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 bitmore verbose comment here. Firstly I was surprised by both questions "why should not appear?" and "where was the secondquery 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)
Hi > Sergei> PS: my note about comments in tests from my previous review is > Sergei> actual too. > > I changed the comment when committing it. Great, thank you! regards, Sergei