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