Re: Optimize external TOAST storage

On Thu, Mar 10, 2022 at 2:04 PM davinder singh
<davindersingh2692(at)gmail(dot)com> wrote:
>
> Thanks Dilip, I have fixed your comments, please find the updated patch.
>

Some more comments

+ /*
+ * For those cases where storing compressed data is not optimal,
We will use
+ * this pointer copy for referring uncompressed data.
+ */
+ memcpy(toast_attr_copy, toast_attr, sizeof(toast_attr));
+ memcpy(toast_values_copy, toast_values, sizeof(toast_values));

I think we can change the comments like below

"Preserve references to the original uncompressed data before
attempting the compression. So that during externalize if we decide
not to store the compressed data then we have the original data with
us. For more details refer to comments atop
toast_tuple_opt_externalize".

+/*
+ * Don't save compressed data to external storage unless it saves I/O while
+ * accessing the same data by reducing the number of chunks.
+ */
+void
+toast_tuple_opt_externalize(ToastTupleContext *ttc, int attribute, int options,
+ Datum orig_toast_value, ToastAttrInfo *orig_attr)

I think these comments are not explaining what is the problem with
storing the compressed data. So you need to expand this comment
saying if it is not reducing the number or chunks then there is no
point in storing the compressed data because then we will have
additional decompression cost whenever we are fetching that data.

+
+ /* 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);

I think we don't need "Sanity check:" here, the remaining part of the
comment is fine.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com