]> git.ipfire.org Git - thirdparty/postgresql.git/commitdiff
Redesign the caching done by get_cached_rowtype().
authorTom Lane <tgl@sss.pgh.pa.us>
Tue, 13 Apr 2021 17:37:07 +0000 (13:37 -0400)
committerTom Lane <tgl@sss.pgh.pa.us>
Tue, 13 Apr 2021 17:37:07 +0000 (13:37 -0400)
Previously, get_cached_rowtype() cached a pointer to a reference-counted
tuple descriptor from the typcache, relying on the ExprContextCallback
mechanism to release the tupdesc refcount when the expression tree
using the tupdesc was destroyed.  This worked fine when it was designed,
but the introduction of within-DO-block COMMITs broke it.  The refcount
is logged in a transaction-lifespan resource owner, but plpgsql won't
destroy simple expressions made within the DO block (before its first
commit) until the DO block is exited.  That results in a warning about
a leaked tupdesc refcount when the COMMIT destroys the original resource
owner, and then an error about the active resource owner not holding a
matching refcount when the expression is destroyed.

To fix, get rid of the need to have a shutdown callback at all, by
instead caching a pointer to the relevant typcache entry.  Those
survive for the life of the backend, so we needn't worry about the
pointer becoming stale.  (For registered RECORD types, we can still
cache a pointer to the tupdesc, knowing that it won't change for the
life of the backend.)  This mechanism has been in use in plpgsql
and expandedrecord.c since commit 4b93f5799, and seems to work well.

This change requires modifying the ExprEvalStep structs used by the
relevant expression step types, which is slightly worrisome for
back-patching.  However, there seems no good reason for extensions
to be familiar with the details of these particular sub-structs.

Per report from Rohit Bhogate.  Back-patch to v11 where within-DO-block
COMMITs became a thing.

Discussion: https://postgr.es/m/CAAV6ZkQRCVBh8qAY+SZiHnz+U+FqAGBBDaDTjF2yiKa2nJSLKg@mail.gmail.com

src/backend/executor/execExpr.c
src/backend/executor/execExprInterp.c
src/include/executor/execExpr.h
src/pl/plpgsql/src/expected/plpgsql_transaction.out
src/pl/plpgsql/src/sql/plpgsql_transaction.sql

