From: Antonin Houska <ah@cybertec.at>
To: pgsql-hackers@postgresql.org
Subject: Incorrect visibility test function assigned to snapshot
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Date: Wed, 30 May 2018 09:26:33 +0200
Message-ID: <23215.1527665193@localhost>
Precedence: bulk

In the header comment, SnapBuildInitialSnapshot() claims to set
snapshot->satisfies to the HeapTupleSatisfiesMVCC test function, and indeed=
 it
converts the "xid" array to match its semantics (i.e. the xid items eventua=
lly
represent running transactions as opposed to the committed ones). However t=
he
test function remains HeapTupleSatisfiesHistoricMVCC as set by
SnapBuildBuildSnapshot().

I suppose this is a bug: HeapTupleSatisfiesHistoricMVCC expects the committ=
ed
transactions in the snapshot->subxip array, however the snapshot returned by
SnapBuildInitialSnapshot() leaves this array empty. And even if the function
used snapshot->xip, it'd find the running transactions there instead of tho=
se
committed.

This is what I propose as a fix:

diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/repl=
ication/logical/snapbuild.c
new file mode 100644
index 4123cde..53e8b95
*** a/src/backend/replication/logical/snapbuild.c
--- b/src/backend/replication/logical/snapbuild.c
*************** SnapBuildInitialSnapshot(SnapBuild *buil
*** 619,624 ****
--- 619,625 ----
=20=20
        snap->xcnt =3D newxcnt;
        snap->xip =3D newxip;
+       snap->satisfies =3D HeapTupleSatisfiesMVCC;
=20=20
        return snap;
  }

--=20
Antonin Houska
Cybertec Sch=C3=B6nig & Sch=C3=B6nig GmbH
Gr=C3=B6hrm=C3=BChlgasse 26, A-2700 Wiener Neustadt
Web: https://www.cybertec-postgresql.com