Re: Rename of triggers for partitioned tables

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
Cc: Arne Roland <A(dot)Roland(at)index(dot)de>, vignesh C <vignesh21(at)gmail(dot)com>, Zhihong Yu <zyu(at)yugabyte(dot)com>, Pg Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Rename of triggers for partitioned tables
Date: 2021-07-25 14:49:43
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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

/* 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

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2021-07-25 16:03:25 Re: Planning counters in pg_stat_statements (using pgss_store)
Previous Message Pavel Stehule 2021-07-25 13:34:15 Re: proposal: plpgsql: special comments that will be part of AST