Re: Review: DTrace probes (merged version) ver_03

Alvaro Herrera napsal(a):
> 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).

Yeah, good catch.

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

If you mean to put it behind "if (!pgstat_track_activities || !beentry)" then I
think that current placement is OK. You should be possibility to track it
without dependency on pgstat_track_activities GUC variable. Keep in mind that
DTrace is designed to monitor/trace production system and ability to monitor
something without any configuration change is main idea.

> 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.

It is because the BUFFER_READ_START needs to report correct blockno. My
suggestion is to add probes to smgrblock function.

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

Good point. The question about isExted is if we want to have exact output
include corner case or be to sync with ReadBufferCount counter. I prefer your
suggested placement.

>
> 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.

No idea. Robert any comment?

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

agree

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

agree

> 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.

+1 It is good suggestion. I hope that Robert will put it on his probe todo list.
Same like probes for memory management.

Zdenek