Re: Rename of triggers for partitioned tables - Mailing list pgsql-hackers

From Tom Lane
Subject Re: Rename of triggers for partitioned tables
Date
Msg-id 1337562.1627224583@sss.pgh.pa.us
Whole thread Raw
In response to Re: Rename of triggers for partitioned tables  (Alvaro Herrera <alvherre@alvh.no-ip.org>)
Responses Re: Rename of triggers for partitioned tables
List pgsql-hackers
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



pgsql-hackers by date:

Previous
From: Pavel Stehule
Date:
Subject: Re: proposal: plpgsql: special comments that will be part of AST
Next
From: Tom Lane
Date:
Subject: Re: Planning counters in pg_stat_statements (using pgss_store)