On Wed, Apr 03, 2019 at 12:13:51PM -0400, Robert Haas wrote: > On Wed, Apr 3, 2019 at 2:38 AM Michael Paquier <mich...@paquier.xyz> wrote: >> Shouldn't we use the reloption instead of the compiled threshold to >> determine if a tuple should be toasted or not? Perhaps I am missing >> something? It seems to me that this is a bug that should be >> back-patched, but it could also be qualified as a behavior change for >> existing relations. > > Could you explain a bit more clearly what you think the bug is?
I mean that toast_tuple_target is broken as-is, because it should be
used on the new tuples of a relation as a threshold to decide if this
tuple should be toasted or not, but we don't actually use the
reloption value for that decision-making: the default threshold
TOAST_TUPLE_THRESHOLD gets used instead all the time! The code does
not even create a toast table in some cases.
You may want to look at the attached patch if those words make little
sense as code may be easier to explain than words here. Here is also
a simple example:
CREATE TABLE toto (b text) WITH (toast_tuple_target = 1024);
INSERT INTO toto SELECT string_agg('', md5(random()::text))
FROM generate_series(1,10); -- 288 bytes
SELECT pg_relation_size(reltoastrelid) = 0 AS is_empty FROM
pg_class where relname = 'toto';
INSERT INTO toto SELECT string_agg('', md5(random()::text))
FROM generate_series(1,40); -- 1248 bytes
SELECT pg_relation_size(reltoastrelid) = 0 AS is_empty FROM
pg_class where relname = 'toto';
On HEAD, the second INSERT does *not* toast the tuple (is_empty is
true). With the patch attached, the second INSERT toasts the tuple as
I would expect (is_empty is *false*) per the custom threshold
defined.
While on it, I think that the extra argument for
RelationGetToastTupleTarget() is useless as the default value should
be TOAST_TUPLE_THRESHOLD all the time.
Does this make sense?
--
Michael
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 05ceb6550d..8bd544e269 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -2080,6 +2080,8 @@ heap_prepare_insert(Relation relation, HeapTuple tup, TransactionId xid,
/*
* If the new tuple is too big for storage or contains already toasted
* out-of-line attributes from some other relation, invoke the toaster.
+ * The minimum tuple length checking if a tuple can be toasted can be
+ * specified using the relation option toast_tuple_target.
*/
if (relation->rd_rel->relkind != RELKIND_RELATION &&
relation->rd_rel->relkind != RELKIND_MATVIEW)
@@ -2088,7 +2090,8 @@ heap_prepare_insert(Relation relation, HeapTuple tup, TransactionId xid,
Assert(!HeapTupleHasExternal(tup));
return tup;
}
- else if (HeapTupleHasExternal(tup) || tup->t_len > TOAST_TUPLE_THRESHOLD)
+ else if (HeapTupleHasExternal(tup) ||
+ tup->t_len > RelationGetToastTupleTarget(relation))
return toast_insert_or_update(relation, tup, NULL, options);
else
return tup;
@@ -3392,7 +3395,7 @@ l2:
else
need_toast = (HeapTupleHasExternal(&oldtup) ||
HeapTupleHasExternal(newtup) ||
- newtup->t_len > TOAST_TUPLE_THRESHOLD);
+ newtup->t_len > RelationGetToastTupleTarget(relation));
pagefree = PageGetHeapFreeSpace(page);
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index bce4274362..f5fd54ef37 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -640,6 +640,9 @@ raw_heap_insert(RewriteState state, HeapTuple tup)
/*
* If the new tuple is too big for storage or contains already toasted
* out-of-line attributes from some other relation, invoke the toaster.
+ * The minimum tuple length checking if a tuple can be toasted can be
+ * specified using the relation option toast_tuple_target, so use it
+ * if specified.
*
* Note: below this point, heaptup is the data we actually intend to store
* into the relation; tup is the caller's original untoasted data.
@@ -650,7 +653,8 @@ raw_heap_insert(RewriteState state, HeapTuple tup)
Assert(!HeapTupleHasExternal(tup));
heaptup = tup;
}
- else if (HeapTupleHasExternal(tup) || tup->t_len > TOAST_TUPLE_THRESHOLD)
+ else if (HeapTupleHasExternal(tup) ||
+ tup->t_len > RelationGetToastTupleTarget(state->rs_new_rel))
{
int options = HEAP_INSERT_SKIP_FSM;
diff --git a/src/backend/access/heap/tuptoaster.c b/src/backend/access/heap/tuptoaster.c
index 74e957abb7..60d37829c1 100644
--- a/src/backend/access/heap/tuptoaster.c
+++ b/src/backend/access/heap/tuptoaster.c
@@ -729,7 +729,7 @@ toast_insert_or_update(Relation rel, HeapTuple newtup, HeapTuple oldtup,
hoff += BITMAPLEN(numAttrs);
hoff = MAXALIGN(hoff);
/* now convert to a limit on the tuple data size */
- maxDataLen = RelationGetToastTupleTarget(rel, TOAST_TUPLE_TARGET) - hoff;
+ maxDataLen = RelationGetToastTupleTarget(rel) - hoff;
/*
* Look for attributes with attstorage 'x' to compress. Also find large
diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index 2276d3c5d3..cfa46275af 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -388,8 +388,9 @@ create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
/*
* Check to see whether the table needs a TOAST table. It does only if
* (1) there are any toastable attributes, and (2) the maximum length
- * of a tuple could exceed TOAST_TUPLE_THRESHOLD. (We don't want to
- * create a toast table for something like "f1 varchar(20)".)
+ * of a tuple could exceed TOAST_TUPLE_THRESHOLD or the relation option
+ * toast_tuple_target. (We don't want to create a toast table for
+ * something like "f1 varchar(20)".)
*/
static bool
needs_toast_table(Relation rel)
@@ -457,5 +458,5 @@ needs_toast_table(Relation rel)
tuple_length = MAXALIGN(SizeofHeapTupleHeader +
BITMAPLEN(tupdesc->natts)) +
MAXALIGN(data_length);
- return (tuple_length > TOAST_TUPLE_THRESHOLD);
+ return (tuple_length > RelationGetToastTupleTarget(rel));
}
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index 54028515a7..416b81c07c 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -275,9 +275,9 @@ typedef struct StdRdOptions
* RelationGetToastTupleTarget
* Returns the relation's toast_tuple_target. Note multiple eval of argument!
*/
-#define RelationGetToastTupleTarget(relation, defaulttarg) \
+#define RelationGetToastTupleTarget(relation) \
((relation)->rd_options ? \
- ((StdRdOptions *) (relation)->rd_options)->toast_tuple_target : (defaulttarg))
+ ((StdRdOptions *) (relation)->rd_options)->toast_tuple_target : (TOAST_TUPLE_TARGET))
/*
* RelationGetFillFactor
signature.asc
Description: PGP signature
