pgsql: Redesign the caching done by get_cached_rowtype().

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-committers(at)lists(dot)postgresql(dot)org
Subject: pgsql: Redesign the caching done by get_cached_rowtype().
Date: 2021-04-13 17:37:31
Message-ID: E1lWMyh-0000gD-D0@gemulon.postgresql.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-committers

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 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

Branch
------
REL_11_STABLE

Details
-------
https://git.postgresql.org/pg/commitdiff/22f2a98cf3ed73ece782c66917e4734b8f736374

Modified Files
--------------
src/backend/executor/execExpr.c | 31 ++--
src/backend/executor/execExprInterp.c | 157 ++++++++++++---------
src/include/executor/execExpr.h | 36 +++--
.../plpgsql/src/expected/plpgsql_transaction.out | 24 ++++
src/pl/plpgsql/src/sql/plpgsql_transaction.sql | 20 +++
5 files changed, 177 insertions(+), 91 deletions(-)

Browse pgsql-committers by date

  From Date Subject
Next Message Tom Lane 2021-04-13 19:10:40 pgsql: Fix some inappropriately-disallowed uses of ALTER ROLE/DATABASE
Previous Message Tom Lane 2021-04-13 16:17:39 pgsql: Avoid improbable PANIC during heap_update.