While looking at the recent bug report from Alexander Lakhin [1], I got
annoyed by the relcache code, and RelationClearRelation in particular. I
propose to refactor it for clarity.
[1]
https://www.postgresql.org/message-id/e56be7d9-14b1-664d-0bfc-00ce9772721c%40gmail.com
## Patch 1
This is just a narrow fix for the reported bug [1], same as I posted on
that thread. Included here because I wrote the refactorings on top of
this patch and didn't commit it yet.
## Patch 2: Simplify call to rebuild relcache entry for indexes
To rebuild a relcache entry that's been marked as invalid,
RelationIdGetRelation() calls RelationReloadIndexInfo() for indexes and
RelationClearRelation(rebuild == true) for other relations. However,
RelationClearRelation calls RelationReloadIndexInfo() for indexes
anyway, so RelationIdGetRelation() can just always call
RelationClearRelation() and let RelationClearRelation() do the right
thing to rebuild the relation, whether it's an index or something else.
That seems more straightforward.
Also add comments explaining how the rebuild works at index creation.
It's a bit special, see the comments.
## Patch 3: Split RelationClearRelation into three different functions
RelationClearRelation() is complicated. Depending on the 'rebuild'
argument and the circumstances, like if it's called in a transaction and
whether the relation is an index, a nailed relation, a regular table, or
a relation dropped in the same xact, it does different things:
- Remove the relation completely from the cache (rebuild == false),
- Mark the entry as invalid (rebuild == true, but not in xact), or
- Rebuild the entry (rebuild == true).
The callers have expectations on what they want it to do. Mostly the
callers with 'rebuild == false' expect the entry to be removed, and
callers with 'rebuild == true' expect it to be rebuilt or invalidated,
but there are exceptions. RelationForgetRelation() for example sets
rd_droppedSubid and expects RelationClearRelation() to then merely
invalidate it, and the call from RelationIdGetRelation() expects it to
rebuild, not merely invalidate it.
I propose to split RelationClearRelation() into three functions:
RelationInvalidateRelation: mark the relcache entry as invalid, so that
it it is rebuilt on next access.
RelationRebuildRelation: rebuild the relcache entry in-place.
RelationClearRelation: Remove the entry from the relcache.
This moves the responsibility of deciding the right action to the
callers. Which they were mostly already doing. Each of those actions
have different preconditions, e.g. RelationRebuildRelation() can only be
called in a valid transaction, and RelationClearRelation() can only be
called if the reference count is zero. Splitting them makes those
preconditions more clear, we can have assertions to document them in each.
## RelationInitPhysicalAddr() call in RelationReloadNailed()
One question or little doubt I have: Before these patches,
RelationReloadNailed() calls RelationInitPhysicalAddr() even when it
leaves the relation as invalidated because we're not in a transaction or
if the relation isn't currently in use. That's a bit surprising, I'd
expect it to be done when the entry is reloaded, not when it's
invalidated. That's how it's done for non-nailed relations. And in fact,
for a nailed relation, RelationInitPhysicalAddr() is called *again* when
it's reloaded.
Is it important? Before commit a54e1f1587, nailed non-index relations
were not reloaded at all, except for the call to
RelationInitPhysicalAddr(), which seemed consistent. I think this was
unintentional in commit a54e1f1587, or perhaps just overly defensive, as
there's no harm in some extra RelationInitPhysicalAddr() calls.
This patch removes that extra call from the invalidation path, but if it
turns out to be wrong, we can easily add it to RelationInvalidateRelation.
--
Heikki Linnakangas
Neon (https://neon.tech)
From 227f173d1066955a2ab6aba12c508aaa1f416c27 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Wed, 5 Jun 2024 13:29:51 +0300
Subject: [PATCH 1/3] Make RelationFlushRelation() work without ResourceOwner
during abort
ReorderBufferImmediateInvalidation() executes invalidation messages in
an aborted transaction. However, RelationFlushRelation sometimes
required a valid resource owner, to temporarily increment the refcount
of the relache entry. Commit b8bff07daa worked around that in the main
subtransaction abort function, AbortSubTransaction(), but missed this
similar case in ReorderBufferImmediateInvalidation().
To fix, introduce a separate function to invalidate a relcache
entry. It does the same thing as RelationClearRelation(rebuild==true)
does, when outside a transaction, but can be called without
incrementing the refcount.
Add regression test. Before this fix, it failed with:
ERROR: ResourceOwnerEnlarge called after release started
Reported-by: Alexander Lakhin <exclus...@gmail.com>
Disussion: https://www.postgresql.org/message-id/e56be7d9-14b1-664d-0bfc-00ce97727...@gmail.com
---
.../expected/decoding_in_xact.out | 48 +++++++++++++++++
.../test_decoding/sql/decoding_in_xact.sql | 27 ++++++++++
src/backend/access/transam/xact.c | 13 -----
src/backend/utils/cache/relcache.c | 54 ++++++++++++++++---
4 files changed, 122 insertions(+), 20 deletions(-)
diff --git a/contrib/test_decoding/expected/decoding_in_xact.out b/contrib/test_decoding/expected/decoding_in_xact.out
index b65253f4630..8555b3d9436 100644
--- a/contrib/test_decoding/expected/decoding_in_xact.out
+++ b/contrib/test_decoding/expected/decoding_in_xact.out
@@ -79,6 +79,54 @@ SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'inc
COMMIT
(6 rows)
+-- Decoding works in transaction that issues DDL
+--
+-- We had issues handling relcache invalidations with these, see
+-- https://www.postgresql.org/message-id/e56be7d9-14b1-664d-0bfc-00ce97727...@gmail.com
+CREATE TABLE tbl_created_outside_xact(id SERIAL PRIMARY KEY);
+BEGIN;
+ -- TRUNCATE changes the relfilenode and sends relcache invalidation
+ TRUNCATE tbl_created_outside_xact;
+ INSERT INTO tbl_created_outside_xact(id) VALUES('1');
+ -- don't show yet, haven't committed
+ SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
+ data
+------
+(0 rows)
+
+COMMIT;
+SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
+ data
+--------------------------------------------------------------
+ BEGIN
+ table public.tbl_created_outside_xact: TRUNCATE: (no-flags)
+ table public.tbl_created_outside_xact: INSERT: id[integer]:1
+ COMMIT
+(4 rows)
+
+set debug_logical_replication_streaming=immediate;
+BEGIN;
+ CREATE TABLE tbl_created_in_xact(id SERIAL PRIMARY KEY);
+ INSERT INTO tbl_created_in_xact VALUES (1);
+ CHECKPOINT; -- Force WAL flush, so that the above changes will be streamed
+ SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1', 'stream-changes', '1');
+ data
+------------------------------------------
+ opening a streamed block for transaction
+ streaming change for transaction
+ closing a streamed block for transaction
+(3 rows)
+
+COMMIT;
+reset debug_logical_replication_streaming;
+SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
+ data
+---------------------------------------------------------
+ BEGIN
+ table public.tbl_created_in_xact: INSERT: id[integer]:1
+ COMMIT
+(3 rows)
+
SELECT 'stop' FROM pg_drop_replication_slot('regression_slot');
?column?
----------
diff --git a/contrib/test_decoding/sql/decoding_in_xact.sql b/contrib/test_decoding/sql/decoding_in_xact.sql
index 108782dc2e9..841a92fa780 100644
--- a/contrib/test_decoding/sql/decoding_in_xact.sql
+++ b/contrib/test_decoding/sql/decoding_in_xact.sql
@@ -38,4 +38,31 @@ COMMIT;
INSERT INTO nobarf(data) VALUES('3');
SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
+-- Decoding works in transaction that issues DDL
+--
+-- We had issues handling relcache invalidations with these, see
+-- https://www.postgresql.org/message-id/e56be7d9-14b1-664d-0bfc-00ce97727...@gmail.com
+CREATE TABLE tbl_created_outside_xact(id SERIAL PRIMARY KEY);
+BEGIN;
+ -- TRUNCATE changes the relfilenode and sends relcache invalidation
+ TRUNCATE tbl_created_outside_xact;
+ INSERT INTO tbl_created_outside_xact(id) VALUES('1');
+
+ -- don't show yet, haven't committed
+ SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
+COMMIT;
+SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
+
+set debug_logical_replication_streaming=immediate;
+BEGIN;
+ CREATE TABLE tbl_created_in_xact(id SERIAL PRIMARY KEY);
+ INSERT INTO tbl_created_in_xact VALUES (1);
+
+ CHECKPOINT; -- Force WAL flush, so that the above changes will be streamed
+
+ SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1', 'stream-changes', '1');
+COMMIT;
+reset debug_logical_replication_streaming;
+SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1');
+
SELECT 'stop' FROM pg_drop_replication_slot('regression_slot');
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 4f4ce757623..9bda1aa6bc6 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -5279,20 +5279,7 @@ AbortSubTransaction(void)
AtEOSubXact_RelationCache(false, s->subTransactionId,
s->parent->subTransactionId);
-
-
- /*
- * AtEOSubXact_Inval sometimes needs to temporarily bump the refcount
- * on the relcache entries that it processes. We cannot use the
- * subtransaction's resource owner anymore, because we've already
- * started releasing it. But we can use the parent resource owner.
- */
- CurrentResourceOwner = s->parent->curTransactionOwner;
-
AtEOSubXact_Inval(false);
-
- CurrentResourceOwner = s->curTransactionOwner;
-
ResourceOwnerRelease(s->curTransactionOwner,
RESOURCE_RELEASE_LOCKS,
false, false);
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index cc9b0c6524f..35dbb87ae3d 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -275,6 +275,7 @@ static HTAB *OpClassCache = NULL;
static void RelationCloseCleanup(Relation relation);
static void RelationDestroyRelation(Relation relation, bool remember_tupdesc);
+static void RelationInvalidateRelation(Relation relation);
static void RelationClearRelation(Relation relation, bool rebuild);
static void RelationReloadIndexInfo(Relation relation);
@@ -2512,6 +2513,31 @@ RelationDestroyRelation(Relation relation, bool remember_tupdesc)
pfree(relation);
}
+/*
+ * RelationInvalidateRelation - mark a relation cache entry as invalid
+ *
+ * An entry that's marked as invalid will be reloaded on next access.
+ */
+static void
+RelationInvalidateRelation(Relation relation)
+{
+ /*
+ * Make sure smgr and lower levels close the relation's files, if they
+ * weren't closed already. If the relation is not getting deleted, the
+ * next smgr access should reopen the files automatically. This ensures
+ * that the low-level file access state is updated after, say, a vacuum
+ * truncation.
+ */
+ RelationCloseSmgr(relation);
+
+ /* Free AM cached data, if any */
+ if (relation->rd_amcache)
+ pfree(relation->rd_amcache);
+ relation->rd_amcache = NULL;
+
+ relation->rd_isvalid = false;
+}
+
/*
* RelationClearRelation
*
@@ -2846,14 +2872,28 @@ RelationFlushRelation(Relation relation)
* New relcache entries are always rebuilt, not flushed; else we'd
* forget the "new" status of the relation. Ditto for the
* new-relfilenumber status.
- *
- * The rel could have zero refcnt here, so temporarily increment the
- * refcnt to ensure it's safe to rebuild it. We can assume that the
- * current transaction has some lock on the rel already.
*/
- RelationIncrementReferenceCount(relation);
- RelationClearRelation(relation, true);
- RelationDecrementReferenceCount(relation);
+ if (IsTransactionState() && relation->rd_droppedSubid == InvalidSubTransactionId)
+ {
+ /*
+ * The rel could have zero refcnt here, so temporarily increment
+ * the refcnt to ensure it's safe to rebuild it. We can assume
+ * that the current transaction has some lock on the rel already.
+ */
+ RelationIncrementReferenceCount(relation);
+ RelationClearRelation(relation, true);
+ RelationDecrementReferenceCount(relation);
+ }
+ else
+ {
+ /*
+ * During abort processing, the current resource owner is not
+ * valid and we cannot hold a refcnt. Without a valid
+ * transaction, RelationClearRelation() would just mark the rel as
+ * invalid anyway, so we can do the same directly.
+ */
+ RelationInvalidateRelation(relation);
+ }
}
else
{
--
2.39.2
From 105b9b9ec3f724e85e1c738a3010a523f8fab707 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Wed, 5 Jun 2024 16:33:19 +0300
Subject: [PATCH 2/3] Simplify call to rebuild relcache entry for indexes
RelationClearRelation(rebuild == true) calls RelationReloadIndexInfo()
for indexes. We can rely on that in RelationIdGetRelation(), instead
of calling RelationReloadIndexInfo() directly. That simplifies the
code a little.
In the passing, add a comment in RelationBuildLocalRelation()
explaining why it doesn't call RelationInitIndexAccessInfo(). It's
because at index creation, it's called before the pg_index row has
been created. That's also the reason that RelationClearRelation()
still needs a special case to go through the full-blown rebuild if the
index support information in the relcache entry hasn't been populated
yet.
Discussion: XXX
---
src/backend/utils/cache/relcache.c | 41 +++++++++++++-----------------
1 file changed, 17 insertions(+), 24 deletions(-)
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 35dbb87ae3d..621046f5cc4 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -2085,16 +2085,7 @@ RelationIdGetRelation(Oid relationId)
/* revalidate cache entry if necessary */
if (!rd->rd_isvalid)
{
- /*
- * Indexes only have a limited number of possible schema changes,
- * and we don't want to use the full-blown procedure because it's
- * a headache for indexes that reload itself depends on.
- */
- if (rd->rd_rel->relkind == RELKIND_INDEX ||
- rd->rd_rel->relkind == RELKIND_PARTITIONED_INDEX)
- RelationReloadIndexInfo(rd);
- else
- RelationClearRelation(rd, true);
+ RelationClearRelation(rd, true);
/*
* Normally entries need to be valid here, but before the relcache
@@ -2266,14 +2257,6 @@ RelationReloadIndexInfo(Relation relation)
!relation->rd_isvalid &&
relation->rd_droppedSubid == InvalidSubTransactionId);
- /* Ensure it's closed at smgr level */
- RelationCloseSmgr(relation);
-
- /* Must free any AM cached data upon relcache flush */
- if (relation->rd_amcache)
- pfree(relation->rd_amcache);
- relation->rd_amcache = NULL;
-
/*
* If it's a shared index, we might be called before backend startup has
* finished selecting a database, in which case we have no way to read
@@ -2602,15 +2585,19 @@ RelationClearRelation(Relation relation, bool rebuild)
return;
/*
- * Even non-system indexes should not be blown away if they are open and
- * have valid index support information. This avoids problems with active
- * use of the index support information. As with nailed indexes, we
- * re-read the pg_class row to handle possible physical relocation of the
- * index, and we check for pg_index updates too.
+ * Indexes only have a limited number of possible schema changes, and we
+ * don't want to use the full-blown procedure because it's a headache for
+ * indexes that reload itself depends on.
+ *
+ * As an exception, use the full procedure if the index access info hasn't
+ * been initialized yet. Index creation relies on that: it first builds
+ * the relcache entry with RelationBuildLocalRelation(), creates the
+ * pg_index tuple only after that, and then relies on
+ * CommandCounterIncrement to load the pg_index contents.
*/
if ((relation->rd_rel->relkind == RELKIND_INDEX ||
relation->rd_rel->relkind == RELKIND_PARTITIONED_INDEX) &&
- relation->rd_refcnt > 0 &&
+ rebuild &&
relation->rd_indexcxt != NULL)
{
if (IsTransactionState())
@@ -3719,6 +3706,12 @@ RelationBuildLocalRelation(const char *relname,
if (RELKIND_HAS_TABLE_AM(relkind) || relkind == RELKIND_SEQUENCE)
RelationInitTableAccessMethod(rel);
+ /*
+ * Leave index index access method uninitialized, because the pg_index row
+ * has not been inserted at this stage of index creation yet. The cache
+ * invalidation after pg_index row has been inserted will initialize it.
+ */
+
/*
* Okay to insert into the relcache hash table.
*
--
2.39.2
From 446300b407359dd4f41305a19fc52bc40d864db5 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Wed, 5 Jun 2024 16:33:38 +0300
Subject: [PATCH 3/3] Split RelationClearRelation into three different
functions
The old RelationClearRelation function did different things depending
on the arguments and circumstances. It could:
- Remove the relation completely from relcache (rebuild == false),
- Mark the entry as invalid (rebuild == true, but not in xact), or
- Rebuild the entry (rebuild == true).
Different callers used it for different purposes, and often assumed a
particular behavior, which was confusing. Split it into three
different functions, one for each of the above actions (one of them,
RelationInvalidateRelation, was already added in commit XXX). Move
the responsibility of choosing the action and calling the right
function to the callers.
Discussion: XXX
---
src/backend/utils/cache/relcache.c | 301 ++++++++++++++---------------
1 file changed, 141 insertions(+), 160 deletions(-)
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 621046f5cc4..2f34a7f3fe0 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -276,7 +276,8 @@ static HTAB *OpClassCache = NULL;
static void RelationCloseCleanup(Relation relation);
static void RelationDestroyRelation(Relation relation, bool remember_tupdesc);
static void RelationInvalidateRelation(Relation relation);
-static void RelationClearRelation(Relation relation, bool rebuild);
+static void RelationClearRelation(Relation relation);
+static void RelationRebuildRelation(Relation relation);
static void RelationReloadIndexInfo(Relation relation);
static void RelationReloadNailed(Relation relation);
@@ -2085,7 +2086,7 @@ RelationIdGetRelation(Oid relationId)
/* revalidate cache entry if necessary */
if (!rd->rd_isvalid)
{
- RelationClearRelation(rd, true);
+ RelationRebuildRelation(rd);
/*
* Normally entries need to be valid here, but before the relcache
@@ -2213,7 +2214,7 @@ RelationCloseCleanup(Relation relation)
if (RelationHasReferenceCountZero(relation) &&
relation->rd_createSubid == InvalidSubTransactionId &&
relation->rd_firstRelfilelocatorSubid == InvalidSubTransactionId)
- RelationClearRelation(relation, false);
+ RelationClearRelation(relation);
#endif
}
@@ -2228,12 +2229,6 @@ RelationCloseCleanup(Relation relation)
* and/or in active use. We support full replacement of the pg_class row,
* as well as updates of a few simple fields of the pg_index row.
*
- * We can't necessarily reread the catalog rows right away; we might be
- * in a failed transaction when we receive the SI notification. If so,
- * RelationClearRelation just marks the entry as invalid by setting
- * rd_isvalid to false. This routine is called to fix the entry when it
- * is next needed.
- *
* We assume that at the time we are called, we have at least AccessShareLock
* on the target index. (Note: in the calls from RelationClearRelation,
* this is legitimate because we know the rel has positive refcount.)
@@ -2353,7 +2348,13 @@ RelationReloadIndexInfo(Relation relation)
static void
RelationReloadNailed(Relation relation)
{
+ /* Should be called only for invalidated, nailed relations */
+ Assert(!relation->rd_isvalid);
Assert(relation->rd_isnailed);
+ /* nailed indexes are handled by RelationReloadIndexInfo() */
+ Assert(relation->rd_rel->relkind == RELKIND_RELATION);
+ /* can only reread catalog contents in a transaction */
+ Assert(IsTransactionState());
/*
* Redo RelationInitPhysicalAddr in case it is a mapped relation whose
@@ -2361,58 +2362,35 @@ RelationReloadNailed(Relation relation)
*/
RelationInitPhysicalAddr(relation);
- /* flag as needing to be revalidated */
- relation->rd_isvalid = false;
-
/*
- * Can only reread catalog contents if in a transaction. If the relation
- * is currently open (not counting the nailed refcount), do so
- * immediately. Otherwise we've already marked the entry as possibly
- * invalid, and it'll be fixed when next opened.
+ * Reload a non-index entry. We can't easily do so if relcaches
+ * aren't yet built, but that's fine because at that stage the
+ * attributes that need to be current (like relfrozenxid) aren't yet
+ * accessed. To ensure the entry will later be revalidated, we leave
+ * it in invalid state, but allow use (cf. RelationIdGetRelation()).
*/
- if (!IsTransactionState() || relation->rd_refcnt <= 1)
- return;
-
- if (relation->rd_rel->relkind == RELKIND_INDEX)
- {
- /*
- * If it's a nailed-but-not-mapped index, then we need to re-read the
- * pg_class row to see if its relfilenumber changed.
- */
- RelationReloadIndexInfo(relation);
- }
- else
+ if (criticalRelcachesBuilt)
{
+ HeapTuple pg_class_tuple;
+ Form_pg_class relp;
+
/*
- * Reload a non-index entry. We can't easily do so if relcaches
- * aren't yet built, but that's fine because at that stage the
- * attributes that need to be current (like relfrozenxid) aren't yet
- * accessed. To ensure the entry will later be revalidated, we leave
- * it in invalid state, but allow use (cf. RelationIdGetRelation()).
+ * NB: Mark the entry as valid before starting to scan, to avoid
+ * self-recursion when re-building pg_class.
*/
- if (criticalRelcachesBuilt)
- {
- HeapTuple pg_class_tuple;
- Form_pg_class relp;
-
- /*
- * NB: Mark the entry as valid before starting to scan, to avoid
- * self-recursion when re-building pg_class.
- */
- relation->rd_isvalid = true;
+ relation->rd_isvalid = true;
- pg_class_tuple = ScanPgRelation(RelationGetRelid(relation),
- true, false);
- relp = (Form_pg_class) GETSTRUCT(pg_class_tuple);
- memcpy(relation->rd_rel, relp, CLASS_TUPLE_SIZE);
- heap_freetuple(pg_class_tuple);
+ pg_class_tuple = ScanPgRelation(RelationGetRelid(relation),
+ true, false);
+ relp = (Form_pg_class) GETSTRUCT(pg_class_tuple);
+ memcpy(relation->rd_rel, relp, CLASS_TUPLE_SIZE);
+ heap_freetuple(pg_class_tuple);
- /*
- * Again mark as valid, to protect against concurrently arriving
- * invalidations.
- */
- relation->rd_isvalid = true;
- }
+ /*
+ * Again mark as valid, to protect against concurrently arriving
+ * invalidations.
+ */
+ relation->rd_isvalid = true;
}
}
@@ -2522,44 +2500,28 @@ RelationInvalidateRelation(Relation relation)
}
/*
- * RelationClearRelation
- *
- * Physically blow away a relation cache entry, or reset it and rebuild
- * it from scratch (that is, from catalog entries). The latter path is
- * used when we are notified of a change to an open relation (one with
- * refcount > 0).
+ * RelationClearRelation - physically blow away a relation cache entry
*
- * NB: when rebuilding, we'd better hold some lock on the relation,
- * else the catalog data we need to read could be changing under us.
- * Also, a rel to be rebuilt had better have refcnt > 0. This is because
- * a sinval reset could happen while we're accessing the catalogs, and
- * the rel would get blown away underneath us by RelationCacheInvalidate
- * if it has zero refcnt.
- *
- * The "rebuild" parameter is redundant in current usage because it has
- * to match the relation's refcnt status, but we keep it as a crosscheck
- * that we're doing what the caller expects.
+ * The caller must ensure that the entry is no longer needed, i.e. its
+ * reference count is zero. Also, the rel or its storage must not be created
+ * in the current transaction (rd_createSubid and rd_firstRelfilelocatorSubid
+ * must not be set).
*/
static void
-RelationClearRelation(Relation relation, bool rebuild)
+RelationClearRelation(Relation relation)
{
- /*
- * As per notes above, a rel to be rebuilt MUST have refcnt > 0; while of
- * course it would be an equally bad idea to blow away one with nonzero
- * refcnt, since that would leave someone somewhere with a dangling
- * pointer. All callers are expected to have verified that this holds.
- */
- Assert(rebuild ?
- !RelationHasReferenceCountZero(relation) :
- RelationHasReferenceCountZero(relation));
+ Assert(RelationHasReferenceCountZero(relation));
+ Assert(!relation->rd_isnailed);
/*
- * Make sure smgr and lower levels close the relation's files, if they
- * weren't closed already. If the relation is not getting deleted, the
- * next smgr access should reopen the files automatically. This ensures
- * that the low-level file access state is updated after, say, a vacuum
- * truncation.
+ * Relations created in the same transaction must never be removed, see
+ * RelationFlushRelation.
*/
+ Assert(relation->rd_createSubid == InvalidSubTransactionId);
+ Assert(relation->rd_firstRelfilelocatorSubid == InvalidSubTransactionId);
+ Assert(relation->rd_droppedSubid == InvalidSubTransactionId);
+
+ /* Ensure it's closed at smgr level */
RelationCloseSmgr(relation);
/* Free AM cached data, if any */
@@ -2567,23 +2529,51 @@ RelationClearRelation(Relation relation, bool rebuild)
pfree(relation->rd_amcache);
relation->rd_amcache = NULL;
- /*
- * Treat nailed-in system relations separately, they always need to be
- * accessible, so we can't blow them away.
- */
- if (relation->rd_isnailed)
- {
- RelationReloadNailed(relation);
- return;
- }
+ /* Mark it as invalid (just pro forma since we will free it below) */
+ relation->rd_isvalid = false;
+
+ /* Remove it from the hash table */
+ RelationCacheDelete(relation);
+
+ /* And release storage */
+ RelationDestroyRelation(relation, false);
+}
+
+/*
+ * RelationRebuildRelation - rebuild a relation cache entry in place
+ *
+ * Reset and rebuild a relation cache entry from scratch (that is, from
+ * catalog entries). This is used when we are notified of a change to an open
+ * relation (one with refcount > 0). The entry is reconstructed without
+ * moving the physical RelationData record, so that the refcount holder's
+ * pointer is still valid.
+ *
+ * NB: when rebuilding, we'd better hold some lock on the relation, else the
+ * catalog data we need to read could be changing under us. Also, a rel to be
+ * rebuilt had better have refcnt > 0. This is because a sinval reset could
+ * happen while we're accessing the catalogs, and the rel would get blown away
+ * underneath us by RelationCacheInvalidate if it has zero refcnt.
+ */
+static void
+RelationRebuildRelation(Relation relation)
+{
+ Assert(!RelationHasReferenceCountZero(relation));
+ /* rebuilding requires access to the catalogs */
+ Assert(IsTransactionState());
+ /* there is no reason to ever rebuild a dropped relation */
+ Assert(relation->rd_droppedSubid == InvalidSubTransactionId);
+
+ /* Ensure it's closed at smgr level */
+ RelationCloseSmgr(relation);
+
+ /* Free AM cached data, if any */
+ if (relation->rd_amcache)
+ pfree(relation->rd_amcache);
+ relation->rd_amcache = NULL;
/* Mark it invalid until we've finished rebuild */
relation->rd_isvalid = false;
- /* See RelationForgetRelation(). */
- if (relation->rd_droppedSubid != InvalidSubTransactionId)
- return;
-
/*
* Indexes only have a limited number of possible schema changes, and we
* don't want to use the full-blown procedure because it's a headache for
@@ -2597,49 +2587,15 @@ RelationClearRelation(Relation relation, bool rebuild)
*/
if ((relation->rd_rel->relkind == RELKIND_INDEX ||
relation->rd_rel->relkind == RELKIND_PARTITIONED_INDEX) &&
- rebuild &&
relation->rd_indexcxt != NULL)
{
- if (IsTransactionState())
- RelationReloadIndexInfo(relation);
+ RelationReloadIndexInfo(relation);
return;
}
-
- /*
- * If we're really done with the relcache entry, blow it away. But if
- * someone is still using it, reconstruct the whole deal without moving
- * the physical RelationData record (so that the someone's pointer is
- * still valid).
- */
- if (!rebuild)
+ /* Nailed relations are handled separately. */
+ else if (relation->rd_isnailed)
{
- /* Remove it from the hash table */
- RelationCacheDelete(relation);
-
- /* And release storage */
- RelationDestroyRelation(relation, false);
- }
- else if (!IsTransactionState())
- {
- /*
- * If we're not inside a valid transaction, we can't do any catalog
- * access so it's not possible to rebuild yet. Just exit, leaving
- * rd_isvalid = false so that the rebuild will occur when the entry is
- * next opened.
- *
- * Note: it's possible that we come here during subtransaction abort,
- * and the reason for wanting to rebuild is that the rel is open in
- * the outer transaction. In that case it might seem unsafe to not
- * rebuild immediately, since whatever code has the rel already open
- * will keep on using the relcache entry as-is. However, in such a
- * case the outer transaction should be holding a lock that's
- * sufficient to prevent any significant change in the rel's schema,
- * so the existing entry contents should be good enough for its
- * purposes; at worst we might be behind on statistics updates or the
- * like. (See also CheckTableNotInUse() and its callers.) These same
- * remarks also apply to the cases above where we exit without having
- * done RelationReloadIndexInfo() yet.
- */
+ RelationReloadNailed(relation);
return;
}
else
@@ -2868,28 +2824,47 @@ RelationFlushRelation(Relation relation)
* that the current transaction has some lock on the rel already.
*/
RelationIncrementReferenceCount(relation);
- RelationClearRelation(relation, true);
+ RelationRebuildRelation(relation);
RelationDecrementReferenceCount(relation);
}
else
- {
- /*
- * During abort processing, the current resource owner is not
- * valid and we cannot hold a refcnt. Without a valid
- * transaction, RelationClearRelation() would just mark the rel as
- * invalid anyway, so we can do the same directly.
- */
RelationInvalidateRelation(relation);
- }
}
else
{
/*
* Pre-existing rels can be dropped from the relcache if not open.
+ *
+ * If the entry is in use, rebuild it if possible. If we're not
+ * inside a valid transaction, we can't do any catalog access so it's
+ * not possible to rebuild yet. Just mark it as invalid in that case,
+ * so that the rebuild will occur when the entry is next opened.
+ *
+ * Note: it's possible that we come here during subtransaction abort,
+ * and the reason for wanting to rebuild is that the rel is open in
+ * the outer transaction. In that case it might seem unsafe to not
+ * rebuild immediately, since whatever code has the rel already open
+ * will keep on using the relcache entry as-is. However, in such a
+ * case the outer transaction should be holding a lock that's
+ * sufficient to prevent any significant change in the rel's schema,
+ * so the existing entry contents should be good enough for its
+ * purposes; at worst we might be behind on statistics updates or the
+ * like. (See also CheckTableNotInUse() and its callers.)
*/
- bool rebuild = !RelationHasReferenceCountZero(relation);
-
- RelationClearRelation(relation, rebuild);
+ if (RelationHasReferenceCountZero(relation))
+ RelationClearRelation(relation);
+ else if (relation->rd_isnailed && relation->rd_refcnt == 1)
+ {
+ /*
+ * A nailed relation with refcnt == 1 is unused. We cannot clear
+ * it, but there's also no need no need to rebuild it immediately.
+ */
+ RelationInvalidateRelation(relation);
+ }
+ else if (!IsTransactionState())
+ RelationInvalidateRelation(relation);
+ else
+ RelationRebuildRelation(relation);
}
}
@@ -2915,14 +2890,15 @@ RelationForgetRelation(Oid rid)
{
/*
* In the event of subtransaction rollback, we must not forget
- * rd_*Subid. Mark the entry "dropped" so RelationClearRelation()
- * invalidates it in lieu of destroying it. (If we're in a top
- * transaction, we could opt to destroy the entry.)
+ * rd_*Subid. Mark the entry "dropped" and invalidate it, instead of
+ * destroying it right away. (If we're in a top transaction, we could
+ * opt to destroy the entry.)
*/
relation->rd_droppedSubid = GetCurrentSubTransactionId();
+ RelationInvalidateRelation(relation);
}
-
- RelationClearRelation(relation, false);
+ else
+ RelationClearRelation(relation);
}
/*
@@ -3034,8 +3010,7 @@ RelationCacheInvalidate(bool debug_discard)
if (RelationHasReferenceCountZero(relation))
{
/* Delete this entry immediately */
- Assert(!relation->rd_isnailed);
- RelationClearRelation(relation, false);
+ RelationClearRelation(relation);
}
else
{
@@ -3075,17 +3050,23 @@ RelationCacheInvalidate(bool debug_discard)
*/
smgrreleaseall();
- /* Phase 2: rebuild the items found to need rebuild in phase 1 */
+ /* Phase 2: rebuild (or invalidate) the items found to need rebuild in phase 1 */
foreach(l, rebuildFirstList)
{
relation = (Relation) lfirst(l);
- RelationClearRelation(relation, true);
+ if (!IsTransactionState() || (relation->rd_isnailed && relation->rd_refcnt == 1))
+ RelationInvalidateRelation(relation);
+ else
+ RelationRebuildRelation(relation);
}
list_free(rebuildFirstList);
foreach(l, rebuildList)
{
relation = (Relation) lfirst(l);
- RelationClearRelation(relation, true);
+ if (!IsTransactionState() || (relation->rd_isnailed && relation->rd_refcnt == 1))
+ RelationInvalidateRelation(relation);
+ else
+ RelationRebuildRelation(relation);
}
list_free(rebuildList);
@@ -3343,7 +3324,7 @@ AtEOXact_cleanup(Relation relation, bool isCommit)
{
if (RelationHasReferenceCountZero(relation))
{
- RelationClearRelation(relation, false);
+ RelationClearRelation(relation);
return;
}
else
@@ -3453,7 +3434,7 @@ AtEOSubXact_cleanup(Relation relation, bool isCommit,
relation->rd_newRelfilelocatorSubid = InvalidSubTransactionId;
relation->rd_firstRelfilelocatorSubid = InvalidSubTransactionId;
relation->rd_droppedSubid = InvalidSubTransactionId;
- RelationClearRelation(relation, false);
+ RelationClearRelation(relation);
return;
}
else
--
2.39.2
