Subject: Re: Avoid unncessary always true test (src/backend/storage/buffer/bufmgr.c)

From: Richard Guo <[email-masked]>
Date: Mon, 5 Jun 2023 10:37:10 +0800

On Sun, Jun 4, 2023 at 8:42 PM Ranier Vilela <[email-masked]> wrote:

> Hi,
>
> Per Coverity.
>
> At function ExtendBufferedRelShared, has a always true test.
> eb.rel was dereferenced one line above, so in
> if (eb.rel) is always true.
>
> I think it's worth removing the test, because Coverity raises dozens of
> alerts thinking eb.rel might be NULL.
> Besides, one less test is one less branch.
>

This also happens in ExtendBufferedRelTo, and the comment there explains
that the eb.rel 'could have been closed while waiting for lock'.  So for
the same consideration, the test in ExtendBufferedRelShared might be
still needed?  But I'm not familiar with the arounding codes, so need
someone else to confirm that.

Thanks
Richard

---

From: Ranier Vilela <[email-masked]>
Date: Mon, 5 Jun 2023 08:06:26 -0300

Em dom., 4 de jun. de 2023 às 23:37, Richard Guo <[email-masked]>
escreveu:

> that the eb.rel 'could have been closed while waiting for lock'.
>
Well, RelationGetSmgr also dereferences eb.rel.
If eb.rel could be closed while waiting for lock,
anyone who references eb.rel below takes a risk?

static inline SMgrRelation
RelationGetSmgr(Relation rel)
{
if (unlikely(rel->rd_smgr == NULL))
smgrsetowner(&(rel->rd_smgr), smgropen(rel->rd_locator, rel->rd_backend));
return rel->rd_smgr;
}

regards,
Ranier Vilela

---

From: Ranier Vilela <[email-masked]>
Date: Mon, 5 Jun 2023 08:24:13 -0300

> smgrsetowner(&(rel->rd_smgr), smgropen(rel->rd_locator, rel->rd_backend));
Sorry Richard, nevermind.

My fault, I withdraw this patch.

regards,
Ranier Vilela

---

From: Gurjeet Singh <[email-masked]>
Date: Mon, 12 Jun 2023 22:51:24 -0700

On Mon, Jun 5, 2023 at 4:24 AM Ranier Vilela <[email-masked]> wrote:
> Em seg., 5 de jun. de 2023 às 08:06, Ranier Vilela <[email-masked]> escreveu:
>> Em dom., 4 de jun. de 2023 às 23:37, Richard Guo <[email-masked]> escreveu:
>>>> I think it's worth removing the test, because Coverity raises dozens of alerts thinking eb.rel might be NULL.

I'm not quite convinced of the reasoning provided; either the reason
is not good enough, or my C is rusty. In either case, I'd like a
resolution.

The code in question:

>       LockRelationForExtension(eb.rel, ExclusiveLock);
>
>       /* could have been closed while waiting for lock */
>       if (eb.rel)
>           eb.smgr = RelationGetSmgr(eb.rel);

eb.rel is being passed by-value at line 1, so even if the relation is
closed, the value of the eb.rel cannot change between line 1 and line
3. So a code verification tool complaining that the 'if' condition
will always be true is quite right, IMO.

To verify my assumptions, I removed those checks and ran `make
{check,check-world,installcheck}`, and all those tests passed.

The only way, that I can think of, the value of eb.rel can change
between lines 1 and 3 is if 'eb' is a shared-memory data structure,
and some other process changed the 'rel' member in shared-memory. And
I don't think 'eb' is in shared memory in this case.

To me, it looks like these checks are a result of code being
copy-pasted from somewhere else, where this check might have been
necessary. The checks are sure not necessary at these spots.

Please see attached v2 of the patch; it includes both occurrences of
the spurious checks identified in this thread.

Best regards,
Gurjeet
http://Gurje.et

---

From: Michael Paquier <[email-masked]>
Date: Tue, 13 Jun 2023 15:11:26 +0900

> necessary. The checks are sure not necessary at these spots.

I am not completely sure based on my read of the code, but isn't this
check needed to avoid some kind of race condition with a concurrent
backend may have worked on the relation when attempting to get the
lock?
--
Michael

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Tue, 13 Jun 2023 16:39:21 +0900 (JST)

At Tue, 13 Jun 2023 15:11:26 +0900, Michael Paquier <[email-masked]> wrote in 
> backend may have worked on the relation when attempting to get the

Gurjeet has mentioned that eb.rel cannot be modified by another
process since the value or memory is in the local stack, and I believe
he's correct.

If the pointed Relation had been blown out, eb.rel would be left
dangling, not nullified. However, I don't believe this situation
happens (or it shouldn't happen) as the entire relation should already
be locked.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

---

From: Ranier Vilela <[email-masked]>
Date: Tue, 13 Jun 2023 08:21:20 -0300

> the spurious checks identified in this thread.
+1
LGTM.

regards,
Ranier Vilela

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Wed, 14 Jun 2023 10:01:59 +0900 (JST)

At Tue, 13 Jun 2023 08:21:20 -0300, Ranier Vilela <[email-masked]> wrote in 

> -		/* could have been closed while waiting for lock */
> -		if (eb.rel)
> -			eb.smgr = RelationGetSmgr(eb.rel);
> +		eb.smgr = RelationGetSmgr(eb.rel);
 
(It seems to me) The removed comment does refer to smgr, so we could
consider keeping it.  There are places instances where the function
calls are accompanied by similar comments and others where they
aren't. However, personally, I inclined towards its removal. That's
because our policy is to call RelationGetSmgr() each time before using
smgr, and this is well documented in the function's comment.

If we decide to remove it, the preceding blank line seems to be a
separator from the previous function call. So, we might want to
consider removing that blank line, too.

Otherwise it LGTM.

regards.

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Wed, 14 Jun 2023 10:05:51 +0900 (JST)

At Wed, 14 Jun 2023 10:01:59 +0900 (JST), Kyotaro Horiguchi <[email-masked]> wrote in 
> separator from the previous function call. So, we might want to

Mmm. that is a bit short. Anyway I meant that the blank will become
useless after removing the comment.

regards.

---

From: Richard Guo <[email-masked]>
Date: Wed, 14 Jun 2023 17:50:41 +0800

> happens (or it shouldn't happen) as the entire relation should already

Yeah, Gurjeet is right.  I had a thinko here.  eb.rel should not be NULL
pointer in any case.  And as we've acquired the lock for it, it should
not have been closed.  So I think we can remove the check for eb.rel in
the two places.

Thanks
Richard