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
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.
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.)
> @@ -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.
> +/* ----------
> + * 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. 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.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Para tener más hay que desear menos"