Re: pg_stat_transaction patch

"Accessor functions to get so far collected statistics for the current
transaction"
https://commitfest.postgresql.org/action/patch_view?id=301

The latest version of the patch works as expected, and also well-formed.
I'll mark the patch to "Ready for Committer".
http://archives.postgresql.org/message-id/AANLkTikSYdRwATGAP5U6O6zwIO4b_WNJXIbUd2y2tI01@mail.gmail.com

The added views and functions only accumulates activity counters
in the same transaction where the stat views are queried. So we
need to check the view before COMMIT or ROLLBACK.

The only issue in the patch is too long view and function names:
- pg_stat_transaction_user_tables (31 chars)
- pg_stat_get_transaction_tuples_hot_updated (42 chars)
- pg_stat_get_transaction_function_self_time (42 chars)

Since we've already used _xact_ in some system objects, we could replace
_transaction_ parts with _xact_. It will save 7 key types per query ;-)

--
Itagaki Takahiro

On Mon, Jul 12, 2010 at 11:36, Itagaki Takahiro
<itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> "Accessor functions to get so far collected statistics for the current
> transaction"
> https://commitfest.postgresql.org/action/patch_view?id=301
>
> The latest version of the patch works as expected, and also well-formed.
> I'll mark the patch to "Ready for Committer".
> http://archives.postgresql.org/message-id/AANLkTikSYdRwATGAP5U6O6zwIO4b_WNJXIbUd2y2tI01@mail.gmail.com
>
> The added views and functions only accumulates activity counters
> in the same transaction where the stat views are queried. So we
> need to check the view before COMMIT or ROLLBACK.
>
> The only issue in the patch is too long view and function names:
>  - pg_stat_transaction_user_tables (31 chars)
>  - pg_stat_get_transaction_tuples_hot_updated (42 chars)
>  - pg_stat_get_transaction_function_self_time (42 chars)
>
> Since we've already used _xact_ in some system objects, we could replace
> _transaction_ parts with _xact_. It will save 7 key types per query ;-)

+1 on shortening that down, they're scary long now :-)

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> writes:
> "Accessor functions to get so far collected statistics for the current
> transaction"
> https://commitfest.postgresql.org/action/patch_view?id=301

> The latest version of the patch works as expected, and also well-formed.
> I'll mark the patch to "Ready for Committer".

I'm working through this patch now. I kind of think that we ought to
drop the functions and view columns that claim to report live/dead
tuples. In the first place, they're misnamed, because what they're
actually reporting is delta values (ie, new live tuples or new dead
tuples). In the second place, they don't seem very useful. The
live_tuples count is absolutely, positively guaranteed to read out as
zero, because a transaction that hasn't reached commit cannot have
created any known-live tuples. The dead_tuples count can read out as
positive under certain circumstances, for example if a subtransaction
inserted some tuples and was then rolled back --- we know for certain
those tuples are dead and so the t_delta_dead_tuples count gets
incremented at subtransaction rollback. But for the most part the
dead_tuples count is going to be a lot less than people might expect
based on what the transaction's done so far.

If we keep these we're going to have to document them a lot better
than the submitted patch does. But I think we should just take 'em out.
Comments?

regards, tom lane

Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> writes:
> "Accessor functions to get so far collected statistics for the current
> transaction"
> https://commitfest.postgresql.org/action/patch_view?id=301

> The only issue in the patch is too long view and function names:
> - pg_stat_transaction_user_tables (31 chars)
> - pg_stat_get_transaction_tuples_hot_updated (42 chars)
> - pg_stat_get_transaction_function_self_time (42 chars)

> Since we've already used _xact_ in some system objects, we could replace
> _transaction_ parts with _xact_. It will save 7 key types per query ;-)

Applied, with assorted corrections -

* Renamed *_transaction_* to *_xact_* as suggested by Itagaki-san.

* Removed functions and view columns for delta live/dead tuple counts.

* Marked functions as volatile ... they certainly aren't stable.

