Thread: pg_stat_transaction patch

Excerpts from Joel Jacobson's message of jue may 06 09:51:41 -0400 2010:
> Hi,
> 
> I propose a set of new statistics functions and system views.

Hi,

Please add your patch to the next commitfest: http://commitfest.postgresql.org

Thanks
-- 

Joel Jacobson <joel@gluefinance.com> wrote:

> I propose a set of new statistics functions and system views.
>
> I need these functions in order to do automated testing of our system,
> consisting of hundreds of stored procedures in plpgsql.
> My plan is to develop some additional functions to pgTAP, benefiting from
> the new system tables I've added.

I ported your patch into 9.0beta, but it doesn't work well.
I had two assertion failures from the run.sql:

TRAP: FailedAssertion("!(entry->trans == ((void *)0))", File: "pgstat.c", Line: 715)
TRAP: FailedAssertion("!(tabstat->trans == trans)", File: "pgstat.c", Line: 1756)

Also, pg_stat_transaction_functions returned no rows from the test case even
after I removed those assertions. There are no rows in your test/run.out, too.

I like your idea itself, but more works are required for the implementation.

Regards,
---
Takahiro Itagaki
NTT Open Source Software Center

Joel Jacobson <joel@gluefinance.com> wrote:

> I applied all the changes on 9.0beta manually and then it compiled without
> any assertion failures.
> 
> I also changed the oids to a different unused range, since the ones I used
> before had been taken in 9.0beta1.

Thanks, but you still need to test your patch:
- You need to check your patch with "make check", because it requires  adjustments in "rule" test; Your
pg_stat_transaction_functionis the  longest name in the system catalog.
 
- You need to configure postgres with --enable-cassert to enable internal  varidations. The attached test case failed
withthe following TRAP.
 
TRAP: FailedAssertion("!(entry->trans == ((void *)0))", File: "pgstat.c", Line: 715)
TRAP: FailedAssertion("!(tabstat->trans == trans)", File: "pgstat.c", Line: 1758)

> I suspect it is because get_tabstat_entry for some reason returns NULL, in
> for example pg_stat_get_transaction_tuples_inserted(PG_FUNCTION_ARGS).
> 
> Does the function look valid? If you can find the error in it, the other
> functions probably have the same problem.

For the above trap, we can see the comment:   /* Shouldn't have any pending transaction-dependent counts */
We don't expect to read stats entries during transactions. I'm not sure
whether accessing transitional stats during transaction is safe or not.

We might need to go other directions, for example: - Use "session stats" instead "transaction stats". You can see the
same  information in difference of counters between before and after the   transaction. - Export pgBufferUsage instead
ofrelation counters. They are   buffer counters for all relations, but we can obviously export   them because they are
justplain variables.
 

Regards,
---
Takahiro Itagaki
NTT Open Source Software Center

"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.takahiro@gmail.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.takahiro@gmail.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.takahiro@gmail.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