Обсуждение: an older problem? hash table out of memory

Michael Contzen <mcontzen@dohle.com> writes:
> I=B4ve just downloaded the snapshot-version april, 28th. Doing a join
> between two tables, I got the message:
> ERROR:  hash table out of memory. Use -B parameter to increase buffers.

I saw this too over the weekend, but didn't have time to look into it.

After a quick eyeballing of nodeHash.c, I have a question for anyone
who's worked on the hashjoin code before: why is the sizing of the
hash table driven off -B in the first place?  It looks like the table
was once allocated in shared buffer memory, but it ain't anymore; it's
just palloc'd.  Offhand it seems like the -S (sort space) parameter
might be a better thing to use as the hashtable size control.

That specific error message comes out if the hashtable "overflow area"
fills up because too many tuples landed in the same hashbucket.  So you
can provoke it easily with a test case where a table contains a few
thousand identical rows (which is in fact what my test data looked like;
dunno about Michael's).  In real life it'd have a small but definitely
not zero probability of happening.  I'm surprised that we have not seen
this complaint more before.  It's possible that the recent work on the
optimizer has made it more likely to choose hashjoin than it used to be.
Anyway, I think we'd better invest the work to make the overflow area
expansible.

> Well, increasing the buffers to -B256 results to:
> pqReadData() -- backend closed the channel unexpectedly.

Hmm, I didn't try that.  There must be some other problem as well.
Will look into it.
        regards, tom lane

Michael Contzen <mcontzen@dohle.com> writes:
> Well, increasing the buffers to -B256 results to:
> pqReadData() -- backend closed the channel unexpectedly.
> The log says:
> FATAL: s_lock(4015e1c5) at bufmgr.c:1949, stuck spinlock. Aborting.

I can't reproduce that here...  anyone else?

The "hashtable out of memory" problem is reproducible, however.
I'm on it.
        regards, tom lane

> Michael Contzen <mcontzen@dohle.com> writes:
> > Well, increasing the buffers to -B256 results to:
> > pqReadData() -- backend closed the channel unexpectedly.
> > The log says:
> > FATAL: s_lock(4015e1c5) at bufmgr.c:1949, stuck spinlock. Aborting.
> 
> I can't reproduce that here...  anyone else?
> 
> The "hashtable out of memory" problem is reproducible, however.
> I'm on it.

Historically, no one knows much about the hash routines.

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

Bruce Momjian <maillist@candle.pha.pa.us> writes:
>> The "hashtable out of memory" problem is reproducible, however.
>> I'm on it.

> Historically, no one knows much about the hash routines.

Well, I've been learning some unpleasant truths :-(.  Hope to have
some fixes to commit in the next few days.

The immediate cause of one coredump I saw was that someone who was
overenthusiastically replacing sprintf's with snprintf's had written
snprintf(tempname, strlen(tempname), ...);

where tempname points to just-allocated, quite uninitialized
memory.  Exercise for the student: how many different ways can
this go wrong?  Unsettling question: how many other places did
that someone make the same mistake??

I don't have time for this right now, but it'd be a real good idea
to grep the source for strlen near snprintf to see if this same
problem appears anywhere else...
        regards, tom lane

> Bruce Momjian <maillist@candle.pha.pa.us> writes:
> >> The "hashtable out of memory" problem is reproducible, however.
> >> I'm on it.
> 
> > Historically, no one knows much about the hash routines.
> 
> Well, I've been learning some unpleasant truths :-(.  Hope to have
> some fixes to commit in the next few days.
> 
> The immediate cause of one coredump I saw was that someone who was
> overenthusiastically replacing sprintf's with snprintf's had written
> 
>     snprintf(tempname, strlen(tempname), ...);

Here they are.  Can you properly fix them?  Looks like good news that I
found one of the ones you found.  The others may be OK:

./backend/commands/view.c:    snprintf(buf, strlen(viewName) + 5, "_RET%s", viewName);
./backend/executor/nodeHash.c:    snprintf(tempname, strlen(tempname), "HJ%d.%d", (int) MyProcPid, hjtmpcnt);
./backend/libpq/pqcomm.c:            snprintf(PQerrormsg + strlen(PQerrormsg), ERROR_MSG_LENGTH,


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

Bruce Momjian <maillist@candle.pha.pa.us> writes:
> Here they are.  Can you properly fix them?  Looks like good news that I
> found one of the ones you found.  The others may be OK:

Is that all?  Great, I was afraid we had some major problems lurking.

> ./backend/commands/view.c:    snprintf(buf, strlen(viewName) + 5, "_RET%s", viewName);

This one is OK since viewName is passed in (and is valid, we hope).

> ./backend/executor/nodeHash.c:    snprintf(tempname, strlen(tempname), "HJ%d.%d", (int) MyProcPid, hjtmpcnt);

This is the one I found.  I'm still working on nodeHash but hope to
commit fixes in the next day or so.

> ./backend/libpq/pqcomm.c:            snprintf(PQerrormsg + strlen(PQerrormsg), ERROR_MSG_LENGTH,

This is a bit bogus --- ERROR_MSG_LENGTH is the overall size of
PQerrormsg, but we're concatenating to what's already in the buffer, so
snprintf's limit should really be ERROR_MSG_LENGTH - strlen(PQerrormsg).
I applied a patch for consistency's sake, although I doubt this
statement could ever overrun the buffer in practice.
        regards, tom lane