* Got rid of use of get_tabstat_entry() to fetch table entries. That
function forcibly creates tabstat entries if they weren't there before,
which was absolutely not what we want here: it'd result in bloating the
tabstat arrays with entries for tables the current transaction actually
never touched. Worse, since you weren't passing the correct isshared
flag for the particular relation, the entries could be created with the
wrong isshared setting, leading to misbehavior if they did get used later
in the transaction. We have to use a find-don't-create function here.

* Fixed bogus handling of inserted/updated/deleted counts --- you need to
add on the pending counts for all open levels of subtransaction.

* Assorted docs improvement and other minor polishing.

BTW, I notice that the patch provides pg_stat_get_xact_blocks_fetched()
and pg_stat_get_xact_blocks_hit(), but doesn't build any views on top of
them. Was this intentional? Providing a full complement of
pg_statio_xact_* views seems like overkill to me, but maybe that was where
you were intending to go and forgot. If the functions are there then
anyone who needs the functionality can easily build their own views atop
them, so this might be an intentional compromise position, but I'm not
sure. Or maybe we should decide that intratransaction statio numbers
aren't likely to be of interest to anybody, and drop the functions too.

regards, tom lane

2010/8/8 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>

> Itagaki Takahiro <itagaki(dot)takahiro(at)gmail(dot)com> writes:
> > "Accessor functions to get so far collected statistics for the current
> > transaction"
> > https://commitfest.postgresql.org/action/patch_view?id=301
>
> > The only issue in the patch is too long view and function names:
> > - pg_stat_transaction_user_tables (31 chars)
> > - pg_stat_get_transaction_tuples_hot_updated (42 chars)
> > - pg_stat_get_transaction_function_self_time (42 chars)
>
> > Since we've already used _xact_ in some system objects, we could replace
> > _transaction_ parts with _xact_. It will save 7 key types per query ;-)
>
> Applied, with assorted corrections -
>
> * Renamed *_transaction_* to *_xact_* as suggested by Itagaki-san.
>
> * Removed functions and view columns for delta live/dead tuple counts.
>
> * Marked functions as volatile ... they certainly aren't stable.
>
> * Got rid of use of get_tabstat_entry() to fetch table entries. That
> function forcibly creates tabstat entries if they weren't there before,
> which was absolutely not what we want here: it'd result in bloating the
> tabstat arrays with entries for tables the current transaction actually
> never touched. Worse, since you weren't passing the correct isshared
> flag for the particular relation, the entries could be created with the
> wrong isshared setting, leading to misbehavior if they did get used later
> in the transaction. We have to use a find-don't-create function here.
>
> * Fixed bogus handling of inserted/updated/deleted counts --- you need to
> add on the pending counts for all open levels of subtransaction.
>
> * Assorted docs improvement and other minor polishing.
>
> BTW, I notice that the patch provides pg_stat_get_xact_blocks_fetched()
> and pg_stat_get_xact_blocks_hit(), but doesn't build any views on top of
> them. Was this intentional? Providing a full complement of
> pg_statio_xact_* views seems like overkill to me, but maybe that was where
> you were intending to go and forgot. If the functions are there then
> anyone who needs the functionality can easily build their own views atop
> them, so this might be an intentional compromise position, but I'm not
> sure. Or maybe we should decide that intratransaction statio numbers
> aren't likely to be of interest to anybody, and drop the functions too.
>

When I created the views, I just copied the existing pg_stat_user_* views
without knowing if any columns where irrelevant for current transaction
data.
I guess if someone would need the blocks_fetched/hit, they could build their
own view.

>
> regards, tom lane
>

--
Best regards,

Joel Jacobson
Glue Finance

E: jj(at)gluefinance(dot)com
T: +46 70 360 38 01

Postal address:
Glue Finance AB
Box 549
114 11 Stockholm
Sweden

Visiting address:
Glue Finance AB
Birger Jarlsgatan 14
114 34 Stockholm
Sweden