SEO

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_list
is empty or only contains the resultRelation. I can't convince myself
this is always ok. The condition that 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 to Assert() 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.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFER2sLIB7bNG8LQkwRAmp2AKCMCwZSEoK6iOL2l8unKM EGaiK3pQCfS/v+
5RrDxEpE7cAUZ+JEIBUCIC8=
=6R+h
-----END PGP SIGNATURE-----

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

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

> 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

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

> 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


---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

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.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFESNj3IB7bNG8LQkwRAnrOAJ4secP6Ls4ChMzi3iYVIS oQK4+JMQCeMb6C
nsB468f7djME3N8ovV6RSLk=
=vx0P
-----END PGP SIGNATURE-----

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. +

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

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. +

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

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...4/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.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)

iD8DBQFETfcFIB7bNG8LQkwRAuQyAJwIxXb9HLruBGCkSluxPI DUn2c4qwCfdESS
clIjBMWXJLNU0LJcrqiUE4M=
=cvVa
-----END PGP SIGNATURE-----