From f0d531a82abae9c9c40fdae61d598d87ce303774 Mon Sep 17 00:00:00 2001 From: Dilip Kumar Date: Mon, 15 Mar 2021 15:57:45 +0530 Subject: [PATCH v35 6/7] Disallow compressed data inside container types Currently, we have a general rule that Datums of container types (rows, arrays, ranges, etc) must not contain any external TOAST pointers. But the rule for the compressed data is not defined and no specific rule is followed e.g. while constructing the array we decompress the compressed field but while constructing the row in some cases we don't decompress the compressed data whereas in the other cases we only decompress while flattening the external toast pointers. This patch make a general rule for the compressed data i.e. we don't allow the compressed data in the container type. Dilip Kumar based on idea from Robert Haas --- src/backend/access/common/heaptuple.c | 13 +++--- src/backend/access/heap/heaptoast.c | 4 +- src/backend/executor/execExprInterp.c | 10 ++--- src/backend/executor/execTuples.c | 4 -- src/backend/utils/adt/expandedrecord.c | 78 ++++++++++++---------------------- src/backend/utils/adt/jsonfuncs.c | 5 +-- src/include/funcapi.h | 4 +- src/pl/plpgsql/src/pl_exec.c | 11 +++-- 8 files changed, 50 insertions(+), 79 deletions(-) diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c index c36c283..12f6fdd 100644 --- a/src/backend/access/common/heaptuple.c +++ b/src/backend/access/common/heaptuple.c @@ -984,15 +984,12 @@ Datum heap_copy_tuple_as_datum(HeapTuple tuple, TupleDesc tupleDesc) { /* - * If the tuple contains any external TOAST pointers, we have to inline - * those fields to meet the conventions for composite-type Datums. + * We have to inline any external/compressed data to meet the conventions + * for composite-type Datums. */ - if (HeapTupleHasExternal(tuple)) - return toast_flatten_tuple_to_datum(tuple->t_data, - tuple->t_len, - tupleDesc); - else - return heap_copy_tuple_as_raw_datum(tuple, tupleDesc); + return toast_flatten_tuple_to_datum(tuple->t_data, + tuple->t_len, + tupleDesc); } /* ---------------- diff --git a/src/backend/access/heap/heaptoast.c b/src/backend/access/heap/heaptoast.c index 55bbe1d..b094623 100644 --- a/src/backend/access/heap/heaptoast.c +++ b/src/backend/access/heap/heaptoast.c @@ -589,9 +589,9 @@ toast_build_flattened_tuple(TupleDesc tupleDesc, struct varlena *new_value; new_value = (struct varlena *) DatumGetPointer(new_values[i]); - if (VARATT_IS_EXTERNAL(new_value)) + if (VARATT_IS_EXTERNAL(new_value) || VARATT_IS_COMPRESSED(new_value)) { - new_value = detoast_external_attr(new_value); + new_value = detoast_attr(new_value); new_values[i] = PointerGetDatum(new_value); freeable_values[num_to_free++] = (Pointer) new_value; } diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c index 772ab11..f8ed1fa 100644 --- a/src/backend/executor/execExprInterp.c +++ b/src/backend/executor/execExprInterp.c @@ -2840,13 +2840,12 @@ ExecEvalRow(ExprState *state, ExprEvalStep *op) { HeapTuple tuple; - /* detoast any external data before forming the tuple */ + /* detoast any external/compressed data before forming the tuple */ for (int i = 0; i < op->d.row.tupdesc->natts; i++) { Form_pg_attribute attr = TupleDescAttr(op->d.row.tupdesc, i); - if (op->d.row.elemnulls[i] || attr->attlen != -1 || - !VARATT_IS_EXTERNAL(DatumGetPointer(op->d.row.elemvalues[i]))) + if (op->d.row.elemnulls[i] || attr->attlen != -1) continue; op->d.row.elemvalues[i] = @@ -3098,13 +3097,12 @@ ExecEvalFieldStoreForm(ExprState *state, ExprEvalStep *op, ExprContext *econtext { HeapTuple tuple; - /* detoast any external data before forming the tuple */ + /* detoast any external/compressed data before forming the tuple */ for (int i = 0; i < (*op->d.fieldstore.argdesc)->natts; i++) { Form_pg_attribute attr = TupleDescAttr(*op->d.fieldstore.argdesc, i); - if (op->d.fieldstore.nulls[i] || attr->attlen != -1 || - !VARATT_IS_EXTERNAL(DatumGetPointer(op->d.fieldstore.values[i]))) + if (op->d.fieldstore.nulls[i] || attr->attlen != -1) continue; op->d.fieldstore.values[i] = PointerGetDatum(PG_DETOAST_DATUM_PACKED(op->d.fieldstore.values[i])); diff --git a/src/backend/executor/execTuples.c b/src/backend/executor/execTuples.c index 73c35df..f115464 100644 --- a/src/backend/executor/execTuples.c +++ b/src/backend/executor/execTuples.c @@ -2207,10 +2207,6 @@ HeapTupleHeaderGetDatum(HeapTupleHeader tuple) Datum result; TupleDesc tupDesc; - /* No work if there are no external TOAST pointers in the tuple */ - if (!HeapTupleHeaderHasExternal(tuple)) - return PointerGetDatum(tuple); - /* Use the type data saved by heap_form_tuple to look up the rowtype */ tupDesc = lookup_rowtype_tupdesc(HeapTupleHeaderGetTypeId(tuple), HeapTupleHeaderGetTypMod(tuple)); diff --git a/src/backend/utils/adt/expandedrecord.c b/src/backend/utils/adt/expandedrecord.c index e19491e..765d58c 100644 --- a/src/backend/utils/adt/expandedrecord.c +++ b/src/backend/utils/adt/expandedrecord.c @@ -673,14 +673,6 @@ ER_get_flat_size(ExpandedObjectHeader *eohptr) erh->er_typmod = tupdesc->tdtypmod; } - /* - * If we have a valid flattened value without out-of-line fields, we can - * just use it as-is. - */ - if (erh->flags & ER_FLAG_FVALUE_VALID && - !(erh->flags & ER_FLAG_HAVE_EXTERNAL)) - return erh->fvalue->t_len; - /* If we have a cached size value, believe that */ if (erh->flat_size) return erh->flat_size; @@ -693,38 +685,36 @@ ER_get_flat_size(ExpandedObjectHeader *eohptr) tupdesc = erh->er_tupdesc; /* - * Composite datums mustn't contain any out-of-line values. + * Composite datums mustn't contain any out-of-line/compressed values. */ - if (erh->flags & ER_FLAG_HAVE_EXTERNAL) + for (i = 0; i < erh->nfields; i++) { - for (i = 0; i < erh->nfields; i++) - { - Form_pg_attribute attr = TupleDescAttr(tupdesc, i); + Form_pg_attribute attr = TupleDescAttr(tupdesc, i); - if (!erh->dnulls[i] && - !attr->attbyval && attr->attlen == -1 && - VARATT_IS_EXTERNAL(DatumGetPointer(erh->dvalues[i]))) - { - /* - * expanded_record_set_field_internal can do the actual work - * of detoasting. It needn't recheck domain constraints. - */ - expanded_record_set_field_internal(erh, i + 1, - erh->dvalues[i], false, - true, - false); - } + if (!erh->dnulls[i] && + !attr->attbyval && attr->attlen == -1 && + (VARATT_IS_EXTERNAL(DatumGetPointer(erh->dvalues[i])) || + VARATT_IS_COMPRESSED(DatumGetPointer(erh->dvalues[i])))) + { + /* + * expanded_record_set_field_internal can do the actual work of + * detoasting. It needn't recheck domain constraints. + */ + expanded_record_set_field_internal(erh, i + 1, + erh->dvalues[i], false, + true, + false); } - - /* - * We have now removed all external field values, so we can clear the - * flag about them. This won't cause ER_flatten_into() to mistakenly - * take the fast path, since expanded_record_set_field() will have - * cleared ER_FLAG_FVALUE_VALID. - */ - erh->flags &= ~ER_FLAG_HAVE_EXTERNAL; } + /* + * We have now removed all external field values, so we can clear the flag + * about them. This won't cause ER_flatten_into() to mistakenly take the + * fast path, since expanded_record_set_field() will have cleared + * ER_FLAG_FVALUE_VALID. + */ + erh->flags &= ~ER_FLAG_HAVE_EXTERNAL; + /* Test if we currently have any null values */ hasnull = false; for (i = 0; i < erh->nfields; i++) @@ -770,19 +760,6 @@ ER_flatten_into(ExpandedObjectHeader *eohptr, Assert(erh->er_magic == ER_MAGIC); - /* Easy if we have a valid flattened value without out-of-line fields */ - if (erh->flags & ER_FLAG_FVALUE_VALID && - !(erh->flags & ER_FLAG_HAVE_EXTERNAL)) - { - Assert(allocated_size == erh->fvalue->t_len); - memcpy(tuphdr, erh->fvalue->t_data, allocated_size); - /* The original flattened value might not have datum header fields */ - HeapTupleHeaderSetDatumLength(tuphdr, allocated_size); - HeapTupleHeaderSetTypeId(tuphdr, erh->er_typeid); - HeapTupleHeaderSetTypMod(tuphdr, erh->er_typmod); - return; - } - /* Else allocation should match previous get_flat_size result */ Assert(allocated_size == erh->flat_size); @@ -1151,15 +1128,16 @@ expanded_record_set_field_internal(ExpandedRecordHeader *erh, int fnumber, { MemoryContext oldcxt; - /* If requested, detoast any external value */ + /* If requested, detoast any external/compressed value */ if (expand_external) { if (attr->attlen == -1 && - VARATT_IS_EXTERNAL(DatumGetPointer(newValue))) + (VARATT_IS_EXTERNAL(DatumGetPointer(newValue)) || + VARATT_IS_COMPRESSED(DatumGetPointer(newValue)))) { /* Detoasting should be done in short-lived context. */ oldcxt = MemoryContextSwitchTo(get_short_term_cxt(erh)); - newValue = PointerGetDatum(detoast_external_attr((struct varlena *) DatumGetPointer(newValue))); + newValue = PointerGetDatum(detoast_attr((struct varlena *) DatumGetPointer(newValue))); MemoryContextSwitchTo(oldcxt); } else diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c index dc3f9d4..cccd675 100644 --- a/src/backend/utils/adt/jsonfuncs.c +++ b/src/backend/utils/adt/jsonfuncs.c @@ -3388,9 +3388,8 @@ populate_record(TupleDesc tupdesc, &field, &nulls[i]); - /* detoast any external data before forming the tuple */ - if (!nulls[i] && att->attlen == -1 && - VARATT_IS_EXTERNAL(DatumGetPointer(values[i]))) + /* detoast any external/compressed data before forming the tuple */ + if (!nulls[i] && att->attlen == -1) values[i] = PointerGetDatum(PG_DETOAST_DATUM_PACKED(values[i])); } diff --git a/src/include/funcapi.h b/src/include/funcapi.h index 8ba7ae2..c869012 100644 --- a/src/include/funcapi.h +++ b/src/include/funcapi.h @@ -208,10 +208,10 @@ extern TupleDesc build_function_result_tupdesc_t(HeapTuple procTuple); * Macro declarations/inline functions: * HeapTupleHeaderGetRawDatum(HeapTupleHeader tuple) - same as * HeapTupleHeaderGetDatum but the input tuple should not contain - * external varlena + * external/compressed varlena * HeapTupleGetDatum(HeapTuple tuple) - convert a HeapTuple to a Datum. * HeapTupleGetRawDatum(HeapTuple tuple) - same as HeapTupleGetDatum - * but the input tuple should not contain external varlena + * but the input tuple should not contain external/compressed varlena * * Obsolete routines and macros: * TupleDesc RelationNameGetTupleDesc(const char *relname) - Use to get a diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index d4fcb70..af2c85f 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -7267,7 +7267,8 @@ compatible_tupdescs(TupleDesc src_tupdesc, TupleDesc dst_tupdesc) * The result tuple is freshly palloc'd in caller's context. Some junk * may be left behind in eval_mcontext, too. * - * flatten - if this is passed true then any external data will be flattened. + * flatten - if this is passed true then any external/compressed data will be + * flattened. * ---------- */ static HeapTuple @@ -7305,9 +7306,11 @@ make_tuple_from_row(PLpgSQL_execstate *estate, if (fieldtypeid != TupleDescAttr(tupdesc, i)->atttypid) return NULL; - /* detoast any external data if the caller has asked to do so */ - if (flatten && !nulls[i] && TupleDescAttr(tupdesc, i)->attlen == -1 && - VARATT_IS_EXTERNAL(DatumGetPointer(dvalues[i]))) + /* + * detoast any external/compressed data if the caller has asked to do + * so. + */ + if (flatten && !nulls[i] && TupleDescAttr(tupdesc, i)->attlen == -1) dvalues[i] = PointerGetDatum(PG_DETOAST_DATUM_PACKED(dvalues[i])); /* XXX should we insist on typmod match, too? */ } -- 1.8.3.1