backend dies on 7.1.1 loading large datamodel.

On Mon, May 07, 2001 at 05:14:48PM -0500, Robert Hentosh wrote:
:: snip ::

I can send the datamodel and core file if needed. I loaded GDB with the
core file and got the following:

I just put the datamodel at http://www.io.com/~hentosh/sql.tar.gz

Robert Hentosh <hentosh@io.com> writes:

I then proceed to load the OpenACS datamodel and had the backend crash.
This datamodel loads fine on 7.1.

Ugh.

I can send the datamodel and core file if needed.

Datamodel please, corefile no.

regards, tom lane

Robert Hentosh <hentosh@io.com> writes:

I just put the datamodel at http://www.io.com/~hentosh/sql.tar.gz

Hm. I notice that postgres.sql hardwires the location of the plpgsql
handler:

create function plpgsql_call_handler() RETURNS opaque
as '/usr/local/pgsql/lib/plpgsql.so' language 'c';

create trusted procedural language 'plpgsql'
HANDLER plpgsql_call_handler
LANCOMPILER 'PL/pgSQL';

If this were to suck in a wrong-version copy of plpgsql.so (and yes,
I think 7.1 vs 7.1.1 could be wrong version) then that could cause
failures.

postgres-pgtcl.sql is equally unwise about the pltcl handler.

This is *not* the source of your problem, since I was able to
reproduce the crash even with a proper "createlang plpgsql" used
instead of the bogus commands. But you might want to pass on the
observation to the OpenACS guys.

On with debugging ...

regards, tom lane

Robert Hentosh <hentosh@io.com> writes:

I then proceed to load the OpenACS datamodel and had the backend crash.
This datamodel loads fine on 7.1.

Sigh. Looks like I managed to break plpgsql's handling of SELECT with
no data found. Mea maxima culpa (though the regression tests perhaps
deserve a share of the blame too, for not covering that case).

