Re: Optimize external TOAST storage - Mailing list pgsql-hackers

Do you think it is worth making this configurable?  I don't think it is
outside the realm of possibility for some users to care more about disk
space than read performance.  And maybe the threshold for this optimization
could differ based on the workload.

Regarding the 2nd part of configuring the threshold, Based on our
experiments, we have fixed it for the attributes with size > 2 * chunk_size.
The default chunk_size is 2KB and the page size is 8KB. While toasting each
attribute is divided into chunks, and each page can hold a max of 4 such chunks.
We only need to think about the space used by the last chunk of the attribute.
This means with each value optimization, it might use extra space in the range
(0B,2KB]. I think this extra space is independent of attribute size. So we don't
need to worry about configuring this threshold. Let me know if I missed something

here.

 extern void toast_tuple_externalize(ToastTupleContext *ttc, int attribute,
                                     int options);
+extern void toast_tuple_opt_externalize(ToastTupleContext *ttc, int attribute,
+                                        int options, Datum old_toast_value,
+                                        ToastAttrInfo *old_toast_attr);

Could we bake this into toast_tuple_externalize() so that all existing
callers benefit from this optimization?  Is there a reason to export both
functions?  Perhaps toast_tuple_externalize() should be renamed and made
static, and then this new function could be called
toast_tuple_externalize() (which would be a wrapper around the internal
function).

+    /* Sanity check: if data is not compressed then we can proceed as usual. */
+    if (!VARATT_IS_COMPRESSED(DatumGetPointer(*value)))
+        toast_tuple_externalize(ttc, attribute, options);

With a --enable-cassert build, this line causes assertion failures in the
call to GetMemoryChunkContext() from pfree().  'make check' is enough to
reproduce it.  Specifically, it fails the following assertion:

        AssertArg(MemoryContextIsValid(context));

+    /* If data is not compressed then we can proceed as usual. */
+    if (!VARATT_IS_COMPRESSED(DatumGetPointer(*value)))

v1 patch didn't have this problem because it was verifying whether we have

compressed data in this toasting round or not. I have changed back to the

earlier version.

+    /* If data is not compressed then we can proceed as usual. */

+    if (*value == orig_toast_value)