Re: Review: DTrace probes (merged version) ver_03

Zdenek Kotala wrote:
> I performed review and I prepared own patch which contains only probes 
> without any issue. I suggest commit this patch because the rest of patch 
> is independent and it can be committed next commit fest after rework.
>
> I found following issues:

I noticed that CLOG, Subtrans and Multixact probes are added during a
regular Checkpoint, but not during a shutdown flush.  I think the probes
should count that too (probably with the same counter).

In the pgstat_report_activity probe, is it good to call the probe before
taking the fast path out?

In the BUFFER_READ_START probe, we do not include the smgrnblocks()
call, which could be significant since it includes a number of system
calls.

I think BUFFER_HIT and BUFFER_MISS should include the "isLocalBuf" flag.
I also wonder whether BUFFER_HIT should be called in the block above,
lines 220-238, where we check the "found" flag, i.e.
   if (isLocalBuf)   {       ReadLocalBufferCount++;       bufHdr = LocalBufferAlloc(smgr, blockNum, &found);       if
(found)      {           LocalBufferHitCount++;           TRACE_POSTGRESQL_BUFFER_HIT(true);        /* local buffer */
    }else{    TRACE_POSTGRESQL_BUFFER_MISS(true);        /* ditto */}   }   else   {       ReadBufferCount++;
 
       /*        * lookup the buffer.  IO_IN_PROGRESS is set if the requested block is        * not currently in
memory.       */       bufHdr = BufferAlloc(smgr, blockNum, strategy, &found);       if (found)       {
BufferHitCount++;          TRACE_POSTGRESQL_BUFFER_HIT(false);        /* not local */       }else{
TRACE_POSTGRESQL_BUFFER_MISS(false);   /* ditto */}   }
 

(note that this changes the semantics w.r.t. the isExtend flag).


I understand the desire to have DEADLOCK_FOUND, but is there really a
point in having a DEADLOCK_NOTFOUND probe?  Since this code runs every
time someone waits for a lock longer than a second, there would be a lot
of useless counts and nothing useful.

I find it bogus that we include query rewriting in QUERY_PARSE_START/DONE.  
I think query rewriting should be a separate probe.

QUERY_PLAN_START is badly placed -- it should be after the check for
utility commands (alternatively there could be a QUERY_PLAN_DONE in the
fast way out for utility commands, but in that case a "is utility" flag
would be needed.  I don't see that there's any point in tracing planning
of utility commands though).

Why are there no probes for the v3 protocol stuff?  There should
be probes for Parse, Bind, Execute message processing too, for
completeness.  Also, I wonder if these probes should be in the for(;;)
loop in PostgresMain() instead of sprinkled in the other routines.
I note that the probes in PortalRun and PortalRunMulti are schizophrenic
about whether they include utility functions or not.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support