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(at)2ndquadrant(dot)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(at)cybertec(dot)at> wrote:
>Alvaro Herrera <alvherre(at)2ndquadrant(dot)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(at)anarazel(dot)de> wrote:

> On May 30, 2018 9:45:32 AM EDT, Antonin Houska <ah(at)cybertec(dot)at> wrote:
> >Alvaro Herrera <alvherre(at)2ndquadrant(dot)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(at)momjian(dot)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(at)momjian(dot)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

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

Michael Paquier <michael(at)paquier(dot)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

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