Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 11.2 \(3445.5.20\))
Subject: Re: [HACKERS] logical decoding of two-phase transactions
From: Stas Kelvich <s.kelvich@postgrespro.ru>
In-Reply-To: 
 <CAMGcDxfc0yWmdkn28UBkyEjdCWqXmNgS2v=cjOgWD39mvvoY+w@mail.gmail.com>
Date: Thu, 8 Feb 2018 15:43:23 +0300
Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
 Simon Riggs <simon@2ndquadrant.com>,
 Craig Ringer <craig@2ndquadrant.com>,
 Petr Jelinek <petr.jelinek@2ndquadrant.com>,
 Sokolov Yura <y.sokolov@postgrespro.ru>,
 Andres Freund <andres@anarazel.de>,
 Dmitry Dolgov <9erthalion6@gmail.com>,
 Tom Lane <tgl@sss.pgh.pa.us>,
 Masahiko Sawada <sawada.mshk@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
 Robert Haas <robertmhaas@gmail.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <11AEF68F-14A2-4662-AAEE-87422ADA3864@postgrespro.ru>
References: <A434797D-1283-42C5-8CE5-7F1CD0B68F36@postgrespro.ru>
 <CAMsr+YF4QfAKzsfHQ7MS+5NG3aY-NG3T_DdhcdPEPkWD4fo7fQ@mail.gmail.com>
 <CAMGcDxf83P5SGnGH52=_0wRP9pO6uRWCMRwAA0nxKtZvir2_vQ@mail.gmail.com>
 <CAMsr+YGX1z5t_MccrJxd4VJPgZKx17EsAQcAQ_1iJ4dKDp0LOg@mail.gmail.com>
 <CAMGcDxeDd4uB+tvaYrtjLgykeOpa-5nwPetNM+prCQ=wpPmPaQ@mail.gmail.com>
 <CAMsr+YH_x7Lrz3arUewzAk4=FZ-+hdum-eW7gZJBAEmzo937eg@mail.gmail.com>
 <e55b1240-df0b-c5eb-bcdb-18902cdf42f8@2ndquadrant.com>
 <CAMsr+YGE84n5_1Z=xNwaRxS3Fx6WS7=FLcYtezt1wRx7VNjW0Q@mail.gmail.com>
 <CAMGcDxfin6iYuHN1_21Z7YYvPDU9qLgfBSBYti1KWiUUpJBr7g@mail.gmail.com>
 <CAMGcDxf0YDPxgG3sU=0k8zTZniEe2RhT90v4BP__3a1P4iHpEQ@mail.gmail.com>
 <45525c30-4002-861e-a82c-3b95b19d1514@2ndquadrant.com>
 <f351fc25-7b4d-3bf3-0398-d8a55bb3bd87@2ndquadrant.com>
 <CAMGcDxc0+17=5jZ7CXPpZFT9ugwLNXzPcht2J9EbaR-dXrDXCQ@mail.gmail.com>
 <CANP8+jKwhA9cofuUPP_OxEgy_2k3-LHBxw9E57MnBiS1U-3xVw@mail.gmail.com>
 <CAMGcDxca0zLfpMWTUV+1QT2NjZw+w2Zfzc2NKGfOH41UwFjQmw@mail.gmail.com>
 <dec39ab4-cc36-e01c-7a00-0087fe8644da@2ndquadrant.com>
 <CAMGcDxfxEodz8+THwaXtjpW5WuBpeYp1t-UT9Wa-NfNuWaBVGA@mail.gmail.com>
 <CAMGcDxdDqZHyi=WJcOZUZ3J-oY6cKvWhHJ2dS+4qex8xA8Jz6A@mail.gmail.com>
 <CAMGcDxdDCQipHyBeqT6crJE4WpFKT9LxV1TY2151Jy9udnMLFQ@mail.gmail.com>
 <CAMGcDxfc0yWmdkn28UBkyEjdCWqXmNgS2v=cjOgWD39mvvoY+w@mail.gmail.com>
To: Nikhil Sontakke <nikhils@2ndquadrant.com>
Precedence: bulk

Hi!

Thanks for working on this patch.

Reading through patch I=E2=80=99ve noticed that you deleted call to =
SnapBuildCommitTxn()
in DecodePrepare(). As you correctly spotted upthread there was =
unnecessary
code that marked transaction as running after decoding of prepare. =
However call
marking it as committed before decoding of prepare IMHO is still needed =
as
SnapBuildCommitTxn does some useful thing like setting base snapshot for =
parent
transactions which were skipped because of SnapBuildXactNeedsSkip().

E.g. current code will crash in assert for following transaction:

BEGIN;
SAVEPOINT one;
CREATE TABLE test_prepared_savepoints (a int);
PREPARE TRANSACTION 'x';
COMMIT PREPARED 'x';
:get_with2pc_nofilter
:get_with2pc_nofilter  <- second call will crash decoder


With following backtrace:
=20
   frame #3: 0x000000010dc47b40 =
postgres`ExceptionalCondition(conditionName=3D"!(txn->ninvalidations =3D=3D=
 0)", errorType=3D"FailedAssertion", fileName=3D"reorderbuffer.c", =
lineNumber=3D1944) at assert.c:54
    frame #4: 0x000000010d9ff4dc =
postgres`ReorderBufferForget(rb=3D0x00007fe1ab832318, xid=3D816, =
lsn=3D35096144) at reorderbuffer.c:1944
    frame #5: 0x000000010d9f055c =
postgres`DecodePrepare(ctx=3D0x00007fe1ab81b918, buf=3D0x00007ffee2650408,=
 parsed=3D0x00007ffee2650088) at decode.c:703
    frame #6: 0x000000010d9ef718 =
postgres`DecodeXactOp(ctx=3D0x00007fe1ab81b918, buf=3D0x00007ffee2650408) =
at decode.c:310

That can be fixed by calling SnapBuildCommitTxn() in DecodePrepare()
which I believe is safe because during normal work prepared transaction
holds relation locks until commit/abort and in between nobody can access
altered relations (or just I don=E2=80=99t know such situations =E2=80=94 =
that was the reason
why i had marked that xids as running in previous versions).


> On 6 Feb 2018, at 15:20, Nikhil Sontakke <nikhils@2ndquadrant.com> =
wrote:
>=20
> Hi all,
>=20
>>=20
>> PFA, patch which applies cleanly against latest git head. I also
>> removed unwanted newlines and took care of the cleanup TODO about
>> making ReorderBufferTXN structure using a txn_flags field instead of
>> separate booleans for various statuses like has_catalog_changes,
>> is_subxact, is_serialized etc. The patch uses this txn_flags field =
for
>> the newer prepare related info as well.
>>=20
>> "make check-world" passes ok, including the additional regular and =
tap
>> tests that we have added as part of this patch.
>>=20
>=20
> PFA, latest version of this patch.
>=20
> This latest version takes care of the abort-while-decoding issue along
> with additional test cases and documentation changes.
>=20
>=20


--
Stas Kelvich
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company