Hi,
On 2023-02-08 16:08:39 +0300, Aleksander Alekseev wrote:
> > To me it's a pretty fundamental violation of how heap visibility works.
>
> I don't think this has much to do with heap visibility. It's true that
> generally a command doesn't see its own tuples. This is done in order
> to avoid the Halloween problem which however can't happen in this
> particular case.
>
> Other than that the heap doesn't care about the visibility, it merely
> stores the tuples. The visibility is determined by xmin/xmax, the
> isolation level, etc.
Yes, and the fact is that cmin == cmax is something that we don't normally
produce, yet you emit it now, without, as far as I can tell it, a convincing
reason.
> > > That's a reasonable concern, however I was unable to break unique
> > > constraints or triggers so far:
> >
> > I think you'd have to do a careful analysis of a lot of code for that to hold
> > any water.
>
> Alternatively we could work smarter, not harder, and let the hardware
> find the bugs for us. Writing tests is much simpler and bullet-proof
> than analyzing the code.
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. This is particularly true for something like heapam,
where a lot of the tricky behaviour requires complicated interactions between
multiple connections.
> Again, to clarify, I'm merely playing the role of Devil's advocate
> here. I'm not saying that the patch should necessarily be accepted,
> nor am I 100% sure that it has any undiscovered bugs. However the
> arguments against received so far don't strike me personally as being
> particularly convincing.
I've said my piece, as-is I vote to reject the patch.
Greetings,
Andres Freund