Re: Incorrect visibility test function assigned to snapshot
On Mon, Feb 18, 2019 at 10:45:38AM -0300, Alvaro Herrera wrote: > None here. Thanks. And committed. -- Michael signature.asc Description: PGP signature
Re: Incorrect visibility test function assigned to snapshot
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Incorrect visibility test function assigned to snapshot
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 signature.asc Description: PGP signature
Re: Incorrect visibility test function assigned to snapshot
Michael Paquier 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
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 signature.asc Description: PGP signature
Re: Incorrect visibility test function assigned to snapshot
Bruce Momjian 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 ... */
Re: Incorrect visibility test function assigned to snapshot
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 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 +
Re: Incorrect visibility test function assigned to snapshot
Andres Freund wrote: > On May 30, 2018 9:45:32 AM EDT, Antonin Houska wrote: > >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? > > > >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
Re: Incorrect visibility test function assigned to snapshot
On May 30, 2018 9:45:32 AM EDT, Antonin Houska wrote: >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? > >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.
Re: Incorrect visibility test function assigned to snapshot
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? 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
Re: Incorrect visibility test function assigned to snapshot
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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
