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

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.