Re: Open 7.3 items: heap tuple header - Mailing list pgsql-hackers

On Fri, 16 Aug 2002 12:25:37 -0400 (EDT), Bruce Momjian
<pgman@candle.pha.pa.us> wrote:
>Manfred Koizar wrote:
>> This is the main point of disagreement:  Tom Lane wants lighter
>> macros, I want heavier macros.  Which direction shall we go?
>
>Could you or Tom explain that in a way that others could understand.

I'm not sure we understand each other :-)

>My guess is that you want the sanity checks in the macros, and Tom wants
>more of them in the main code?

I guess we agree on having the sanity checks in the macros.  It's more
on what the macros are allowed to change and what decisions they are
allowed to take.  Do the following quotes make it clearer?

:On Sat, 20 Jul 2002 16:27:14 -0400, Tom Lane <tgl@sss.pgh.pa.us>
:wrote:
:>I'd recommend redesigning the HeapTupleHeaderSet macros so that they
:>do not do any setting of t_infomask bits, or even take any conditional
:>action based on them,

to which I replied on Sun, 21 Jul 2002 23:00:22 +0200:
:The HEAP_XMIN_IS_XMAX bit is exclusively managed by these macros.
:Pulling the handling of this bit out of the macros and spreading it to
:the places, where the macros are used, cannot make the whole thing
:more robust.  This would mean, the caller had to decide whether to
:store Cmax into t_cid or t_xmax...

Tom:
:> but solely Assert() that the bits are already
:>in the appropriate state to allow storing of the value to be stored.
:>Then, all uses have to be checked to ensure that t_infomask is coerced
:>into the right state *before* doing HeapTupleHeaderSetFoo.

me:
:Apart from HEAP_XMIN_IS_XMAX this was my intention;  we already do
:this with HEAP_MOVED.  I could add an assertion to SetCmax:
:    Assert(!((tup)->t_infomask & HEAP_XMAX_INVALID));
:
:OTOH I thought about putting *more* logic into the macros to make
:their use less fragile.  For example SetXmaxInvalid could set the
:HEAP_XMAX_INVALID bit, likewise SetCminInvalid with XMIN_INVALID.

>> BTW, my changes have been criticized with words like "vague unease",
>> "zero confidence", "very obviously not robust", "do not trust the
>> current code at all" etc., while from day one all my patches have
>> passed all regression tests.
>
>I totally agree with you here.  You code has been great, and it did
>something (reduce tuple size) that no one else thought possible.

Thanks a lot!  But I was not fishing for compliments, I was just
preparing for the next sentence:  There is something wrong with the
regression tests.  In fact "vague unease" more than once was a proper
description for my own feelings when I posted a patch, because I
didn't know whether I could trust the tests.  If core developers share
this unease, I find myself in good company.

>>  This makes me wonder whether there is
>> something wrong with the regression tests ...
>
>However, I should add that the regression tests test only a small subset
>of how the database has to operate.

So, please, could you add to TODO:
* Add more regression tests

>There are so many bizarre
>conditions that we can't test them all.

When Tom Lane recently posted a way to reproduce a bug ("No one parent
tuple was found", CASE 1) I thought about how to add this case to the
regression tests, but we have no vehicle for testing concurrent
transactions.  TODO:
* Build a test vehicle for concurrent transactions* Add even more regression tests

ServusManfred