Re: Re: A separate table level option to control compression

On Tue, Apr 02, 2019 at 11:37:56AM +0900, Masahiko Sawada wrote:
> Marked.

+   compress_tuple_threshold = RelationGetCompressTupleTarget(relation,
+                               toast_tuple_threshold);
+   compress_tuple_threshold = Min(compress_tuple_threshold,
+                                  toast_tuple_threshold);
All the callers of RelationGetCompressTupleTarget directly compile the
minimum between compress_tuple_threshold and toast_tuple_threshold,
and also call RelationGetToastTupleTarget() beforehand.  Wouldn't it
be better to merge all that in a common routine?  The same calculation
method is duplicated 5 times.

+   /*
+    * Get the limit at which we should apply compression. This will be same as
+    * maxDataLen unless overridden by the user explicitly.
+    */
+   maxCompressLen = RelationGetCompressTupleTarget(rel, maxDataLen) - hoff;

Is that fine?  hoff gets substracted twice.

+     This parameter cannot be set for TOAST tables.
Missing <acronym> markup here for TOAST.

The format of the whole new documentation paragraph is a bit weird.
Could it be possible to adjust it 72-80 characters per line?

The same boundary logic applies to compress_tuple_target and
toast_tuple_target.  Perhaps it would make sense to remove the 4-tuple
limit-per-block from the new paragraph, and mention that the same
limits as for toast_tuple_target apply to compress_tuple_target.

+-- expect 0 blocks
+select 0 = pg_relation_size('pg_toast.pg_toast_'||(select oid from
pg_class where relname =
'compresstest250'))/current_setting('block_size')::integer as blocks;
+select 0 = pg_relation_size('pg_toast.pg_toast_'||(select oid from
pg_class where relname =
'compresstest2040'))/current_setting('block_size')::integer as blocks;

This is unreadable.  Cannot pg_class.reltoastrelid be used instead?
--
Michael