Re: Segfault on ANALYZE in SERIALIZABLE isolation

Hi,

Thanks for the report Joe!

I've pushed a fix for this.

I ended up going down the path of making scan_begin's arguments a
bitmask. Given that several people expressed desire for that, and that
recognizing analyze scans would have required a new argument, that
seemed the most reasonable course.

I think the code handling sync/strat in heapam's initscan() could be
simplified so we don't set/unset the flags (and previously the booleans)
multiple times. But that seems like it ought to be done separately.

I'd normally have asked for a round of feedback for the changes, but it
seems more urgent to get something out for beta1. As the changes are all
below tableam, we can adjust this later without causing much trouble.

Regards,

Andres

On 2019-05-18 13:12:41 -0700, Andres Freund wrote:
> Hi,
> 
> On 2019-05-18 15:48:47 -0400, Tom Lane wrote:
> > Andres Freund <andres@anarazel.de> writes:
> > > Not quite - that was about the DML callbacks, this is about the scan itself. And while we have a snapshot allocated, the analyze version of the beginscan intentionally doesn't take a snapshot.
> > 
> > Uh, what?  That's a *huge* regression.  See, eg, 7170268ef.  We
> > really want ANALYZE to act as though it's reading a normal MVCC
> > snapshot.
> 
> Hm? That's not new at all? In 11 we just do:
> 
> 			switch (HeapTupleSatisfiesVacuum(&targtuple,
> 											 OldestXmin,
> 											 targbuffer))
> 
> I.e. unrelated to the tableam changes there's no mvcc snapshot in for
> visibility determinations. And that's not changed, heap's implementation
> still uses HTSV.  We do *hold* a snapshot, but that's all outside of
> analyze.c afaik.

Greetings,

Andres Freund