Hi,
Good catch. Indeed, eb.rel shouldn't be NULL there and the tests should be
unnecessary. However, it doesn't follow from the code of these functions.
>From what I can see eb.rel can be NULL in both of these functions. There is
the following Assert in the beginning of the ExtendBufferedRelTo() function:
Assert((eb.rel != NULL) != (eb.smgr != NULL));
And ExtendBufferedRelShared() is only called from ExtendBufferedRelCommon()
which can be called from ExtendBufferedRelTo() or ExtendBufferedRelBy() that
also has the same Assert(). And none of these functions assigns eb.rel, so
it can be NULL from the very beginning and it stays the same.
And there is the following call in xlogutils.c, which is exactly the case
when
eb.rel is NULL:
buffer = ExtendBufferedRelTo(EB_SMGR(smgr, RELPERSISTENCE_PERMANENT),
forknum,
NULL,
EB_PERFORMING_RECOVERY |
EB_SKIP_EXTENSION_LOCK,
blkno + 1,
mode);
So as for me calling LockRelationForExtension() and
UnlockRelationForExtension()
without testing eb.rel first looks more like a bug here. However, they are
never
actually called with eb.rel=NULL because of the EB_* flags, so there is no
bug
here. I believe we should add Assert checking that when eb.rel is NULL,
flags
are such that we won't use eb.rel. And yes, we can remove unnecessary checks
where the flags value guaranty us that eb.rel is not NULL.
And another minor observation. It seems to me that we don't need a "could
have
been closed while waiting for lock" in ExtendBufferedRelShared(), because I
believe the comment above already explains why updating eb.smgr:
* Note that another backend might have extended the relation by the time
* we get the lock.
I attached the new version of the patch as I see it.
Best regards,
Karina Litskevich
Postgres Professional: http://postgrespro.com/
From 7425d5b1b9d30f0da75aabd94e862906f9635a60 Mon Sep 17 00:00:00 2001
From: Karina Litskevich <litskevichkar...@gmail.com>
Date: Fri, 30 Jun 2023 21:26:18 +0300
Subject: [PATCH v4] Remove always true checks
Also add asserts that help understanding these checks are always true
---
src/backend/storage/buffer/bufmgr.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 3c59bbd04e..7b1e2e1284 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -851,6 +851,8 @@ ExtendBufferedRelBy(ExtendBufferedWhat eb,
{
Assert((eb.rel != NULL) != (eb.smgr != NULL));
Assert(eb.smgr == NULL || eb.relpersistence != 0);
+ Assert(eb.rel != NULL || (flags & EB_SKIP_EXTENSION_LOCK) &&
+ !(flags & EB_CREATE_FORK_IF_NEEDED));
Assert(extend_by > 0);
if (eb.smgr == NULL)
@@ -887,6 +889,8 @@ ExtendBufferedRelTo(ExtendBufferedWhat eb,
Assert((eb.rel != NULL) != (eb.smgr != NULL));
Assert(eb.smgr == NULL || eb.relpersistence != 0);
+ Assert(eb.rel != NULL || (flags & EB_SKIP_EXTENSION_LOCK) &&
+ !(flags & EB_CREATE_FORK_IF_NEEDED));
Assert(extend_to != InvalidBlockNumber && extend_to > 0);
if (eb.smgr == NULL)
@@ -908,8 +912,7 @@ ExtendBufferedRelTo(ExtendBufferedWhat eb,
LockRelationForExtension(eb.rel, ExclusiveLock);
/* could have been closed while waiting for lock */
- if (eb.rel)
- eb.smgr = RelationGetSmgr(eb.rel);
+ eb.smgr = RelationGetSmgr(eb.rel);
/* recheck, fork might have been created concurrently */
if (!smgrexists(eb.smgr, fork))
@@ -1875,8 +1878,7 @@ ExtendBufferedRelShared(ExtendBufferedWhat eb,
if (!(flags & EB_SKIP_EXTENSION_LOCK))
{
LockRelationForExtension(eb.rel, ExclusiveLock);
- if (eb.rel)
- eb.smgr = RelationGetSmgr(eb.rel);
+ eb.smgr = RelationGetSmgr(eb.rel);
}
/*
--
2.25.1
