Thread: Incorrect visibility test function assigned to snapshot

Incorrect visibility test function assigned to snapshot

From
Antonin Houska
Date:
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


Re: Incorrect visibility test function assigned to snapshot

From
Alvaro Herrera
Date:
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


Re: Incorrect visibility test function assigned to snapshot

From
Antonin Houska
Date:
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


Re: Incorrect visibility test function assigned to snapshot

From
Andres Freund
Date:

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.


Re: Incorrect visibility test function assigned to snapshot

From
Antonin Houska
Date:
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


Re: Incorrect visibility test function assigned to snapshot

From
Bruce Momjian
Date:
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 +


Re: Incorrect visibility test function assigned to snapshot

From
Antonin Houska
Date:
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 ... */

Re: Incorrect visibility test function assigned to snapshot

From
Michael Paquier
Date:
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

Re: Incorrect visibility test function assigned to snapshot

From
Antonin Houska
Date:
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


Re: Incorrect visibility test function assigned to snapshot

From
Michael Paquier
Date:
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

Re: Incorrect visibility test function assigned to snapshot

From
Alvaro Herrera
Date:
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


Re: Incorrect visibility test function assigned to snapshot

From
Michael Paquier
Date:
On Mon, Feb 18, 2019 at 10:45:38AM -0300, Alvaro Herrera wrote:
> None here.

Thanks.  And committed.
--
Michael

Attachment