Re: [PATCH] pg_stat_toast v6

Am 03.01.22 um 22:23 schrieb Alvaro Herrera:
> Overall I think this is a good feature to have; assessing the need for
> compression is important for tuning, so +1 for the goal of the patch.

Much appreciated!


> I didn't look into the patch carefully, but here are some minor
> comments:
> 
> On 2022-Jan-03, Gunnar "Nick" Bluth wrote:
> 
>> @@ -229,7 +230,9 @@ toast_tuple_try_compression(ToastTupleContext *ttc, int attribute)
>>       Datum       *value = &ttc->ttc_values[attribute];
>>       Datum        new_value;
>>       ToastAttrInfo *attr = &ttc->ttc_attr[attribute];
>> +    instr_time    start_time;
>>   
>> +    INSTR_TIME_SET_CURRENT(start_time);
>>       new_value = toast_compress_datum(*value, attr->tai_compression);
>>   
>>       if (DatumGetPointer(new_value) != NULL)
> 
> Don't INSTR_TIME_SET_CURRENT unconditionally; in some systems it's an
> expensive syscall.  Find a way to only do it if the feature is enabled.

Yeah, I was worried about that (and asking if it would be required) already.
Adding the check was easier than I expected, though I'm absolutely 
clueless if I did it right!

#include "pgstat.h"
extern PGDLLIMPORT bool pgstat_track_toast;


> This also suggests that perhaps it'd be a good idea to allow this to be
> enabled for specific tables only, rather than system-wide.  (Maybe in
> order for stats to be collected, the user should have to both set the
> GUC option *and* set a per-table option?  Not sure.)

That would of course be nice, but I seriously doubt the required 
additional logic would be justified. The patch currently tampers with as 
few internal structures as possible, and for good reason... ;-)

>> @@ -82,10 +82,12 @@ typedef enum StatMsgType
>>       PGSTAT_MTYPE_DEADLOCK,
>>       PGSTAT_MTYPE_CHECKSUMFAILURE,
>>       PGSTAT_MTYPE_REPLSLOT,
>> +    PGSTAT_MTYPE_CONNECTION,
> 
> I think this new enum value doesn't belong in this patch.

Yeah, did I mention I'm struggling with rebasing? ;-|


>> +/* ----------
>> + * PgStat_ToastEntry            Per-TOAST-column info in a MsgFuncstat
>> + * ----------
> 
> Not in "a MsgFuncstat", right?

Obviously... fixed!

> 
>> +-- function to wait for counters to advance
>> +create function wait_for_stats() returns void as $$
> 
> I don't think we want a separate copy of wait_for_stats; see commit
> fe60b67250a3 and the discussion leading to it.  Maybe you'll want to
> move the test to stats.sql.  I'm not sure what to say about relying on

Did so.

> LZ4; maybe you'll want to leave that part out, and just verify in an
> LZ4-enabled build that some 'l' entry exists.  BTW, don't we have any
> decent way to turn that 'l' into a more reasonable, descriptive string?

> Also, perhaps make the view-defining query turn an empty compression
> method into whatever the default is.

I'm not even sure that having it in there is useful at all. It's simply 
JOINed in from pg_attribute.
Which is where I'd see that "make it look nicer" change happening, tbth. ;-)

> Or even better, stats collection
> should store the real compression method used rather than empty string,
> to avoid confusing things when some stats are collected, then the
> default is changed, then some more stats are collected.

I was thinking about that already, but came to the conclusion that it a) 
would blow up the size of these statistics by quite a bit and b) would 
be quite tricky to display in a useful way.

I mean, the use case of track_toast is pretty limited anyway; you'll 
probably turn this feature on with a specific column in mind, of which 
you'll probably know which compression method is used at the time.

Thanks for the feedback!
v7 attached.
-- 
Gunnar "Nick" Bluth

Eimermacherweg 106
D-48159 Münster

Mobil +49 172 8853339
Email: gunnar.bluth@pro-open.de
__________________________________________________________________________
"Ceterum censeo SystemD esse delendam" - Cato