Re: Incorrect visibility test function assigned to snapshot

On Fri, Feb 08, 2019 at 11:59:05AM +0100, Antonin Houska wrote:
> Sorry, I forgot. Patch is below and I'm going to add an entry to the
> next CF.

> @@ -615,6 +615,8 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
>
> TransactionIdAdvance(xid);
> }
> + /* And of course, adjust snapshot type accordingly. */
> + snap->snapshot_type = SNAPSHOT_MVCC;

Wouldn't it be cleaner to have an additional argument to
SnapBuildBuildSnapshot() for the snapshot type? It looks confusing to
me to overwrite the snapshot type after initializing it once.

> @@ -1502,6 +1502,13 @@ ImportSnapshot(const char *idstr)
> */
> memset(&snapshot, 0, sizeof(snapshot));
>
> + /*
> + * Do not rely on the fact that SNAPSHOT_MVCC is zero. (The core code
> + * currently does not use this field of imported snapshot, but let's keep
> + * things consistent.)
> + */
> + snapshot.snapshot_type = SNAPSHOT_MVCC;

Okay for this one, however the comment does not add much value.
--
Michael