Re: contrib/cache_scan (Re: What's needed for cache-only table scan?)

On Fri, Feb 21, 2014 at 2:19 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:

> Hello,
>
> The attached patch is a revised one for cache-only scan module
> on top of custom-scan interface. Please check it.
>

Thanks for the revised patch. Please find some minor comments.

1. memcpy(dest, tuple, HEAPTUPLESIZE);
+ memcpy((char *)dest + HEAPTUPLESIZE,
+ tuple->t_data, tuple->t_len);

For a normal tuple these two addresses are different but in case of
ccache, it is a continuous memory.
Better write a comment as even if it continuous memory, it is treated as
different only.

2. + uint32 required = HEAPTUPLESIZE + MAXALIGN(tuple->t_len);

t_len is already maxaligned. No problem of using it again, The required
length calculation is differing function to function.
For example, in below part of the same function, the same t_len is used
directly. It didn't generate any problem, but it may give some confusion.

4. + cchunk = ccache_vacuum_tuple(ccache, ccache->root_chunk, &ctid);
+ if (pchunk != NULL && pchunk != cchunk)
+ ccache_merge_chunk(ccache, pchunk);
+ pchunk = cchunk;

The merge_chunk is called only when the heap tuples are spread across two
cache chunks. Actually one cache chunk can accommodate one or more than
heap pages. it needs some other way of handling.

4. for (i=0; i < 20; i++)

Better to replace this magic number with a meaningful macro.

5. "columner" is present in sgml file. correct it.

6. "max_cached_attnum" value in the document saying as 128 by default but
in the code it set as 256.

I will start regress and performance tests. I will inform you the same once
i finish.

Regards,
Hari Babu
Fujitsu Australia