Thread: Checking assumptions
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: src/backend/executor/nodeMaterial.c function ExecMaterial if( !node->randomAccess && !ScanDirectionIsForward && !node->eof_underlying) dies line 87 randomAccess is set if EXEC_FLAG_BACKWARD is set, but does that guarentee it will never be tried? src/backend/optimizer/plan/planner.c function inheritance_planner If the bulk of the loop is skipped for any reason, we segfault right after. This can only happen if ((PlannerInfo *)root)->append_rel_listis empty or only contains the resultRelation. I can't convince myself this is always ok. The conditionthat invokes this function in subquery_planner is obtuse enough that I can't trigger it. 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 toAssert() it three lines down. src/backend/utils/adt/ruleutils.c function get_sublink_expr We assume sublink->subLinkType == ANY_SUBLINK implies sublink->testexpr != NULL. Otherwise we die at line 4114. src/backend/rewrite/rewriteHandler.c function AcquireRewriteLocks Assume ((Var*)var)->varno > 0 src/backend/executor/execMain.c function ExecutePlan We assume an UPDATE statement always has a junkfilter. 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.
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/ > From each according to his ability. To each according to his ability to litigate.
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] 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/ > From each according to his ability. To each according to his ability to litigate.