On Mon, Jul 23, 2018 at 04:04:42PM +0900, 임명규 wrote:
> This proposal is about recording additional statistics of wait events.
You should avoid sending things in html format, text format being
recommended on those mailing lists... The patch applies after using
patch -p0 by the way.
I would recommend that you generate your patches using "git
format-patch". Here are general guidelines on the matter:
https://wiki.postgresql.org/wiki/Submitting_a_Patch
Please study those guidelines, those are helpful if you want to get
yourself familiar with community process.
I have comments about your patch. First, I don't think that you need to
count precisely the number of wait events triggered as usually when it
comes to analyzing a workload's bottleneck what counts is a periodic
*sampling* of events, patterns which can be fetched already from
pg_stat_activity and stored say in a different place. This can be
achieved easily by using a cron job with an INSERT SELECT query which
adds data on a relation storing the event counts. I took the time to
look at your patch, and here is some feedback.
This does not need a configure switch. I assume that what you did is
good to learn the internals of ./configure though.
There is no documentation. What does the patch do? What is it useful
for?
+ case PG_WAIT_IPC:
+ arrayIndex = NUM_WAIT_LWLOCK +
+ NUM_WAIT_LOCK + NUM_WAIT_BUFFER_PIN +
+ NUM_WAIT_ACTIVITY + NUM_WAIT_CLIENT +
+ NUM_WAIT_EXTENSION + eventId;
+ break;
This is ugly and unmaintainable style. You could perhaps have
considered an enum instead.
pg_stat_get_wait_events should be a set-returning function, which you
could filter at SQL level using a PID, so no need of it as an argument.
What's the performance penalty? I am pretty sure that this is
measurable as wait events are stored for a backend for each I/O
operation as well, and you are calling a C routine within an inlined
function which is designed to be light-weight, doing only a four-byte
atomic operation.
--
Michael
