Re: I: About "Our CLUSTER implementation is pessimal" patch

Hi all,

while testing the seq scan + sort CLUSTER implementation, I've found a
bug in write/readtup_rawheap.

The functions are needed by the sort part.
The code in

http://archives.postgresql.org/pgsql-hackers/2008-08/msg01371.php

didn't copy the whole tuple, but only the HeapTuple "header": the t_data
part wasn't copied (at least, that's my understanding), The original code
is at the bottom of the email. It didn't work (the table wasn't fully clustered
at the end of the CLUSTER command).

So I modified write/readtup_rawheap: they are supposed to store/retrieve
both the "fixed" part of HeapTupleData, plus the "variable" part t_data.
But a get a lot of:
WARNING: problem in alloc set TupleSort: detected write past chunk end
in block 0x96853f0, chunk 0x968723c
WARNING: problem in alloc set TupleSort: detected write past chunk end
in block 0x96853f0, chunk 0x96872c8
warnings when calling "tuplesort_end" and some of the data gets garbled
after the CLUSTER command.

What am I doing wrong? Using the debugger, data coming out of
readtup_rawheap seems fine...

I *really* need your help here...

static void
writetup_rawheap(Tuplesortstate *state, int tapenum, SortTuple *stup)
{
HeapTuple tuple = (HeapTuple) stup->tuple;
tuple->t_len += sizeof(HeapTupleData); /* write out the header as well */

LogicalTapeWrite(state->tapeset, tapenum,
tuple, sizeof(HeapTupleData));
LogicalTapeWrite(state->tapeset, tapenum, tuple->t_data,
tuple->t_len-sizeof(HeapTupleData));
if (state->randomAccess) /* need trailing length word? */
LogicalTapeWrite(state->tapeset, tapenum,
tuple, sizeof(tuple->t_len));

FREEMEM(state, GetMemoryChunkSpace(tuple));
heap_freetuple(tuple);
}

static void
readtup_rawheap(Tuplesortstate *state, SortTuple *stup,
int tapenum, unsigned int tuplen)
{
HeapTuple tuple = (HeapTuple) palloc(sizeof(HeapTupleData));
tuple->t_data = (HeapTupleHeader) palloc(tuplen-sizeof(HeapTupleData));

USEMEM(state, GetMemoryChunkSpace(tuple));

tuple->t_len = tuplen - sizeof(HeapTupleData);
if (LogicalTapeRead(state->tapeset, tapenum, &tuple->t_self, sizeof(HeapTupleData)-sizeof(tuplen))
!= sizeof(HeapTupleData)-sizeof(tuplen))
elog(ERROR, "unexpected end of data");
if (LogicalTapeRead(state->tapeset, tapenum, tuple->t_data, tuple->t_len) != tuple->t_len)
elog(ERROR, "unexpected end of data");
if (state->randomAccess) /* need trailing length word? */
if (LogicalTapeRead(state->tapeset, tapenum, &tuplen,
sizeof(tuplen)) != sizeof(tuplen))
elog(ERROR, "unexpected end of data");

stup->tuple = tuple;
/* set up first-column key value */
if (state->indexInfo->ii_Expressions == NULL)
{
/* no expression index, just get the key datum value */
stup->datum1 = heap_getattr((HeapTuple) stup->tuple,
state->indexInfo->ii_KeyAttrNumbers[0],
state->tupDesc,
&stup->isnull1);
}
else
{
[...] expression index part, removed for clarity
}
}

Original code:

static void
writetup_rawheap(Tuplesortstate *state, int tapenum, SortTuple *stup)
{
HeapTuple tuple = (HeapTuple) stup->tuple;
tuple->t_len += HEAPTUPLESIZE; /* write out the header as well */

LogicalTapeWrite(state->tapeset, tapenum,
tuple, tuple->t_len);

if (state->randomAccess) /* need trailing length word? */
LogicalTapeWrite(state->tapeset, tapenum,
tuple, sizeof(tuple->t_len));

FREEMEM(state, GetMemoryChunkSpace(tuple));
heap_freetuple(tuple);
}

static void
readtup_rawheap(Tuplesortstate *state, SortTuple *stup,
int tapenum, unsigned int tuplen)
{
HeapTuple tuple = (HeapTuple) palloc(tuplen);

USEMEM(state, GetMemoryChunkSpace(tuple));

tuple->t_len = tuplen - HEAPTUPLESIZE;
if (LogicalTapeRead(state->tapeset, tapenum, &tuple->t_self,
tuplen-sizeof(tuplen)) != tuplen-sizeof(tuplen))
elog(ERROR, "unexpected end of data");
if (state->randomAccess) /* need trailing length word? */
if (LogicalTapeRead(state->tapeset, tapenum, &tuplen,
sizeof(tuplen)) != sizeof(tuplen))
elog(ERROR, "unexpected end of data");

stup->tuple = tuple;
/* set up first-column key value */
stup->datum1 = heap_getattr(tuple,
state->scanKeys[0].sk_attno,
state->tupDesc,
&stup->isnull1);
}