index 6dea65f2c02a8583f624eece592f5987538f1e12..6ad6214c956f87593c859aa31f81295507cca20b 100644 (file)
@@ -1129,7 +1129,7 @@ ExecInitExprRec(Expr *node, ExprState *state,
                                scratch.opcode = EEOP_FIELDSELECT;
                                scratch.d.fieldselect.fieldnum = fselect->fieldnum;
                                scratch.d.fieldselect.resulttype = fselect->resulttype;
-                               scratch.d.fieldselect.argdesc = NULL;
+                               scratch.d.fieldselect.rowcache.cacheptr = NULL;
 
                                ExprEvalPushStep(state, &scratch);
                                break;
@@ -1139,7 +1139,7 @@ ExecInitExprRec(Expr *node, ExprState *state,
                        {
                                FieldStore *fstore = (FieldStore *) node;
                                TupleDesc       tupDesc;
-                               TupleDesc  *descp;
+                               ExprEvalRowtypeCache *rowcachep;
                                Datum      *values;
                                bool       *nulls;
                                int                     ncolumns;
@@ -1155,9 +1155,9 @@ ExecInitExprRec(Expr *node, ExprState *state,
                                values = (Datum *) palloc(sizeof(Datum) * ncolumns);
                                nulls = (bool *) palloc(sizeof(bool) * ncolumns);
 
-                               /* create workspace for runtime tupdesc cache */
-                               descp = (TupleDesc *) palloc(sizeof(TupleDesc));
-                               *descp = NULL;
+                               /* create shared composite-type-lookup cache struct */
+                               rowcachep = palloc(sizeof(ExprEvalRowtypeCache));
+                               rowcachep->cacheptr = NULL;
 
                                /* emit code to evaluate the composite input value */
                                ExecInitExprRec(fstore->arg, state, resv, resnull);
@@ -1165,7 +1165,7 @@ ExecInitExprRec(Expr *node, ExprState *state,
                                /* next, deform the input tuple into our workspace */
                                scratch.opcode = EEOP_FIELDSTORE_DEFORM;
                                scratch.d.fieldstore.fstore = fstore;
-                               scratch.d.fieldstore.argdesc = descp;
+                               scratch.d.fieldstore.rowcache = rowcachep;
                                scratch.d.fieldstore.values = values;
                                scratch.d.fieldstore.nulls = nulls;
                                scratch.d.fieldstore.ncolumns = ncolumns;
@@ -1222,7 +1222,7 @@ ExecInitExprRec(Expr *node, ExprState *state,
                                /* finally, form result tuple */
                                scratch.opcode = EEOP_FIELDSTORE_FORM;
                                scratch.d.fieldstore.fstore = fstore;
-                               scratch.d.fieldstore.argdesc = descp;
+                               scratch.d.fieldstore.rowcache = rowcachep;
                                scratch.d.fieldstore.values = values;
                                scratch.d.fieldstore.nulls = nulls;
                                scratch.d.fieldstore.ncolumns = ncolumns;
@@ -1368,17 +1368,24 @@ ExecInitExprRec(Expr *node, ExprState *state,
                case T_ConvertRowtypeExpr:
                        {
                                ConvertRowtypeExpr *convert = (ConvertRowtypeExpr *) node;
+                               ExprEvalRowtypeCache *rowcachep;
+
+                               /* cache structs must be out-of-line for space reasons */
+                               rowcachep = palloc(2 * sizeof(ExprEvalRowtypeCache));
+                               rowcachep[0].cacheptr = NULL;
+                               rowcachep[1].cacheptr = NULL;
 
                                /* evaluate argument into step's result area */
                                ExecInitExprRec(convert->arg, state, resv, resnull);
 
                                /* and push conversion step */
                                scratch.opcode = EEOP_CONVERT_ROWTYPE;
-                               scratch.d.convert_rowtype.convert = convert;
-                               scratch.d.convert_rowtype.indesc = NULL;
-                               scratch.d.convert_rowtype.outdesc = NULL;
+                               scratch.d.convert_rowtype.inputtype =
+                                       exprType((Node *) convert->arg);
+                               scratch.d.convert_rowtype.outputtype = convert->resulttype;
+                               scratch.d.convert_rowtype.incache = &rowcachep[0];
+                               scratch.d.convert_rowtype.outcache = &rowcachep[1];
                                scratch.d.convert_rowtype.map = NULL;
-                               scratch.d.convert_rowtype.initialized = false;
 
                                ExprEvalPushStep(state, &scratch);
                                break;
@@ -2013,7 +2020,7 @@ ExecInitExprRec(Expr *node, ExprState *state,
                                                 (int) ntest->nulltesttype);
                                }
                                /* initialize cache in case it's a row test */
-                               scratch.d.nulltest_row.argdesc = NULL;
+                               scratch.d.nulltest_row.rowcache.cacheptr = NULL;
 
                                /* first evaluate argument into result variable */
                                ExecInitExprRec(ntest->arg, state,
index eff69d8b1b62b8e3d624d567d989a6615c5c598b..838ec936c0907e9df14fb530f3305df9c71c7720 100644 (file)
@@ -144,8 +144,8 @@ static void ExecInitInterpreter(void);
 /* support functions */
 static void CheckVarSlotCompatibility(TupleTableSlot *slot, int attnum, Oid vartype);
 static TupleDesc get_cached_rowtype(Oid type_id, int32 typmod,
-                                  TupleDesc *cache_field, ExprContext *econtext);
-static void ShutdownTupleDescRef(Datum arg);
+                                                                       ExprEvalRowtypeCache *rowcache,
+                                                                       bool *changed);
 static void ExecEvalRowNullInt(ExprState *state, ExprEvalStep *op,
                                   ExprContext *econtext, bool checkisnull);
 
@@ -1903,56 +1903,78 @@ CheckVarSlotCompatibility(TupleTableSlot *slot, int attnum, Oid vartype)
  * get_cached_rowtype: utility function to lookup a rowtype tupdesc
  *
  * type_id, typmod: identity of the rowtype
- * cache_field: where to cache the TupleDesc pointer in expression state node
- *             (field must be initialized to NULL)
- * econtext: expression context we are executing in
+ * rowcache: space for caching identity info
+ *             (rowcache->cacheptr must be initialized to NULL)
+ * changed: if not NULL, *changed is set to true on any update
  *
- * NOTE: because the shutdown callback will be called during plan rescan,
- * must be prepared to re-do this during any node execution; cannot call
- * just once during expression initialization.
+ * The returned TupleDesc is not guaranteed pinned; caller must pin it
+ * to use it across any operation that might incur cache invalidation.
+ * (The TupleDesc is always refcounted, so just use IncrTupleDescRefCount.)
+ *
+ * NOTE: because composite types can change contents, we must be prepared
+ * to re-do this during any node execution; cannot call just once during
+ * expression initialization.
  */
 static TupleDesc
 get_cached_rowtype(Oid type_id, int32 typmod,
-                                  TupleDesc *cache_field, ExprContext *econtext)
+                                  ExprEvalRowtypeCache *rowcache,
+                                  bool *changed)
 {
-       TupleDesc       tupDesc = *cache_field;
-
-       /* Do lookup if no cached value or if requested type changed */
-       if (tupDesc == NULL ||
-               type_id != tupDesc->tdtypeid ||
-               typmod != tupDesc->tdtypmod)
+       if (type_id != RECORDOID)
        {
-               tupDesc = lookup_rowtype_tupdesc(type_id, typmod);
+               /*
+                * It's a named composite type, so use the regular typcache.  Do a
+                * lookup first time through, or if the composite type changed.  Note:
+                * "tupdesc_id == 0" may look redundant, but it protects against the
+                * admittedly-theoretical possibility that type_id was RECORDOID the
+                * last time through, so that the cacheptr isn't TypeCacheEntry *.
+                */
+               TypeCacheEntry *typentry = (TypeCacheEntry *) rowcache->cacheptr;
 
-               if (*cache_field)
+               if (unlikely(typentry == NULL ||
+                                        rowcache->tupdesc_id == 0 ||
+                                        typentry->tupDesc_identifier != rowcache->tupdesc_id))
                {
-                       /* Release old tupdesc; but callback is already registered */
-                       ReleaseTupleDesc(*cache_field);
+                       typentry = lookup_type_cache(type_id, TYPECACHE_TUPDESC);
+                       if (typentry->tupDesc == NULL)
+                               ereport(ERROR,
+                                               (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                                                errmsg("type %s is not composite",
+                                                               format_type_be(type_id))));
+                       rowcache->cacheptr = (void *) typentry;
+                       rowcache->tupdesc_id = typentry->tupDesc_identifier;
+                       if (changed)
+                               *changed = true;
                }
-               else
+               return typentry->tupDesc;
+       }
+       else
+       {
+               /*
+                * A RECORD type, once registered, doesn't change for the life of the
+                * backend.  So we don't need a typcache entry as such, which is good
+                * because there isn't one.  It's possible that the caller is asking
+                * about a different type than before, though.
+                */
+               TupleDesc       tupDesc = (TupleDesc) rowcache->cacheptr;
+
+               if (unlikely(tupDesc == NULL ||
+                                        rowcache->tupdesc_id != 0 ||
+                                        type_id != tupDesc->tdtypeid ||
+                                        typmod != tupDesc->tdtypmod))
                {
-                       /* Need to register shutdown callback to release tupdesc */
-                       RegisterExprContextCallback(econtext,
-                                                                               ShutdownTupleDescRef,
-                                                                               PointerGetDatum(cache_field));
+                       tupDesc = lookup_rowtype_tupdesc(type_id, typmod);
+                       /* Drop pin acquired by lookup_rowtype_tupdesc */
+                       ReleaseTupleDesc(tupDesc);
+                       rowcache->cacheptr = (void *) tupDesc;
+                       rowcache->tupdesc_id = 0;       /* not a valid value for non-RECORD */
+                       if (changed)
+                               *changed = true;
                }
-               *cache_field = tupDesc;
+               return tupDesc;
        }
-       return tupDesc;
 }
 
-/*
- * Callback function to release a tupdesc refcount at econtext shutdown
- */
-static void
-ShutdownTupleDescRef(Datum arg)
-{
-       TupleDesc  *cache_field = (TupleDesc *) DatumGetPointer(arg);
-
-       if (*cache_field)
-               ReleaseTupleDesc(*cache_field);
-       *cache_field = NULL;
-}
 
 /*
  * Fast-path functions, for very simple expressions
@@ -2473,8 +2495,7 @@ ExecEvalRowNullInt(ExprState *state, ExprEvalStep *op,
 
        /* Lookup tupdesc if first time through or if type changes */
        tupDesc = get_cached_rowtype(tupType, tupTypmod,
-                                                                &op->d.nulltest_row.argdesc,
-                                                                econtext);
+                                                                &op->d.nulltest_row.rowcache, NULL);
 
        /*
         * heap_attisnull needs a HeapTuple not a bare HeapTupleHeader.
@@ -2910,8 +2931,7 @@ ExecEvalFieldSelect(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
 
                /* Lookup tupdesc if first time through or if type changes */
                tupDesc = get_cached_rowtype(tupType, tupTypmod,
-                                                                        &op->d.fieldselect.argdesc,
-                                                                        econtext);
+                                                                        &op->d.fieldselect.rowcache, NULL);
 
                /*
                 * Find field's attr record.  Note we don't support system columns
@@ -2969,9 +2989,9 @@ ExecEvalFieldStoreDeForm(ExprState *state, ExprEvalStep *op, ExprContext *econte
 {
        TupleDesc       tupDesc;
 
-       /* Lookup tupdesc if first time through or after rescan */
+       /* Lookup tupdesc if first time through or if type changes */
        tupDesc = get_cached_rowtype(op->d.fieldstore.fstore->resulttype, -1,
-                                                                op->d.fieldstore.argdesc, econtext);
+                                                                op->d.fieldstore.rowcache, NULL);
 
        /* Check that current tupdesc doesn't have more fields than we allocated */
        if (unlikely(tupDesc->natts > op->d.fieldstore.ncolumns))
@@ -3013,10 +3033,14 @@ ExecEvalFieldStoreDeForm(ExprState *state, ExprEvalStep *op, ExprContext *econte
 void
 ExecEvalFieldStoreForm(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
 {
+       TupleDesc       tupDesc;
        HeapTuple       tuple;
 
-       /* argdesc should already be valid from the DeForm step */
-       tuple = heap_form_tuple(*op->d.fieldstore.argdesc,
+       /* Lookup tupdesc (should be valid already) */
+       tupDesc = get_cached_rowtype(op->d.fieldstore.fstore->resulttype, -1,
+                                                                op->d.fieldstore.rowcache, NULL);
+
+       tuple = heap_form_tuple(tupDesc,
                                                        op->d.fieldstore.values,
                                                        op->d.fieldstore.nulls);
 
@@ -3227,13 +3251,13 @@ ExecEvalArrayRefAssign(ExprState *state, ExprEvalStep *op)
 void
 ExecEvalConvertRowtype(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
 {
-       ConvertRowtypeExpr *convert = op->d.convert_rowtype.convert;
        HeapTuple       result;
        Datum           tupDatum;
        HeapTupleHeader tuple;
        HeapTupleData tmptup;
        TupleDesc       indesc,
                                outdesc;
+       bool            changed = false;
 
        /* NULL in -> NULL out */
        if (*op->resnull)
@@ -3242,24 +3266,19 @@ ExecEvalConvertRowtype(ExprState *state, ExprEvalStep *op, ExprContext *econtext
        tupDatum = *op->resvalue;
        tuple = DatumGetHeapTupleHeader(tupDatum);
 
-       /* Lookup tupdescs if first time through or after rescan */
-       if (op->d.convert_rowtype.indesc == NULL)
-       {
-               get_cached_rowtype(exprType((Node *) convert->arg), -1,
-                                                  &op->d.convert_rowtype.indesc,
-                                                  econtext);
-               op->d.convert_rowtype.initialized = false;
-       }
-       if (op->d.convert_rowtype.outdesc == NULL)
-       {
-               get_cached_rowtype(convert->resulttype, -1,
-                                                  &op->d.convert_rowtype.outdesc,
-                                                  econtext);
-               op->d.convert_rowtype.initialized = false;
-       }
-
-       indesc = op->d.convert_rowtype.indesc;
-       outdesc = op->d.convert_rowtype.outdesc;
+       /*
+        * Lookup tupdescs if first time through or if type changes.  We'd better
+        * pin them since type conversion functions could do catalog lookups and
+        * hence cause cache invalidation.
+        */
+       indesc = get_cached_rowtype(op->d.convert_rowtype.inputtype, -1,
+                                                               op->d.convert_rowtype.incache,
+                                                               &changed);
+       IncrTupleDescRefCount(indesc);
+       outdesc = get_cached_rowtype(op->d.convert_rowtype.outputtype, -1,
+                                                                op->d.convert_rowtype.outcache,
+                                                                &changed);
+       IncrTupleDescRefCount(outdesc);
 
        /*
         * We used to be able to assert that incoming tuples are marked with
@@ -3270,8 +3289,8 @@ ExecEvalConvertRowtype(ExprState *state, ExprEvalStep *op, ExprContext *econtext
        Assert(HeapTupleHeaderGetTypeId(tuple) == indesc->tdtypeid ||
                   HeapTupleHeaderGetTypeId(tuple) == RECORDOID);
 
-       /* if first time through, initialize conversion map */
-       if (!op->d.convert_rowtype.initialized)
+       /* if first time through, or after change, initialize conversion map */
+       if (changed)
        {
                MemoryContext old_cxt;
 
@@ -3282,7 +3301,6 @@ ExecEvalConvertRowtype(ExprState *state, ExprEvalStep *op, ExprContext *econtext
                op->d.convert_rowtype.map =
                        convert_tuples_by_name(indesc, outdesc,
                                                                   gettext_noop("could not convert row type"));
-               op->d.convert_rowtype.initialized = true;
 
                MemoryContextSwitchTo(old_cxt);
        }
@@ -3312,6 +3330,9 @@ ExecEvalConvertRowtype(ExprState *state, ExprEvalStep *op, ExprContext *econtext
                 */
                *op->resvalue = heap_copy_tuple_as_datum(&tmptup, outdesc);
        }
+
+       DecrTupleDescRefCount(indesc);
+       DecrTupleDescRefCount(outdesc);
 }
 
 /*
index 02af68f2c25d779e24ec09bcbcd3ad13a354264e..b0a34a10ec6d8be1e1ee2fdf1e01c7d43150ebc9 100644 (file)
@@ -32,6 +32,20 @@ typedef void (*ExecEvalSubroutine) (ExprState *state,
                                                                        struct ExprEvalStep *op,
                                                                        ExprContext *econtext);
 
+/* ExprEvalSteps that cache a composite type's tupdesc need one of these */
+/* (it fits in-line in some step types, otherwise allocate out-of-line) */
+typedef struct ExprEvalRowtypeCache
+{
+       /*
+        * cacheptr points to composite type's TypeCacheEntry if tupdesc_id is not
+        * 0; or for an anonymous RECORD type, it points directly at the cached
+        * tupdesc for the type, and tupdesc_id is 0.  (We'd use separate fields
+        * if space were not at a premium.)  Initial state is cacheptr == NULL.
+        */
+       void       *cacheptr;
+       uint64          tupdesc_id;             /* last-seen tupdesc identifier, or 0 */
+} ExprEvalRowtypeCache;
+
 /*
  * Discriminator for ExprEvalSteps.
  *
@@ -340,8 +354,8 @@ typedef struct ExprEvalStep
                /* for EEOP_NULLTEST_ROWIS[NOT]NULL */
                struct
                {
-                       /* cached tupdesc pointer - filled at runtime */
-                       TupleDesc       argdesc;
+                       /* cached descriptor for composite type - filled at runtime */
+                       ExprEvalRowtypeCache rowcache;
                }                       nulltest_row;
 
                /* for EEOP_PARAM_EXEC/EXTERN */
@@ -466,8 +480,8 @@ typedef struct ExprEvalStep
                {
                        AttrNumber      fieldnum;       /* field number to extract */
                        Oid                     resulttype; /* field's type */
-                       /* cached tupdesc pointer - filled at runtime */
-                       TupleDesc       argdesc;
+                       /* cached descriptor for composite type - filled at runtime */
+                       ExprEvalRowtypeCache rowcache;
                }                       fieldselect;
 
                /* for EEOP_FIELDSTORE_DEFORM / FIELDSTORE_FORM */
@@ -476,9 +490,9 @@ typedef struct ExprEvalStep
                        /* original expression node */
                        FieldStore *fstore;
 
-                       /* cached tupdesc pointer - filled at runtime */
-                       /* note that a DEFORM and FORM pair share the same tupdesc */
-                       TupleDesc  *argdesc;
+                       /* cached descriptor for composite type - filled at runtime */
+                       /* note that a DEFORM and FORM pair share the same cache */
+                       ExprEvalRowtypeCache *rowcache;
 
                        /* workspace for column values */
                        Datum      *values;
@@ -518,12 +532,12 @@ typedef struct ExprEvalStep
                /* for EEOP_CONVERT_ROWTYPE */
                struct
                {
-                       ConvertRowtypeExpr *convert;    /* original expression */
+                       Oid                     inputtype;      /* input composite type */
+                       Oid                     outputtype; /* output composite type */
                        /* these three fields are filled at runtime: */
-                       TupleDesc       indesc; /* tupdesc for input type */
-                       TupleDesc       outdesc;        /* tupdesc for output type */
+                       ExprEvalRowtypeCache *incache;  /* cache for input type */
+                       ExprEvalRowtypeCache *outcache; /* cache for output type */
                        TupleConversionMap *map;        /* column mapping */
-                       bool            initialized;    /* initialized for current types? */
                }                       convert_rowtype;
 
                /* for EEOP_SCALARARRAYOP */
index 6a5e94332a075b8e7605940f892eabab77315b50..ab81c13c286cc0f7633b1b801a837bf582db5577 100644 (file)
@@ -379,6 +379,30 @@ SELECT * FROM test1;
 ---+---
 (0 rows)
 
+-- operations on composite types vs. internal transactions
+DO LANGUAGE plpgsql $$
+declare
+  c test1 := row(42, 'hello');
+  r bool;
+begin
+  for i in 1..3 loop
+    r := c is not null;
+    raise notice 'r = %', r;
+    commit;
+  end loop;
+  for i in 1..3 loop
+    r := c is null;
+    raise notice 'r = %', r;
+    rollback;
+  end loop;
+end
+$$;
+NOTICE:  r = t
+NOTICE:  r = t
+NOTICE:  r = t
+NOTICE:  r = f
+NOTICE:  r = f
+NOTICE:  r = f
 -- COMMIT failures
 DO LANGUAGE plpgsql $$
 BEGIN
index 620d910309de361c5260cfd05b781036f5a32ee8..b13cf369db3cf6440a27ccf322fbb78589d62a60 100644 (file)
@@ -313,6 +313,26 @@ $$;
 SELECT * FROM test1;
 
 
+-- operations on composite types vs. internal transactions
+DO LANGUAGE plpgsql $$
+declare
+  c test1 := row(42, 'hello');
+  r bool;
+begin
+  for i in 1..3 loop
+    r := c is not null;
+    raise notice 'r = %', r;
+    commit;
+  end loop;
+  for i in 1..3 loop
+    r := c is null;
+    raise notice 'r = %', r;
+    rollback;
+  end loop;
+end
+$$;
+
+
 -- COMMIT failures
 DO LANGUAGE plpgsql $$
 BEGIN