Re: More vacuum.c refactoring - Mailing list pgsql-hackers

On Thu, 10 Jun 2004 17:19:22 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>This does not make me comfortable.

I understand you, honestly.  Do I read between your lines that you
didn't review my previous vacuum.c refactoring patch?  Please do.  It'd
make *me* more comfortable.

>  You *think* that two different bits of code are doing the same thing,

I see three significant differences between the code in repair_frag()
and vacuum_page().

1) vacuum_page() hasAssert(vacpage->offsets_used == 0);

vacpage is the last VacPage that has been inserted into Nvacpagelist.
It is allocated in line 1566, offsets_used is immediately set to 0 and
never changed.  So this Assert(...) doesn't hurt.

2) In vacuum_page() the lp_flags are set inside a critical section.

This is no problem because the clear-used-flags loop does not
elog(ERROR, ...).  Please correct me if I'm wrong.

3) vacuum_page() uses vacpage->offsets to locate the itemids that are to
be updated.

If we can show that these are the same itemids that belong to the tuples
that are found by inspecting the tuple headers, then the two code
snippets are equivalent.  The first hint that this is the case isAssert(vacpage->offsets_free == num_tuples);

So both spots expect to update the same number of itemids.  What about
the contents of the offsets[] array?  Offset numbers are entered into
this array by statements like
vacpage->offsets[vacpage->offsets_free++] = offnum;

in or immediately after the walk-along-page loop.  These assignments are
preceded either by code that sets the appropriate infomask bits or by
assertions that the bits are already set appropriately.

The rest (from PageRepairFragmentation to END_CRIT_SECTION) is
identical.

> so you want to hack up vacuum.c?  This
>module is delicate code --- we've had tons of bugs there in the past

But why is it so delicate?  Not only because it handles difficult
problems, but also because it is written in a not very
maintenance-friendly way.  Before I started refactoring the code
repair_frag() had more than 1100 lines and (almost) all variables used
anywhere in the function were declared at function level.

We cannot declare a code freeze for a module just because it is so badly
written that every change is likely to break it.  Someday someone will
*have* to change it.

Better we break it today in an effort to make the code clearer.  

>--- and no I have zero confidence that passing the regression tests
>proves anything

Unfortunately you are right :-(

ServusManfred