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