|From:||"Gunnar \"Nick\" Bluth" <gunnar(dot)bluth(at)pro-open(dot)de>|
|To:||Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>|
|Cc:||pgsql-hackers(at)lists(dot)postgresql(dot)org, "kuroda(dot)hayato(at)fujitsu(dot)com" <kuroda(dot)hayato(at)fujitsu(dot)com>, Justin Pryzby <pryzby(at)telsasoft(dot)com>|
|Subject:||Re: [PATCH] pg_stat_toast v6|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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.
> I didn't look into the patch carefully, but here are some minor
> 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!
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_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?
>> +-- 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
> 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!
Gunnar "Nick" Bluth
Mobil +49 172 8853339
"Ceterum censeo SystemD esse delendam" - Cato
|Next Messagefirstname.lastname@example.org||2022-01-04 11:47:26||RE: Failed transaction statistics to measure the logical replication progress|
|Previous Message||Gunnar "Nick" Bluth||2022-01-04 11:11:23||Re: [PATCH] pg_stat_toast v6|