Patch to appear shortly. I guess there will be a 7.1.2 sooner than
I thought, also :-(

regards, tom lane

On Mon, May 07, 2001 at 08:28:33PM -0400, Tom Lane wrote:

Robert Hentosh <hentosh@io.com> writes:

I just put the datamodel at http://www.io.com/~hentosh/sql.tar.gz

Hm. I notice that postgres.sql hardwires the location of the plpgsql
handler:

create function plpgsql_call_handler() RETURNS opaque
as '/usr/local/pgsql/lib/plpgsql.so' language 'c';

create trusted procedural language 'plpgsql'
HANDLER plpgsql_call_handler
LANCOMPILER 'PL/pgSQL';

If this were to suck in a wrong-version copy of plpgsql.so (and yes,
I think 7.1 vs 7.1.1 could be wrong version) then that could cause
failures.

I played with this a little. What would be the proper solution?
Doesn't the backend go and cd to the data directory. I blindly
tried:
as 'plpgsql.so' language 'c';
and
as 'lib/plpgsql.so' language 'c';

and it can't find the file. Is there a way to correctly reference the
lib directory associated with the execuables directory structure?

One of the examples in the docs shows the full path, too. At the bottom
of this URL:

http://postgresql.readysetnet.com/users-lounge/docs/7.0/postgres/sql-createlanguage.htm

Tom Lane wrote:

Robert Hentosh <hentosh@io.com> writes:

I then proceed to load the OpenACS datamodel and had the backend crash.
This datamodel loads fine on 7.1.

Ugh.

There was a bug report in Japan that plpgsql
crashes if select returns no row. This seems
a bug introduced by your latest change on
pl_exec.c.

regards,
Hiroshi Inoue

Hiroshi Inoue <Inoue@tpf.co.jp> writes:

There was a bug report in Japan that plpgsql
crashes if select returns no row. This seems
a bug introduced by your latest change on
pl_exec.c.

Indeed. Too bad that report didn't arrive on Friday.
I am mightily embarrassed :-(

regards, tom lane

Robert Hentosh <hentosh@io.com> writes:

If this were to suck in a wrong-version copy of plpgsql.so (and yes,
I think 7.1 vs 7.1.1 could be wrong version) then that could cause
failures.

I played with this a little. What would be the proper solution?

At the moment, the solution is to use the createlang script rather than
issuing the commands directly.

In the long run I think we should abandon the notion that full path
specifications are the preferred way to locate dynamic libraries.
It would be a lot better for portability if C function libraries could
be referred to like this:

create function pltcl_call_handler() returns opaque
as 'pltcl' language 'C';

where the backend automatically assumes that a relative path is relative
to $PGLIB. I'd like to see the backend adding the file extension too,
to avoid platform dependencies (".so" is not universal). A function
definition like the above could be dumped and reloaded without fear,
whereas the existing approach is pretty much guaranteed to break
whenever you change machines or install directories.

regards, tom lane

... (though the regression tests perhaps
deserve a share of the blame too, for not covering that case).
Patch to appear shortly...

Will the patch include a case for the regression test? Or could someone
(other than me??!!) volunteer to cover that?

- Thomas

Thomas Lockhart <lockhart@alumni.caltech.edu> writes:

Will the patch include a case for the regression test? Or could someone
(other than me??!!) volunteer to cover that?

Seems like a good idea. As the embarrassee, I'm perhaps too close
to the problem to write a good addition to the regress tests; any
volunteers?

regards, tom lane

Hiroshi Inoue <Inoue@tpf.co.jp> writes:

There was a bug report in Japan that plpgsql
crashes if select returns no row. This seems
a bug introduced by your latest change on
pl_exec.c.

Indeed. Too bad that report didn't arrive on Friday.
I am mightily embarrassed :-(

Humbled, I would say. Happens to us all.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Tom Lane writes:

In the long run I think we should abandon the notion that full path
specifications are the preferred way to locate dynamic libraries.
It would be a lot better for portability if C function libraries could
be referred to like this:

create function pltcl_call_handler() returns opaque
as 'pltcl' language 'C';

where the backend automatically assumes that a relative path is relative
to $PGLIB. I'd like to see the backend adding the file extension too,
to avoid platform dependencies (".so" is not universal).

We could have a run-time parameter that sets a path where to look for
modules. Eventually, we might also want to take a look at libtool's
libltdl, the portable interface to dynamic loading, which would do this
and a number of other things for us.

--
Peter Eisentraut peter_e@gmx.net http://funkturm.homeip.net/~peter

Peter Eisentraut <peter_e@gmx.net> writes:

Tom Lane writes:

where the backend automatically assumes that a relative path is relative
to $PGLIB. I'd like to see the backend adding the file extension too,
to avoid platform dependencies (".so" is not universal).

We could have a run-time parameter that sets a path where to look for
modules.

For obvious security reasons, the library path must only be settable by
the dbadmin, and I see no good reason that it should be changeable
on-the-fly. But we could treat it as a postmaster-start-only GUC
parameter...

regards, tom lane

Tom Lane writes:

Thomas Lockhart <lockhart@alumni.caltech.edu> writes:

Will the patch include a case for the regression test? Or could someone
(other than me??!!) volunteer to cover that?

Seems like a good idea. As the embarrassee, I'm perhaps too close
to the problem to write a good addition to the regress tests; any
volunteers?

The query that showed the bug would serve just fine.

Actually, this practice should be much more widely deployed. For each
bug, a test case should be added to guard against the bug coming back.
At least when a suitable testing infrastructure exists. For instance,
this would probably apply to each of the backend bug fixes that came in
the last few days.

Maybe it's too cumbersome to update the regression tests? Should the
files be split into smaller pieces?

--
Peter Eisentraut peter_e@gmx.net http://funkturm.homeip.net/~peter

Peter Eisentraut <peter_e@gmx.net> writes:

The query that showed the bug would serve just fine.

Most of the bug reports we get are far too bulky to be appropriate to
add to the regress tests as-is. IMHO anyway.

We do need more extensive regress tests, but I don't think that slapping
bug-report samples into them is the right way to get there ...

regards, tom lane

Tom Lane writes:

For obvious security reasons, the library path must only be settable by
the dbadmin, and I see no good reason that it should be changeable
on-the-fly. But we could treat it as a postmaster-start-only GUC
parameter...

On the fly could be useful for trying alternative sets of modules. Also,
you could include such a SET command into your function loading script.

--
Peter Eisentraut peter_e@gmx.net http://funkturm.homeip.net/~peter