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

On Tue, Apr 02, 2019 at 02:35:19PM +0900, Michael Paquier wrote:
> +   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.

I have been looking at this patch more, and here are some notes:
- The tests can be really simplified using directly reltoastrelid, so
I changed the queries this way.  I am aware that the surroundings
hardcode directly the relation name, but that's not really elegant in
my opinion.  And I am really tempted to adjust these as well to
directly use reltoastrelid.
- The docs had a weird indentation.
- I think that we should be careful with the portability of
pg_column_size(), so I have added comparisons instead of the direct
values in a way which does not change the meaning of the tests nor
their coverage.
- Having RelationGetCompressTupleTarget use directly
toast_tuple_threshold as default argument is I think kind of
confusing, so let's use a different static value, named
COMPRESS_TUPLE_TARGET in the attached.  This is similar to
TOAST_TUPLE_TARGET for the toast tuple threshold.
- The comments in tuptoaster.h need to be updated to outline the
difference between the compression invocation and the toast invocation
thresholds.  The wording could be better though.
- Better to avoid comments in the middle of the else/if blocks in my
opinion.

Also, the previous versions of the patch do that when doing a heap
insertion (heapam.c and rewriteheap.c):
+    toast_tuple_threshold = RelationGetToastTupleTarget(relation,
+                                    TOAST_TUPLE_THRESHOLD);
+        compress_tuple_threshold = RelationGetCompressTupleTarget(relation,
+                                    toast_tuple_threshold);
+        compress_tuple_threshold = Min(compress_tuple_threshold,
+                                       toast_tuple_threshold);
[...]
    need_toast = (HeapTupleHasExternal(&oldtup) ||
                  HeapTupleHasExternal(newtup) ||
-                newtup->t_len > TOAST_TUPLE_THRESHOLD);
+                newtup->t_len > compress_tuple_threshold);

This means that the original code always uses the compilation-time,
default value of toast_tuple_target for all relations.  But this gets
changed so as we would use the value set at relation level for
toast_tuple_target if the reloption is changed, without touching
compress_tuple_threshold.  This is out of the scope of this patch, but
shouldn't we always use the relation-level value instead of the
compiled one?  Perhaps there is something I am missing?
--
Michael