Re: [PATCH] pg_stat_toast v6

Am 03.01.22 um 22:03 schrieb Justin Pryzby:
>> +pgstat_report_toast_activity(Oid relid, int attr,
>> +                            bool externalized,
>> +                            bool compressed,
>> +                            int32 old_size,
>> +                            int32 new_size,
> ...
>> +        if (new_size)
>> +        {
>> +            htabent->t_counts.t_size_orig+=old_size;
>> +            if (new_size)
>> +            {
> 
> I guess one of these is supposed to say old_size?

Didn't make a difference, tbth, as they'd both be 0 or have a value. 
Streamlined the whole block now.


>> +CREATE TABLE toast_test (cola TEXT, colb TEXT COMPRESSION lz4, colc TEXT , cold TEXT, cole TEXT);
> 
> Is there a reason this uses lz4 ?

I thought it might help later on, but alas! the LZ4 column mainly broke 
things, so I removed it for the time being.

> If that's needed for stable results, I think you should use pglz, since that's
> what's guaranteed to exist.  I imagine LZ4 won't be required any time soon,
> seeing as zlib has never been required.

Yeah. It didn't prove anything whatsoever.

>> +        Be aware that this feature, depending on the amount of TOASTable columns in
>> +        your databases, may significantly increase the size of the statistics files
>> +        and the workload of the statistics collector. It is recommended to only
>> +        temporarily activate this to assess the right compression and storage method
>> +        for (a) column(s).
> 
> saying "a column" is fine

Changed.

> 
>> +       <structfield>schemaname</structfield> <type>name</type>
>> +       Attribute (column) number in the relation
>> +       <structfield>relname</structfield> <type>name</type>
> 
>> +      <entry role="catalog_table_entry"><para role="column_definition">
>> +       <structfield>compressmethod</structfield> <type>char</type>
>> +      </para>
>> +      <para>
>> +       Compression method of the attribute (empty means default)
> 
> One thing to keep in mind is that the current compression method is only used
> for *new* data - old data can still use the old compression method.  It
> probably doesn't need to be said here, but maybe you can refer to the docs
> about that in alter_table.
> 
>> +       Number of times the compression was successful (gained a size reduction)
> 
> It's more clear to say "was reduced in size"

Changed the wording a bit, I guess it is clear enough now.
The question is if the column should be there at all, as it's simply 
fetched from pg_attribute...

> 
>> +    /* we assume this inits to all zeroes: */
>> +    static const PgStat_ToastCounts all_zeroes;
> 
> You don't have to assume; static/global allocations are always zero unless
> otherwise specified.

Copy-pasta ;-)
Removed.

Thx for looking into this!
Patch v7 will be in the next mail.

-- 
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