On Tue, Jul 12, 2011 at 4:47 PM, Pavan Deolasee
<pavan.deolasee@gmail.com> wrote:
> Comments ?
I was going to spend some time reviewing this, but I see that (1) it
has bit-rotted slightly - there is a failing hunk in pg_class.h and
(2) some of the comments downthread seem to suggest that you're
thinking about whether to revise this somewhat, in particular by using
some counter other than an LSN. Are you planning to submit an updated
version?
A few comments on this version just reading through it:
- In lazy_scan_heap, where you've made the call to
RecordPageWithFreeSpace() unconditional, the comment change you made
immediately above is pretty half-baked. It still refers to
lazy_vacuum_heap, which you've meanwhile removed. You need to rewrite
the whole comment, I think.
- Instead of passing bool need_vaclsn to PageRepairFragmentation(),
how about passing Offset new_special_size? Currently,
PageRepairFragmentation() doesn't know whether it's looking at a heap
page or an index page, and it would be nice to keep it that way. It's
even possible that expanding the special space opportunistically
during page defragmentation could be useful in other contexts besides
this. Or perhaps contracting it.
- itemid.h seems a bit schizophrenic about dead line pointers. Here,
you've decided that it's OK for lp_flags == LP_DEAD && lp_off == 1 to
mean dead-vacuumed, but there existing code says:
#define LP_DEAD 3 /* dead, may or may
not have storage */
AFAICT, the actual situation here is that indexes sometimes use dead
line pointers with storage, but the heap doesn't; thus, the heap can
safely use the storage bits of dead line pointers to mean something
else, but indexes can't. I think the comments throughout itemid.h
should be adjusted to bring this out a bit more clearly, though.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
