On Thu, Mar 10, 2022 at 2:04 PM davinder singh <davindersingh2...@gmail.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
