Michael Paquier <mich...@paquier.xyz> wrote: > 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.
I'm not sure. SnapBuildBuildSnapshot() only creates snapshot for the logical replication purposes and the "snapshot_type" argument would make the function look like it can handle all snapshot types. If adding an argument, I'd prefer a boolean variable (e.g. "historic") telling whether user wants SNAPSHOT_HISTORIC_MVCC or SNAPSHOT_MVCC. But that may deserve a separate patch. As for the bug fix, I think the additional assignment does not make things worse because SnapBuildInitialSnapshot() already does overwrite some fields: "xip" and "xnt". > > @@ -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. o.k. I don't insist on using this comment. -- Antonin Houska Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26, A-2700 Wiener Neustadt Web: https://www.cybertec-postgresql.com
