Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

[Michael Paquier]

On Fri, May 21, 2021 at 02:19:29PM -0700, Andres Freund wrote:
> During VACUUM FULL reform_and_rewrite_tuple() detoasts the old value if
> it was compressed with a different method, while in
> TopTransactionContext. There's nothing freeing that until
> TopTransactionContext ends - obviously not great for a large relation
> being VACUUM FULLed.

Yeah, that's not good.  The confusion comes from the fact that we'd
just overwrite the values without freeing them out if recompressed, so
something like the attached would be fine?
--
Michael

diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 61d9044816..ba8563af22 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2463,12 +2463,19 @@ reform_and_rewrite_tuple(HeapTuple tuple,
 	TupleDesc	newTupDesc = RelationGetDescr(NewHeap);
 	HeapTuple	copiedTuple;
 	int			i;
+	Datum		tup_values[MaxTupleAttributeNumber];
+	bool		values_free[MaxTupleAttributeNumber];
+
+	memset(tup_values, 0, newTupDesc->natts * sizeof(Datum));
+	memset(values_free, 0, newTupDesc->natts * sizeof(bool));
 
 	heap_deform_tuple(tuple, oldTupDesc, values, isnull);
 
 	/* Be sure to null out any dropped columns */
 	for (i = 0; i < newTupDesc->natts; i++)
 	{
+		tup_values[i] = values[i];
+
 		if (TupleDescAttr(newTupDesc, i)->attisdropped)
 			isnull[i] = true;
 
@@ -2507,15 +2514,25 @@ reform_and_rewrite_tuple(HeapTuple tuple,
 
 			/* if compression method doesn't match then detoast the value */
 			if (TupleDescAttr(newTupDesc, i)->attcompression != cmethod)
-				values[i] = PointerGetDatum(detoast_attr(new_value));
+			{
+				tup_values[i] = PointerGetDatum(detoast_attr(new_value));
+				values_free[i] = true;
+			}
 		}
 	}
 
-	copiedTuple = heap_form_tuple(newTupDesc, values, isnull);
+	copiedTuple = heap_form_tuple(newTupDesc, tup_values, isnull);
 
 	/* The heap rewrite module does the rest */
 	rewrite_heap_tuple(rwstate, tuple, copiedTuple);
 
+	/* Free any detoasted value allocated if recompressed */
+	for (i = 0; i < newTupDesc->natts; i++)
+	{
+		if (values_free[i])
+			pfree(DatumGetPointer(tup_values[i]));
+	}
+
 	heap_freetuple(copiedTuple);
 }
 

signature.asc
Description: PGP signature