Horton

Thread

Commits

  1. Mark correctly initial slot snapshots with MVCC type when built

  1. Incorrect visibility test function assigned to snapshot

    Antonin Houska <ah@cybertec.at> — 2018-05-30T07:26:33Z

    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

  2. Re: Incorrect visibility test function assigned to snapshot

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-05-30T13:28:54Z

    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

  3. Re: Incorrect visibility test function assigned to snapshot

    Antonin Houska <ah@cybertec.at> — 2018-05-30T13:45:32Z

    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

  4. Re: Incorrect visibility test function assigned to snapshot

    Andres Freund <andres@anarazel.de> — 2018-05-30T13:49:44Z

    
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.

  5. Re: Incorrect visibility test function assigned to snapshot

    Antonin Houska <ah@cybertec.at> — 2018-05-30T14:00:44Z

    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

  6. Re: Incorrect visibility test function assigned to snapshot

    Bruce Momjian <bruce@momjian.us> — 2018-06-23T15:18:56Z

    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 +

  7. Re: Incorrect visibility test function assigned to snapshot

    Antonin Houska <ah@cybertec.at> — 2019-02-08T10:59:05Z

    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

  8. Re: Incorrect visibility test function assigned to snapshot

    Michael Paquier <michael@paquier.xyz> — 2019-02-14T07:13:13Z

    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

  9. Re: Incorrect visibility test function assigned to snapshot

    Antonin Houska <ah@cybertec.at> — 2019-02-15T12:01:29Z

    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

  10. Re: Incorrect visibility test function assigned to snapshot

    Michael Paquier <michael@paquier.xyz> — 2019-02-18T04:01:20Z

    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

  11. Re: Incorrect visibility test function assigned to snapshot

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-02-18T13:45:38Z

    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

  12. Re: Incorrect visibility test function assigned to snapshot

    Michael Paquier <michael@paquier.xyz> — 2019-02-20T03:38:01Z

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

Thanks.  And committed.
--
Michael