Hi,
On 2023-02-09 13:06:04 +0300, Aleksander Alekseev wrote:
> > Yes, and the fact is that cmin == cmax is something that we don't normally
> > produce
>
> Not sure if this is particularly relevant to this discussion but I
> can't resist noticing that the heap doesn't even store cmin and
> cmax... There is only HeapTupleHeaderData.t_cid and flags. cmin/cmax
> are merely smoke and mirrors we use to trick a user.
No, they're not just that. Yes, cmin/cmax aren't both stored on-disk, but if
both are needed, they *are* stored in-memory. We can do that because it's only
ever needed from within a transaction.
> > That's a spectactularly wrong argument in almost all cases. Unless you have a
> > way to get to full branch coverage or use a model checker that basically does
> > the same, testing isn't going to give you a whole lot of confidence that you
> > haven't introduced bugs.
>
> But neither will reviewing a lot of code...
And yet my review did figure out that your patch would have visibility
problems, which you did end up having, as you noticed yourself downthread :)
> > I've said my piece, as-is I vote to reject the patch.
>
> Fair enough. I'm merely saying that rejecting a patch because it
> doesn't include a TLA+ model is something novel :)
I obviously am not suggesting that (although some things could probably
benefit). Just that not having an example showing something working, isn't
sufficient to consider something suspicious OK.
And changes affecting heapam.c visibility semantics need extremely careful
review, I have the battle scars to prove that to be true :P.
Greetings,
Andres Freund