Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 97b7ad4

Browse files
committed
Redesign the caching done by get_cached_rowtype().
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 4b93f57, 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
1 parent 37e7654 commit 97b7ad4

File tree

5 files changed

+180
-94
lines changed

5 files changed

+180
-94
lines changed

src/backend/executor/execExpr.c

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1132,7 +1132,7 @@ ExecInitExprRec(Expr *node, ExprState *state,
11321132
scratch.opcode = EEOP_FIELDSELECT;
11331133
scratch.d.fieldselect.fieldnum = fselect->fieldnum;
11341134
scratch.d.fieldselect.resulttype = fselect->resulttype;
1135-
scratch.d.fieldselect.argdesc = NULL;
1135+
scratch.d.fieldselect.rowcache.cacheptr = NULL;
11361136

11371137
ExprEvalPushStep(state, &scratch);
11381138
break;
@@ -1142,7 +1142,7 @@ ExecInitExprRec(Expr *node, ExprState *state,
11421142
{
11431143
FieldStore *fstore = (FieldStore *) node;
11441144
TupleDesc tupDesc;
1145-
TupleDesc *descp;
1145+
ExprEvalRowtypeCache *rowcachep;
11461146
Datum *values;
11471147
bool *nulls;
11481148
int ncolumns;
@@ -1158,17 +1158,17 @@ ExecInitExprRec(Expr *node, ExprState *state,
11581158
values = (Datum *) palloc(sizeof(Datum) * ncolumns);
11591159
nulls = (bool *) palloc(sizeof(bool) * ncolumns);
11601160

1161-
/* create workspace for runtime tupdesc cache */
1162-
descp = (TupleDesc *) palloc(sizeof(TupleDesc));
1163-
*descp = NULL;
1161+
/* create shared composite-type-lookup cache struct */
1162+
rowcachep = palloc(sizeof(ExprEvalRowtypeCache));
1163+
rowcachep->cacheptr = NULL;
11641164

11651165
/* emit code to evaluate the composite input value */
11661166
ExecInitExprRec(fstore->arg, state, resv, resnull);
11671167

11681168
/* next, deform the input tuple into our workspace */
11691169
scratch.opcode = EEOP_FIELDSTORE_DEFORM;
11701170
scratch.d.fieldstore.fstore = fstore;
1171-
scratch.d.fieldstore.argdesc = descp;
1171+
scratch.d.fieldstore.rowcache = rowcachep;
11721172
scratch.d.fieldstore.values = values;
11731173
scratch.d.fieldstore.nulls = nulls;
11741174
scratch.d.fieldstore.ncolumns = ncolumns;
@@ -1226,7 +1226,7 @@ ExecInitExprRec(Expr *node, ExprState *state,
12261226
/* finally, form result tuple */
12271227
scratch.opcode = EEOP_FIELDSTORE_FORM;
12281228
scratch.d.fieldstore.fstore = fstore;
1229-
scratch.d.fieldstore.argdesc = descp;
1229+
scratch.d.fieldstore.rowcache = rowcachep;
12301230
scratch.d.fieldstore.values = values;
12311231
scratch.d.fieldstore.nulls = nulls;
12321232
scratch.d.fieldstore.ncolumns = ncolumns;
@@ -1372,17 +1372,24 @@ ExecInitExprRec(Expr *node, ExprState *state,
13721372
case T_ConvertRowtypeExpr:
13731373
{
13741374
ConvertRowtypeExpr *convert = (ConvertRowtypeExpr *) node;
1375+
ExprEvalRowtypeCache *rowcachep;
1376+
1377+
/* cache structs must be out-of-line for space reasons */
1378+
rowcachep = palloc(2 * sizeof(ExprEvalRowtypeCache));
1379+
rowcachep[0].cacheptr = NULL;
1380+
rowcachep[1].cacheptr = NULL;
13751381

13761382
/* evaluate argument into step's result area */
13771383
ExecInitExprRec(convert->arg, state, resv, resnull);
13781384

13791385
/* and push conversion step */
13801386
scratch.opcode = EEOP_CONVERT_ROWTYPE;
1381-
scratch.d.convert_rowtype.convert = convert;
1382-
scratch.d.convert_rowtype.indesc = NULL;
1383-
scratch.d.convert_rowtype.outdesc = NULL;
1387+
scratch.d.convert_rowtype.inputtype =
1388+
exprType((Node *) convert->arg);
1389+
scratch.d.convert_rowtype.outputtype = convert->resulttype;
1390+
scratch.d.convert_rowtype.incache = &rowcachep[0];
1391+
scratch.d.convert_rowtype.outcache = &rowcachep[1];
13841392
scratch.d.convert_rowtype.map = NULL;
1385-
scratch.d.convert_rowtype.initialized = false;
13861393

13871394
ExprEvalPushStep(state, &scratch);
13881395
break;
@@ -2007,7 +2014,7 @@ ExecInitExprRec(Expr *node, ExprState *state,
20072014
(int) ntest->nulltesttype);
20082015
}
20092016
/* initialize cache in case it's a row test */
2010-
scratch.d.nulltest_row.argdesc = NULL;
2017+
scratch.d.nulltest_row.rowcache.cacheptr = NULL;
20112018

20122019
/* first evaluate argument into result variable */
20132020
ExecInitExprRec(ntest->arg, state,

src/backend/executor/execExprInterp.c

Lines changed: 92 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -145,8 +145,8 @@ static void ExecInitInterpreter(void);
145145
static void CheckVarSlotCompatibility(TupleTableSlot *slot, int attnum, Oid vartype);
146146
static void CheckOpSlotCompatibility(ExprEvalStep *op, TupleTableSlot *slot);
147147
static TupleDesc get_cached_rowtype(Oid type_id, int32 typmod,
148-
TupleDesc *cache_field, ExprContext *econtext);
149-
static void ShutdownTupleDescRef(Datum arg);
148+
ExprEvalRowtypeCache *rowcache,
149+
bool *changed);
150150
static void ExecEvalRowNullInt(ExprState *state, ExprEvalStep *op,
151151
ExprContext *econtext, bool checkisnull);
152152

@@ -1942,56 +1942,78 @@ CheckOpSlotCompatibility(ExprEvalStep *op, TupleTableSlot *slot)
19421942
* get_cached_rowtype: utility function to lookup a rowtype tupdesc
19431943
*
19441944
* type_id, typmod: identity of the rowtype
1945-
* cache_field: where to cache the TupleDesc pointer in expression state node
1946-
* (field must be initialized to NULL)
1947-
* econtext: expression context we are executing in
1945+
* rowcache: space for caching identity info
1946+
* (rowcache->cacheptr must be initialized to NULL)
1947+
* changed: if not NULL, *changed is set to true on any update
19481948
*
1949-
* NOTE: because the shutdown callback will be called during plan rescan,
1950-
* must be prepared to re-do this during any node execution; cannot call
1951-
* just once during expression initialization.
1949+
* The returned TupleDesc is not guaranteed pinned; caller must pin it
1950+
* to use it across any operation that might incur cache invalidation.
1951+
* (The TupleDesc is always refcounted, so just use IncrTupleDescRefCount.)
1952+
*
1953+
* NOTE: because composite types can change contents, we must be prepared
1954+
* to re-do this during any node execution; cannot call just once during
1955+
* expression initialization.
19521956
*/
19531957
static TupleDesc
19541958
get_cached_rowtype(Oid type_id, int32 typmod,
1955-
TupleDesc *cache_field, ExprContext *econtext)
1959+
ExprEvalRowtypeCache *rowcache,
1960+
bool *changed)
19561961
{
1957-
TupleDesc tupDesc = *cache_field;
1958-
1959-
/* Do lookup if no cached value or if requested type changed */
1960-
if (tupDesc == NULL ||
1961-
type_id != tupDesc->tdtypeid ||
1962-
typmod != tupDesc->tdtypmod)
1962+
if (type_id != RECORDOID)
19631963
{
1964-
tupDesc = lookup_rowtype_tupdesc(type_id, typmod);
1964+
/*
1965+
* It's a named composite type, so use the regular typcache. Do a
1966+
* lookup first time through, or if the composite type changed. Note:
1967+
* "tupdesc_id == 0" may look redundant, but it protects against the
1968+
* admittedly-theoretical possibility that type_id was RECORDOID the
1969+
* last time through, so that the cacheptr isn't TypeCacheEntry *.
1970+
*/
1971+
TypeCacheEntry *typentry = (TypeCacheEntry *) rowcache->cacheptr;
19651972

1966-
if (*cache_field)
1973+
if (unlikely(typentry == NULL ||
1974+
rowcache->tupdesc_id == 0 ||
1975+
typentry->tupDesc_identifier != rowcache->tupdesc_id))
19671976
{
1968-
/* Release old tupdesc; but callback is already registered */
1969-
ReleaseTupleDesc(*cache_field);
1970-
}
1971-
else
1972-
{
1973-
/* Need to register shutdown callback to release tupdesc */
1974-
RegisterExprContextCallback(econtext,
1975-
ShutdownTupleDescRef,
1976-
PointerGetDatum(cache_field));
1977-
}
1978-
*cache_field = tupDesc;
1977+
typentry = lookup_type_cache(type_id, TYPECACHE_TUPDESC);
1978+
if (typentry->tupDesc == NULL)
1979+
ereport(ERROR,
1980+
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
1981+
errmsg("type %s is not composite",
1982+
format_type_be(type_id))));
1983+
rowcache->cacheptr = (void *) typentry;
1984+
rowcache->tupdesc_id = typentry->tupDesc_identifier;
1985+
if (changed)
1986+
*changed = true;
1987+
}
1988+
return typentry->tupDesc;
1989+
}
1990+
else
1991+
{
1992+
/*
1993+
* A RECORD type, once registered, doesn't change for the life of the
1994+
* backend. So we don't need a typcache entry as such, which is good
1995+
* because there isn't one. It's possible that the caller is asking
1996+
* about a different type than before, though.
1997+
*/
1998+
TupleDesc tupDesc = (TupleDesc) rowcache->cacheptr;
1999+
2000+
if (unlikely(tupDesc == NULL ||
2001+
rowcache->tupdesc_id != 0 ||
2002+
type_id != tupDesc->tdtypeid ||
2003+
typmod != tupDesc->tdtypmod))
2004+
{
2005+
tupDesc = lookup_rowtype_tupdesc(type_id, typmod);
2006+
/* Drop pin acquired by lookup_rowtype_tupdesc */
2007+
ReleaseTupleDesc(tupDesc);
2008+
rowcache->cacheptr = (void *) tupDesc;
2009+
rowcache->tupdesc_id = 0; /* not a valid value for non-RECORD */
2010+
if (changed)
2011+
*changed = true;
2012+
}
2013+
return tupDesc;
19792014
}
1980-
return tupDesc;
19812015
}
19822016

1983-
/*
1984-
* Callback function to release a tupdesc refcount at econtext shutdown
1985-
*/
1986-
static void
1987-
ShutdownTupleDescRef(Datum arg)
1988-
{
1989-
TupleDesc *cache_field = (TupleDesc *) DatumGetPointer(arg);
1990-
1991-
if (*cache_field)
1992-
ReleaseTupleDesc(*cache_field);
1993-
*cache_field = NULL;
1994-
}
19952017

19962018
/*
19972019
* Fast-path functions, for very simple expressions
@@ -2583,8 +2605,7 @@ ExecEvalRowNullInt(ExprState *state, ExprEvalStep *op,
25832605

25842606
/* Lookup tupdesc if first time through or if type changes */
25852607
tupDesc = get_cached_rowtype(tupType, tupTypmod,
2586-
&op->d.nulltest_row.argdesc,
2587-
econtext);
2608+
&op->d.nulltest_row.rowcache, NULL);
25882609

25892610
/*
25902611
* heap_attisnull needs a HeapTuple not a bare HeapTupleHeader.
@@ -3017,8 +3038,7 @@ ExecEvalFieldSelect(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
30173038

30183039
/* Lookup tupdesc if first time through or if type changes */
30193040
tupDesc = get_cached_rowtype(tupType, tupTypmod,
3020-
&op->d.fieldselect.argdesc,
3021-
econtext);
3041+
&op->d.fieldselect.rowcache, NULL);
30223042

30233043
/*
30243044
* Find field's attr record. Note we don't support system columns
@@ -3076,9 +3096,9 @@ ExecEvalFieldStoreDeForm(ExprState *state, ExprEvalStep *op, ExprContext *econte
30763096
{
30773097
TupleDesc tupDesc;
30783098

3079-
/* Lookup tupdesc if first time through or after rescan */
3099+
/* Lookup tupdesc if first time through or if type changes */
30803100
tupDesc = get_cached_rowtype(op->d.fieldstore.fstore->resulttype, -1,
3081-
op->d.fieldstore.argdesc, econtext);
3101+
op->d.fieldstore.rowcache, NULL);
30823102

