pg_stat_statements vs. SELECT FOR UPDATE

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

"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)

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