On Mon, Jun 5, 2023 at 4:24 AM Ranier Vilela <[email protected]> wrote: > Em seg., 5 de jun. de 2023 às 08:06, Ranier Vilela <[email protected]> > escreveu: >> Em dom., 4 de jun. de 2023 às 23:37, Richard Guo <[email protected]> >> escreveu: >>> On Sun, Jun 4, 2023 at 8:42 PM Ranier Vilela <[email protected]> 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
v2-0001-Remove-always-true-checks.patch
Description: Binary data
