Horton

Thread

Commits

  1. Teach pg_stat_statements not to ignore FOR UPDATE clauses

  1. pg_stat_statements vs. SELECT FOR UPDATE

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2019-01-19T14:43:10Z

    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)

  2. Re: pg_stat_statements vs. SELECT FOR UPDATE

    Vik Fearing <vik.fearing@2ndquadrant.com> — 2019-01-19T15:12:52Z

    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

  3. Re: pg_stat_statements vs. SELECT FOR UPDATE

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-19T17:00:30Z

    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

  4. Re: pg_stat_statements vs. SELECT FOR UPDATE

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-19T17:02:09Z

    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

  5. Re: pg_stat_statements vs. SELECT FOR UPDATE

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2019-01-20T00:15:55Z

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

  6. Re: pg_stat_statements vs. SELECT FOR UPDATE

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-20T00:31:58Z

    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

  7. Re: pg_stat_statements vs. SELECT FOR UPDATE

    Vik Fearing <vik.fearing@2ndquadrant.com> — 2019-01-20T01:11:19Z

    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

  8. Re: pg_stat_statements vs. SELECT FOR UPDATE

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2019-01-20T03:49:05Z

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

  9. Re: pg_stat_statements vs. SELECT FOR UPDATE

    Sergei Kornilov <sk@zsrv.org> — 2019-03-12T13:19:27Z

    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

  10. Re: pg_stat_statements vs. SELECT FOR UPDATE

    Sergei Kornilov <sk@zsrv.org> — 2019-07-11T09:08:20Z

    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

  11. Re: pg_stat_statements vs. SELECT FOR UPDATE

    Thomas Munro <thomas.munro@gmail.com> — 2019-07-13T05:39:06Z

    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

  12. Re: pg_stat_statements vs. SELECT FOR UPDATE

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2019-07-14T11:19:43Z

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

  13. Re: pg_stat_statements vs. SELECT FOR UPDATE

    Sergei Kornilov <sk@zsrv.org> — 2019-07-14T11:43:45Z

    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