Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> I pushed this patch with some minor corrections (mostly improved the
> message wording.)
Coverity does not like this bit, and I fully agree:
/srv/coverity/git/pgsql-git/postgresql/src/backend/commands/trigger.c: 1639 in renametrig_partition()
>>> CID 1489387: Incorrect expression (ASSERT_SIDE_EFFECT)
>>> Argument "found++" of Assert() has a side effect. The containing function might work differently in a
non-debugbuild.
1639 Assert(found++ <= 0);
Perhaps there's no actual bug there, but it's still horrible coding.
For one thing, the implication that found could be negative is extremely
confusing to readers. A boolean might be better. However, I wonder why
you bothered with a flag in the first place. The usual convention if
we know there can be only one match is to just not write a loop at all,
with a suitable comment, like this pre-existing example elsewhere in
trigger.c:
/* There should be at most one matching tuple */
if (HeapTupleIsValid(tuple = systable_getnext(tgscan)))
If you're not quite convinced there can be only one match, then it
still shouldn't be an Assert --- a real test-and-elog would be better.
regards, tom lane
