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

On Mon, Jun 5, 2023 at 4:24 AM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:
> Em seg., 5 de jun. de 2023 às 08:06, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> escreveu:
>> Em dom., 4 de jun. de 2023 às 23:37, Richard Guo <guofenglinux(at)gmail(dot)com> escreveu:
>>> On Sun, Jun 4, 2023 at 8:42 PM Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> 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'.
>>
>> 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;
>> }
>
> Sorry Richard, nevermind.
>
> My fault, I withdraw this patch.

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