Thread: Incorrect visibility test function assigned to snapshot
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 eventually
represent running transactions as opposed to the committed ones). However the
test function remains HeapTupleSatisfiesHistoricMVCC as set by
SnapBuildBuildSnapshot().
I suppose this is a bug: HeapTupleSatisfiesHistoricMVCC expects the committed
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 those
committed.
This is what I propose as a fix:
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/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 ----
snap->xcnt = newxcnt;
snap->xip = newxip;
+ snap->satisfies = HeapTupleSatisfiesMVCC;
return snap;
}
--
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26, A-2700 Wiener Neustadt
Web: https://www.cybertec-postgresql.com
On 2018-May-30, Antonin Houska wrote: > 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 eventually > represent running transactions as opposed to the committed ones). However the > test function remains HeapTupleSatisfiesHistoricMVCC as set by > SnapBuildBuildSnapshot(). Interesting. While this sounds like an oversight that should have horrible consequences, it's seems not to because the current callers don't seem to care about the ->satisfies function. Are you able to come up with some scenario in which it causes an actual problem? -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > On 2018-May-30, Antonin Houska wrote: > > > 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 eventually > > represent running transactions as opposed to the committed ones). However the > > test function remains HeapTupleSatisfiesHistoricMVCC as set by > > SnapBuildBuildSnapshot(). > > Interesting. While this sounds like an oversight that should have > horrible consequences, it's seems not to because the current callers > don't seem to care about the ->satisfies function. Are you able to come > up with some scenario in which it causes an actual problem? Right, the current callers in the core do not seem to use that function. I hit the issue when doing and testing some changes in an extension (pg_squeeze). -- Antonin Houska Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26, A-2700 Wiener Neustadt Web: https://www.cybertec-postgresql.com
On May 30, 2018 9:45:32 AM EDT, Antonin Houska <ah@cybertec.at> wrote: >Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > >> On 2018-May-30, Antonin Houska wrote: >> >> > 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 >eventually >> > represent running transactions as opposed to the committed ones). >However the >> > test function remains HeapTupleSatisfiesHistoricMVCC as set by >> > SnapBuildBuildSnapshot(). >> >> Interesting. While this sounds like an oversight that should have >> horrible consequences, it's seems not to because the current callers >> don't seem to care about the ->satisfies function. Are you able to >come >> up with some scenario in which it causes an actual problem? > >Right, the current callers in the core do not seem to use that >function. I hit >the issue when doing and testing some changes in an extension >(pg_squeeze). What is that extension doing with that snapshot? Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Andres Freund <andres@anarazel.de> wrote: > On May 30, 2018 9:45:32 AM EDT, Antonin Houska <ah@cybertec.at> wrote: > >Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > > >> On 2018-May-30, Antonin Houska wrote: > >> > >> > 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 > >eventually > >> > represent running transactions as opposed to the committed ones). > >However the > >> > test function remains HeapTupleSatisfiesHistoricMVCC as set by > >> > SnapBuildBuildSnapshot(). > >> > >> Interesting. While this sounds like an oversight that should have > >> horrible consequences, it's seems not to because the current callers > >> don't seem to care about the ->satisfies function. Are you able to > >come > >> up with some scenario in which it causes an actual problem? > > > >Right, the current callers in the core do not seem to use that > >function. I hit > >the issue when doing and testing some changes in an extension > >(pg_squeeze). > > What is that extension doing with that snapshot? It fetches data from a table in order to insert them into a new table, to eliminate bloat. Something like pg_repack, but it uses logical decoding instead of triggers to capture concurrent data changes. -- Antonin Houska Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26, A-2700 Wiener Neustadt Web: https://www.cybertec-postgresql.com
On Wed, May 30, 2018 at 09:28:54AM -0400, Alvaro Herrera wrote: > On 2018-May-30, Antonin Houska wrote: > > > 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 eventually > > represent running transactions as opposed to the committed ones). However the > > test function remains HeapTupleSatisfiesHistoricMVCC as set by > > SnapBuildBuildSnapshot(). > > Interesting. While this sounds like an oversight that should have > horrible consequences, it's seems not to because the current callers > don't seem to care about the ->satisfies function. Are you able to come > up with some scenario in which it causes an actual problem? Uh, are we going to fix this anyway? Seems we should. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Bruce Momjian <bruce@momjian.us> wrote: > On Wed, May 30, 2018 at 09:28:54AM -0400, Alvaro Herrera wrote: > > On 2018-May-30, Antonin Houska wrote: > > > > > 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 eventually > > > represent running transactions as opposed to the committed ones). However the > > > test function remains HeapTupleSatisfiesHistoricMVCC as set by > > > SnapBuildBuildSnapshot(). > > > > Interesting. While this sounds like an oversight that should have > > horrible consequences, it's seems not to because the current callers > > don't seem to care about the ->satisfies function. Are you able to come > > up with some scenario in which it causes an actual problem? > > Uh, are we going to fix this anyway? Seems we should. Sorry, I forgot. Patch is below and I'm going to add an entry to the next CF. -- Antonin Houska Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26, A-2700 Wiener Neustadt Web: https://www.cybertec-postgresql.com diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c index 2f185f7823..3394abb602 100644 --- a/src/backend/replication/logical/snapbuild.c +++ b/src/backend/replication/logical/snapbuild.c @@ -615,6 +615,8 @@ SnapBuildInitialSnapshot(SnapBuild *builder) TransactionIdAdvance(xid); } + /* And of course, adjust snapshot type accordingly. */ + snap->snapshot_type = SNAPSHOT_MVCC; snap->xcnt = newxcnt; snap->xip = newxip; diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c index 6e02585e10..bc0166cc6f 100644 --- a/src/backend/utils/time/snapmgr.c +++ b/src/backend/utils/time/snapmgr.c @@ -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; + parseVxidFromText("vxid:", &filebuf, path, &src_vxid); src_pid = parseIntFromText("pid:", &filebuf, path); /* we abuse parseXidFromText a bit here ... */
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
Attachment
Michael Paquier <michael@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
On Fri, Feb 15, 2019 at 01:01:29PM +0100, Antonin Houska wrote: > 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". Ah, right. I somewhat missed that. Let's move on with merging your patch then. Are there any objections about that? -- Michael
Attachment
On 2019-Feb-18, Michael Paquier wrote: > On Fri, Feb 15, 2019 at 01:01:29PM +0100, Antonin Houska wrote: > > 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". > > Ah, right. I somewhat missed that. Let's move on with merging your > patch then. Are there any objections about that? None here. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Feb 18, 2019 at 10:45:38AM -0300, Alvaro Herrera wrote: > None here. Thanks. And committed. -- Michael