Em sex., 26 de jun. de 2020 às 18:24, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
Ranier Vilela <ranier.vf@gmail.com> writes: > 1.Assertion check > /* Caller messed up if we have neither a ready query nor held data. */ > Assert(queryDesc || portal->holdStore);
> But in release, if QueryDesc is NULL and portal->holdStore is NULL too, > when Call PushActiveSnapshot *deference* NULL check can happen.
> 2. if (portal->atEnd || count <= 0) is True > No need to recheck count against FETCH_ALL.
> Is it worth correcting them?
No.
The assertion already says that that's a case that cannot happen. Or to look at it another way: if the case were to occur in a devel build, you'd get a core dump at the assertion. If the case were to occur in a production build, you'd get a core dump at the dereference. Not much difference. Either way, it's a *caller* bug, because the caller is supposed to make sure this cannot happen. If we thought that it could possibly happen, we would use an ereport but not an assertion; having both for the same condition is quite misguided.
Ok, thats a job of Assertion.
But I still worry that, in some rare cases, portal-> holdStore might be corrupted in some way and the function is called, causing a segmentation fault.
(If Coverity is whining about this for you, there's something wrong with your Coverity settings. In the project's instance, Coverity accepts assertions as assertions.)
Probable, because reports this:
CID 10127 (#2 of 2): Dereference after null check (FORWARD_NULL)8.var_deref_op:Dereferencing null pointer queryDesc.
I'm unimpressed with the other proposed change too; it's making the logic more complicated and fragile for a completely negligible "performance gain". Moreover the compiler could probably make the same optimization.
Ok.
Anyway, thank you for by responding, your observations are always valuable and help learn "the postgres way" to develop. It's not easy.