30833103
/* Check that current tupdesc doesn't have more fields than we allocated */
30843104
if (unlikely(tupDesc->natts > op->d.fieldstore.ncolumns))
@@ -3120,10 +3140,14 @@ ExecEvalFieldStoreDeForm(ExprState *state, ExprEvalStep *op, ExprContext *econte
31203140
void
31213141
ExecEvalFieldStoreForm(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
31223142
{
3143+
TupleDesc tupDesc;
31233144
HeapTuple tuple;
31243145

3125-
/* argdesc should already be valid from the DeForm step */
3126-
tuple = heap_form_tuple(*op->d.fieldstore.argdesc,
3146+
/* Lookup tupdesc (should be valid already) */
3147+
tupDesc = get_cached_rowtype(op->d.fieldstore.fstore->resulttype, -1,
3148+
op->d.fieldstore.rowcache, NULL);
3149+
3150+
tuple = heap_form_tuple(tupDesc,
31273151
op->d.fieldstore.values,
31283152
op->d.fieldstore.nulls);
31293153

@@ -3334,13 +3358,13 @@ ExecEvalSubscriptingRefAssign(ExprState *state, ExprEvalStep *op)
33343358
void
33353359
ExecEvalConvertRowtype(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
33363360
{
3337-
ConvertRowtypeExpr *convert = op->d.convert_rowtype.convert;
33383361
HeapTuple result;
33393362
Datum tupDatum;
33403363
HeapTupleHeader tuple;
33413364
HeapTupleData tmptup;
33423365
TupleDesc indesc,
33433366
outdesc;
3367+
bool changed = false;
33443368

33453369
/* NULL in -> NULL out */
33463370
if (*op->resnull)
@@ -3349,24 +3373,19 @@ ExecEvalConvertRowtype(ExprState *state, ExprEvalStep *op, ExprContext *econtext
33493373
tupDatum = *op->resvalue;
33503374
tuple = DatumGetHeapTupleHeader(tupDatum);
33513375

3352-
/* Lookup tupdescs if first time through or after rescan */
3353-
if (op->d.convert_rowtype.indesc == NULL)
3354-
{
3355-
get_cached_rowtype(exprType((Node *) convert->arg), -1,
3356-
&op->d.convert_rowtype.indesc,
3357-
econtext);
3358-
op->d.convert_rowtype.initialized = false;
3359-
}
3360-
if (op->d.convert_rowtype.outdesc == NULL)
3361-
{
3362-
get_cached_rowtype(convert->resulttype, -1,
3363-
&op->d.convert_rowtype.outdesc,
3364-
econtext);
3365-
op->d.convert_rowtype.initialized = false;
3366-
}
3367-
3368-
indesc = op->d.convert_rowtype.indesc;
3369-
outdesc = op->d.convert_rowtype.outdesc;
3376+
/*
3377+
* Lookup tupdescs if first time through or if type changes. We'd better
3378+
* pin them since type conversion functions could do catalog lookups and
3379+
* hence cause cache invalidation.
3380+
*/
3381+
indesc = get_cached_rowtype(op->d.convert_rowtype.inputtype, -1,
3382+
op->d.convert_rowtype.incache,
3383+
&changed);
3384+
IncrTupleDescRefCount(indesc);
3385+
outdesc = get_cached_rowtype(op->d.convert_rowtype.outputtype, -1,
3386+
op->d.convert_rowtype.outcache,
3387+
&changed);
3388+
IncrTupleDescRefCount(outdesc);
33703389

33713390
/*
33723391
* We used to be able to assert that incoming tuples are marked with
@@ -3377,8 +3396,8 @@ ExecEvalConvertRowtype(ExprState *state, ExprEvalStep *op, ExprContext *econtext
33773396
Assert(HeapTupleHeaderGetTypeId(tuple) == indesc->tdtypeid ||
33783397
HeapTupleHeaderGetTypeId(tuple) == RECORDOID);
33793398

3380-
/* if first time through, initialize conversion map */
3381-
if (!op->d.convert_rowtype.initialized)
3399+
/* if first time through, or after change, initialize conversion map */
3400+
if (changed)
33823401
{
33833402
MemoryContext old_cxt;
33843403

@@ -3387,7 +3406,6 @@ ExecEvalConvertRowtype(ExprState *state, ExprEvalStep *op, ExprContext *econtext
33873406

33883407
/* prepare map from old to new attribute numbers */
33893408
op->d.convert_rowtype.map = convert_tuples_by_name(indesc, outdesc);
3390-
op->d.convert_rowtype.initialized = true;
33913409

33923410
MemoryContextSwitchTo(old_cxt);
33933411
}
@@ -3417,6 +3435,9 @@ ExecEvalConvertRowtype(ExprState *state, ExprEvalStep *op, ExprContext *econtext
34173435
*/
34183436
*op->resvalue = heap_copy_tuple_as_datum(&tmptup, outdesc);
34193437
}
3438+
3439+
DecrTupleDescRefCount(indesc);
3440+
DecrTupleDescRefCount(outdesc);
34203441
}
34213442

34223443
/*

0 commit comments

Comments
 (0)