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(dot)bluth(at)pro-open(dot)de
__________________________________________________________________________
"Ceterum censeo SystemD esse delendam" - Cato