On Tue, Jul 12, 2011 at 4:47 PM, Pavan Deolasee <pavan.deola...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
