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