Re: Sampling profiler updated

Hi,

Le 14 juil. 09 à 11:47, Itagaki Takahiro a écrit :
> I updated Sampling profiler patch to be applied to HEAD cleanly.

Which I confirm, as I just spent some time to reviewing the patch (it
was left unassigned in the commit fest). Reviewing code didn't strike
anything so obvious that I'd notice...

> Basic concept of the patch is same as DTrace probes:
> Call pgstat_push_condition(condition) before a operation and call
> pgstat_pop_condition() at the end of the operation. Those functions
> should be light-weight because they only change a variable on shared
> memory without any locks.


...except that the typical integration is like this:

+     pgstat_push_condition(PGCOND_XLOG_OPEN);      fd = BasicOpenFile(path, O_RDWR | PG_BINARY |
get_sync_bit(sync_method),                         S_IRUSR | S_IWUSR);
+     pgstat_pop_condition();

And we have this:

! void
! pgstat_push_condition(PgCondition condition)
! {
!     volatile PgBackendStatus *beentry = MyBEEntry;
!     PgCondition    prev;
!
!     if (profiling_interval <= 0 || !beentry)
!         return;

I'm wondering if it'll be enough for people not interested into
profiling not to bother. The patch contains a lot of added call sites.
I'm not sure if it's possible to arrange for not calling the function
at all when profiling is disabled (GUC profiling_interval = 0).

On the same vein:

+ static void
+ LockPageContent(volatile BufferDesc *buf, LWLockMode mode)
+ {
+     pgstat_push_condition(PGCOND_LWLOCK_PAGE);
+     LWLockAcquire(buf->content_lock, mode);
+     pgstat_pop_condition();
+ }
+
+ static void
+ LockPageIO(volatile BufferDesc *buf, LWLockMode mode)
+ {
+     pgstat_push_condition(PGCOND_LWLOCK_IO);
+     LWLockAcquire(buf->io_in_progress_lock, mode);
+     pgstat_pop_condition();
+ }

With the call site being (in src/backend/storage/buffer/bufmgr.c,
FlushRelationBuffers(Relation rel), FlushDatabaseBuffers(Oid dbid)):
!             LWLockAcquire(bufHdr->content_lock, LW_SHARED);
...
!             LockPageContent(bufHdr, LW_SHARED);

Maybe there's nothing to worry about, but I figured I'd better raise
the concert for further reviewing.

Oh, and this too:
*************** LockBuffer(Buffer buffer, int mode)
*** 2372,2380 ****      if (mode == BUFFER_LOCK_UNLOCK)          LWLockRelease(buf->content_lock);      else if (mode
==BUFFER_LOCK_SHARE) 
!         LWLockAcquire(buf->content_lock, LW_SHARED);      else if (mode == BUFFER_LOCK_EXCLUSIVE)
!         LWLockAcquire(buf->content_lock, LW_EXCLUSIVE);


Now the build went fine, but the testing (default configuration) not
so much:

dim=# create table series(i integer);
dim=# insert into series select generate_series(1, 10000000);
LOG:  checkpoints are occurring too frequently (4 seconds apart)
HINT:  Consider increasing the configuration parameter
"checkpoint_segments".
WARNING:  condition stack overflow: 10
...
WARNING:  condition stack overflow: 11
WARNING:  condition stack overflow: 11
WARNING:  condition stack overflow: 11
ERROR:  canceling statement due to user request

dim=# select count(*) from series; count
-------     0
(1 row)

Time: 9504.624 ms

dim=# select * from pg_profiles; profid |      profname       | profnum
--------+---------------------+---------  83000 | LWLock:Page         |      15  58000 | Data:Extend         |       7
80018| LWLock:BgWriterComm |       1  10000 | CPU                 |      85  22000 | Network:Send        |     128
80009| LWLock:WALWrite     |       6  55000 | Data:Read           |       1  45000 | XLog:Write          |       1
15000| CPU:Utility         |       5  15100 | CPU:Commit          |       1  57000 | Data:Write          |      15
42000| XLog:Insert         |      24  46000 | XLog:Flush          |       4 
(13 rows)

Time: 11.372 ms

The error I got is matching this part of the patch:
! void
! pgstat_push_condition(PgCondition condition)
! {
...
!     if (condition_stack_top < MAX_COND_STACK)
!         condition_stack[condition_stack_top] = prev;
!     else
!         elog(WARNING, "condition stack overflow: %d", condition_stack_top);


So I'm going to change patch state to "Returned with Feedback" as I
guess we'll need to talk about the issue and find a way to solve it,
and I don't think this state prevent from getting back to the patch in
this same fest.

Regards,
--
dim