Re: Rename of triggers for partitioned tables

Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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-debug build.
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