Hi Melih,

> I'm not sure about what undefined behaviour could harm this badly.

You are right that in practice nothing wrong will (probably) happen on
x86/x64 architecture with (most?) modern C compilers. This is not true in
the general case though. It's up to the compiler to decide how reading the
bufHdr->tag is going to be actually implemented. This can be one assembly
instruction or several instructions. This reading can be optimized-out if
the compiler believes the required value is already in the register, etc.
Since the result will be different depending on the assembly code used this
is an undefined behaviour and we can't use code like this.

> In the attached patch, I added buffer header locks just before examining
tag as follows

Many thanks for the updated patch! It looks better now.

However I have somewhat mixed feelings about avg_usagecount. Generally
AVG() is a relatively useless methric for monitoring. What if the user
wants MIN(), MAX() or let's say a 99th percentile? I suggest splitting it
into usagecount_min, usagecount_max and usagecount_sum. AVG() can be
derived as usercount_sum / used_buffers.

Also I suggest changing the names of the columns in order to make them
consistent with the rest of the system. If you consider pg_stat_activity
and family [1] you will notice that the columns are named
(entity)_(property), e.g. backend_xid, backend_type, client_addr, etc. So
instead of used_buffers and unused_buffers the naming should be
buffers_used and buffers_unused.

[1]: https://www.postgresql.org/docs/current/monitoring-stats.html

-- 
Best regards,
Aleksander Alekseev

Reply via email to