From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Arne Roland <A.Roland@index.de>
Cc: vignesh C <vignesh21@gmail.com>
Cc: Zhihong Yu <zyu@yugabyte.com>
Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Rename of triggers for partitioned tables
Date: Sun, 25 Jul 2021 10:49:43 -0400
Message-ID: <1337562.1627224583@sss.pgh.pa.us> (raw)
In-Reply-To: <202107222235.djjcscrqxpwn@alvherre.pgsql>
References: <202107222235.djjcscrqxpwn@alvherre.pgsql>
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-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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, alvherre@alvh.no-ip.org, A.Roland@index.de, vignesh21@gmail.com, zyu@yugabyte.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Rename of triggers for partitioned tables
In-Reply-To: <1337562.1627224583@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox