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.
(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.)
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.
regards, tom lane