Checking assumptions

Martijn van Oosterhout <kleptog@svana.org> writes:

randomAccess is set if EXEC_FLAG_BACKWARD is set, but does that
guarentee it will never be tried?

If it were tried, that would be caller error. Think of it as an Assert ;-)

src/backend/optimizer/plan/planner.c function inheritance_planner=20
If the bulk of the loop is skipped for any reason, we segfault right
after.

Another poor man's Assert.

src/backend/utils/adt/selfuncs.c function like_selectivity

Assume this function is never called with a zero length bytea
constant. It just looks wierd to set patt to NULL only to Assert() it
three lines down.

This may be a real bug --- I'm not sure how well the bytea-LIKE path has
been tested, and it looks odd to me too.

src/backend/utils/adt/ruleutils.c function get_sublink_expr
We assume sublink->subLinkType == ANY_SUBLINK implies
sublink->testexpr != NULL.

Yeah, it does.

src/backend/rewrite/rewriteHandler.c function AcquireRewriteLocks
Assume ((Var*)var)->varno > 0

For a join alias, I don't see any problem there.

src/backend/executor/execMain.c function ExecutePlan
We assume an UPDATE statement always has a junkfilter.

Yup.

regards, tom lane

Martijn van Oosterhout <kleptog@svana.org> writes:

src/backend/utils/adt/selfuncs.c function like_selectivity

Assume this function is never called with a zero length bytea
constant. It just looks wierd to set patt to NULL only to Assert() it
three lines down.

This may be a real bug --- I'm not sure how well the bytea-LIKE path has
been tested, and it looks odd to me too.

I checked into this, and in fact the path can't be taken: we only reach
like_selectivity when trying to estimate selectivity of a pattern that
is not an "exact match" pattern --- and in LIKE, that requires at least
one wildcard, so the pattern can't be empty.

So there's no bug, but the coding is certainly a bit obscure --- I'll
try to make it more clear.

regards, tom lane

I havn't been able to find any more serious issues in the Coverity
report, now that they've fixed the ereport() issue. A number of the
issues it complains about are things we already Assert() for. For the
rest, as long as the following assumptions are true we're done (well,
except for ECPG). I think they are true but it's always good to check:

Everytime someone does this, we fix everything except ECPG. Surely it's
time we fixed ECPG as well?

Chris

On Fri, Apr 21, 2006 at 09:12:51AM +0800, Christopher Kings-Lynne wrote:

I havn't been able to find any more serious issues in the Coverity
report, now that they've fixed the ereport() issue. A number of the
issues it complains about are things we already Assert() for. For the
rest, as long as the following assumptions are true we're done (well,
except for ECPG). I think they are true but it's always good to check:

Everytime someone does this, we fix everything except ECPG. Surely it's
time we fixed ECPG as well?

I've got a patch (not by me) that should fix most of the issues.
However, we have no way to test for regressions. So, that's why I
suggested (elsewhere) someone get the ECPG regression stuff working so
we can apply fixes and check they don't break anything...

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

Martijn van Oosterhout wrote:
-- Start of PGP signed section.

On Fri, Apr 21, 2006 at 09:12:51AM +0800, Christopher Kings-Lynne wrote:

I havn't been able to find any more serious issues in the Coverity
report, now that they've fixed the ereport() issue. A number of the
issues it complains about are things we already Assert() for. For the
rest, as long as the following assumptions are true we're done (well,
except for ECPG). I think they are true but it's always good to check:

Everytime someone does this, we fix everything except ECPG. Surely it's
time we fixed ECPG as well?

I've got a patch (not by me) that should fix most of the issues.
However, we have no way to test for regressions. So, that's why I
suggested (elsewhere) someone get the ECPG regression stuff working so
we can apply fixes and check they don't break anything...

Well, we should wait a reasonable time for Michael to review the
changes, but if not, we should just move ahead and do our best to fix
ecpg ourselves.

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Are we OK with the Coverity reports now?

---------------------------------------------------------------------------

Martijn van Oosterhout wrote:
-- Start of PGP signed section.

On Fri, Apr 21, 2006 at 09:12:51AM +0800, Christopher Kings-Lynne wrote:

I havn't been able to find any more serious issues in the Coverity
report, now that they've fixed the ereport() issue. A number of the
issues it complains about are things we already Assert() for. For the
rest, as long as the following assumptions are true we're done (well,
except for ECPG). I think they are true but it's always good to check:

Everytime someone does this, we fix everything except ECPG. Surely it's
time we fixed ECPG as well?

I've got a patch (not by me) that should fix most of the issues.
However, we have no way to test for regressions. So, that's why I
suggested (elsewhere) someone get the ECPG regression stuff working so
we can apply fixes and check they don't break anything...

Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

From each according to his ability. To each according to his ability to litigate.

-- End of PGP section, PGP failed!

--
Bruce Momjian http://candle.pha.pa.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Mon, Apr 24, 2006 at 11:11:59PM -0400, Bruce Momjian wrote:

Are we OK with the Coverity reports now?

Well, you can see for yourself:

http://scan.coverity.com/

We're down from the near-300 to just 60. They've unfixed the ereport()
issue but it was fixed for two days which allowed me to isolate then
and mark the false positives. More than 50% of those remaining are in
the ECPG code (primarily memory-leaks in error conditions which may or
may not be real). The remaining are in the src/bin directory, where the
issues are not that important.

The only one remaining in the backend I consider important was the one
relating to the failure to allocate a shared hash [1]http://archives.postgresql.org/pgsql-hackers/2006-04/msg00732.php -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ which I posted
earlier.

We're now into the hard-slog part. For example, the fix to
ecpg/ecpglib/execute.c yesterday fixes the old problems but creates new
ones (nval leaked on last iteration of loop).

I'm still trying to find a way to export info on the memory leaks so
other people can look at them.

Have a nice day,
[1]: http://archives.postgresql.org/pgsql-hackers/2006-04/msg00732.php -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/