Subject: Re: [HACKERS] logical decoding of two-phase transactions
To: Arseny Sher <a.sher@postgrespro.ru>,
 Nikhil Sontakke <nikhils@2ndquadrant.com>
Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>,
 Andres Freund <andres@anarazel.de>, Robert Haas <robertmhaas@gmail.com>,
 David Steele <david@pgmasters.net>,
 Andrew Dunstan <andrew.dunstan@2ndquadrant.com>,
 Simon Riggs <simon@2ndquadrant.com>, Craig Ringer <craig@2ndquadrant.com>,
 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
 Sokolov Yura <y.sokolov@postgrespro.ru>,
 Stas Kelvich <s.kelvich@postgrespro.ru>,
 Dmitry Dolgov <9erthalion6@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
 Masahiko Sawada <sawada.mshk@gmail.com>,
 PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
References: <20180719194208.j5m7qhdmqp46d6yg@alap3.anarazel.de>
 <20180720145836.gxwhbftuoyx5h4gc@alap3.anarazel.de>
 <CAMGcDxebtC_ZO6EtLA1MgJ6sWbud3Fjtq9HsBwxxhYfoHJdANw@mail.gmail.com>
 <20180723135009.w25re6qxbb7njrbd@alap3.anarazel.de>
 <CAMGcDxdJNZVq1mX9dZbwEniigi=af8H5dt62C+q2UHwsE+BTaw@mail.gmail.com>
 <20180723141435.ercxxh3bj2uc2mjm@alap3.anarazel.de>
 <3B37B16B-A473-4352-ABCF-F23AE314D4F7@2ndquadrant.com>
 <CAMGcDxeBcMBF0KZ9qMpcb07uX6uLNO5hk=VWCQC514oZuofLmw@mail.gmail.com>
 <20180726200241.aje4dv4jsv25v4k2@alap3.anarazel.de>
 <CAMGcDxfVBiBXt4F7v58_DfqFD9xnmU8hBEiyqoypon6Ftroe_Q@mail.gmail.com>
 <a5ba1686-bb79-f2c3-568d-993e4c53b920@2ndquadrant.com>
 <CAMGcDxfWORpkaOaNhFwS4839R_w4wtDgnB8Wj2TmAQXnV78HsQ@mail.gmail.com>
 <CAMGcDxe276HwzQ1ErqMHKPf=KTvdSGgu3jYaR+j3-368+amZSA@mail.gmail.com>
 <2ba9c978-ede8-35bf-14ec-45570bf0f6aa@2ndquadrant.com>
 <87h8fbxjxw.fsf@ars-thinkpad>
 <CAMGcDxfnkjtB0MaLRp6wdGoTbGr20yq=dwGf5V26guKBNCtacg@mail.gmail.com>
 <87a7k2996m.fsf@ars-thinkpad>
From: Petr Jelinek <petr.jelinek@2ndquadrant.com>
Openpgp: preference=signencrypt
Autocrypt: addr=petr.jelinek@2ndquadrant.com; prefer-encrypt=mutual; keydata=
 mQINBFawwSABEAC45Vua8rSggpM2kkMMmoOHz51a+fi3OXFfnrStVh80pM2/J9nSzjrIKQKf
 H5PtRnoYzSKnoUDoLOkLwDYitK3HzUUWXma+AWAqef2Y0GSZ7A3jJkD+JHOfkpS1CIpkEB/e
 WjA8Wqegwzc/OEE5111/l0Q1e6jxgZI86h8MMnm35Of1Y6Lz4kqWCyxHhaTJKTCkq66ICQ2C
 VJkbvuUoEE+Hr/I38dPHTAqIlCnycVnJPLHBQ/UYGHR/USF8qI/qJJx6tJcNAEqz69QZ/VlY
 31UyJHdYe5NtBVz+rYZCOVWfpe3crbHuq9FjAPT+xcZefdxridmMKuW8eBmmXI9iZ3EG+X8J
 IsdLWL8QoqW0lnO1otYz1QDjdOvbEpq+u5/TAU8pgtaYH2J+6/GaQ6hMLyLBkaIAtnqlLCzq
 pxXcxMsFT14P5/rZA7EbBI8RS1ZIWJBlS2oO+dtVuENLJUC0SpptaRyn/uyOIopt7hSmBekL
 SzFnet7P9qofFJY+fQEz84v0G1gvwonHGHiCGgLpbMdPtnfPzda1aiTURvvOD7w/RmX1/ogk
 TqSGO4ejVNO/28QYouxmxzy8jbbIEiLCbHpLR/M/TvYHHPURxcx+MxVl3G+d/4Ihy4abxOdW
 pe9zLIcVMvqiUFQO28/79Gfhb7G4K+L21ORB82QYkzm3M7ebXQARAQABtCNQZXRyIEplbMOt
 bmVrIDxwam1vZG9zQHBqbW9kb3MubmV0PokCQAQTAQoAKgIbAwUJBaOagAULCQgHAwUVCgkI
 CwUWAgMBAAIeAQIXgAUCVrDBqwIZAQAKCRBci2l0giH4y/QJEACfiLHyvxlwMvo3Y88Qa9QI
 i7Hu3ZW+xxojojdRd7do0nBnf2a4YstD9u6ZOLJx4f5Ijb/8LxY10NgLugmfQ7sw2W8ui6wT
 NiAeNHpr4wpT3IV+V53psOMRYPdq2zNTLO4iNxqPFPgNufR/m4FqcGe0Wo7U0wd1p0MErXTg
 EXJDVmjPDkYnzE9ejSCOxF57+o3xs6+eW9Al2m3nqP8JCzpauCr0gqpQzCaw0VZ05aLL0t9E
 B0fdm90+ydVe10Wg+I348005smuSSckBCHZqUfjjWvjwE6v/6dZ3/50ycAbi6cTe7Fo4kOSO
 sXBFXkzYGWYrEFPkzK0zDH65D98gL1+l9uokCR2t/OYVwudbUCV3cS3xRd0eFgHbXuzV+aZc
 jRjqkPfpCA2loKtrX2U739mpVSHnpIfSBf48fLg7O5dVn5ZCrydGkEzgvyynPo7IikQrIA02
 91IoSp4RisKsJpaXHwHu6DH2rDCbYFumAMgXk2osLFX6VMPoSWKVRkr87A0VLafCtwaqr6Hy
 sFE6Lmu7jP4/BDr+9A2MXK+v0yYrXqqsZN6V9856ywlZrWK3MB6jc2bKpKS2YshneKxpitNV
 0ITs2Li0OlAXOEHE0eXwI93POgLoyH+8JiNbem3orw3Mnc+7XpIY86AejwUiYl8T7EO/2tIx
 HDaXLK3BPNOdH7kCDQRWsMEgARAA6Z3NrDLdcFF/P3TD/F7WQtMALEEKn5TXSF/K2VHt21GD
 8tGY1DZhbxjLuQJeheY/2L4F4Y5WCkAoUdBXQs7nIauN+63EL88/qPdH6f3+/HZ+cmPG1E5o
 dBABCkC42eGcI5O8/9TAaFQcSzg86pdAE8v927tnc6aH4W4/Mj7RJNlQiZaz2ngCC9mr5uu+
 CysOTfKCS0llNUkExZOJzZsQU3zDkcg3+xmgtHkCjglJ006KuytHj55HR6TzaKaAW8coHGoS
 OI/BQ9KAzvlj+32/zS8FSISZSTY3spMpraADsYklejCeJOmqQ3nTrCtwlX3hTSggfr4eelKA
 hdZKs1rIimNBtX90jJc0u2A/GmC959aTSrCUcfaPRweKUuexg7jxuhXyg9br2GscD2G6W7EV
 c+3s4dOHgoJFVMBBIqTtZviaNJ63IsZIFTYJzGKKMHOioZ3naoBVav0w63waDzj82nPYMIPj
 TxnClKiecuBQQPzMaG0jTmK0lis6rO0nIXrCwca8e/3Xzvi7sykOdcz9t4ef8HrDCCkG0yDO
 o8qHy1e7EtmJD7kWffIXZMt4TZVxpiOqgWtv+heJAfmqzr8pkNLUubmgvNdnakXJOJT41Lui
 50C5OhpU9mfFCIDOeZXTQLsHA4KYaL8W/qcjv/YDmvpJtnK24y6tByWuTu7r/SMAEQEAAYkC
 JQQYAQoADwUCVrDBIAIbDAUJBaOagAAKCRBci2l0giH4yyWpD/91jd4BMN2CDYqLtU7kaoAa
 aXnRVpf0z+R8rJ4LZJX1UNbHjsdClMQLfdBuZeDsdYar4iMfwScz6fiCiRwaRpr3XT6znaM0
 wba6fjzEqVSHr3WB1jrb/TMiNaNgQJaRPhI5C7VsLuPrv5/L8gFmyjl712q7F3busGFBKrjK
 qTESMu+sHNvVeC3QyBFCe2fudbAJdaaRsUoTbAZYiowVoQHOow8eBoWY+RzQoYGuw1fls5Ck
 85N73pYHXzjfk1s49sDKTYI263h9wbGGA234biHRI3NQN6sSYWRr/u5GOwTm/tzyadRj0pnZ
 S6KyLSEL4qx8jiyniD/XLoftlvR4YWQ0BXveEElx2n3TThLL1cVtOI4qE9BZKm32euBqwLta
 k9Jc659hmZB9E9aWLo8KcLjngwqSZupoHRPZ3yAlgZkpDoTIZpMCGxGEuuxgnosRbNpOJ1fE
 nNXenYYfi87tHRUCRKeHhoKkfGZEgC9zv9Y97j75npqV2tQyh4fNQT1tGdBigtvzI4nHM1/1
 PLJtJBdRHucP76vUH9RlP9EE/Sg5DFQqsfSYPqxLW9r+ZfSEeT0APdz7TQC9MzNzUhyXvA5a
 RQQCRMXKbGaYhKkYEYiYOGqtTxN+PmyvNUBmFEcagBX2TXqEkgnI6zOnN13mB4SihJ2lne2r
 9RNiPYs/hmRxYg==
Message-ID: <2e1cdeb7-6940-8558-910a-9673b507bacb@2ndquadrant.com>
Date: Fri, 25 Jan 2019 15:26:55 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.2.1
MIME-Version: 1.0
In-Reply-To: <87a7k2996m.fsf@ars-thinkpad>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 14/01/2019 23:16, Arseny Sher wrote:
> 
> Nikhil Sontakke <nikhils@2ndquadrant.com> writes:
> 
>> I'd like to believe that the latest patch set tries to address some
>> (if not all) of your concerns. Can you please take a look and let me
>> know?
> 
> Hi, sure.
> 
> General things:
> 
> - Earlier I said that there is no point of sending COMMIT PREPARED if
>   decoding snapshot became consistent after PREPARE, i.e. PREPARE hadn't
>   been sent. I realized since then that such use cases actually exist:
>   prepare might be copied to the replica by e.g. basebackup or something
>   else earlier. 

Basebackup does not copy slots though and slot should not reach
consistency until all prepared transactions are committed no?

> 
> - BTW, ReorderBufferProcessXid at PREPARE should be always called
>   anyway, because otherwise if xact is empty, we will not prepare it
>   (and call cb), even if the output plugin asked us not to filter it
>   out. However, we will call commit_prepared cb, which is inconsistent.
> 
> - I find it weird that in DecodePrepare and in DecodeCommit you always
>   ask the plugin whether to filter an xact, given that sometimes you
>   know beforehand that you are not going to replay it: it might have
>   already been replayed, might have wrong dbid, origin, etc. One
>   consequence of this: imagine that notorious xact with PREPARE before
>   point where snapshot became consistent and COMMIT PREPARED after that
>   point. Even if filter_cb says 'I want 2PC on this xact', with current
>   code it won't be replayed on PREPARE and rbxid will be destroyed with
>   ReorderBufferForget. Now this xact is lost.

Yeah this is wrong.

> 
> Second patch:
> 
> +	/* filter_prepare is optional, but requires two-phase decoding */
> +	if ((ctx->callbacks.filter_prepare_cb != NULL) && (!opt->enable_twophase))
> +		ereport(ERROR,
> +				(errmsg("Output plugin does not support two-phase decoding, but "
> +						"registered filter_prepared callback.")));
> 
> I actually think that enable_twophase output plugin option is
> redundant. If plugin author wants 2PC, he just provides
> filter_prepare_cb callback and potentially others.

+1

> I also don't see much
> value in checking that exactly 0 or 3 callbacks were registred.
> 

I think that check makes sense, if you support 2pc you need to register
all callbacks.

> 
> Nitpicking:
> 
> First patch: I still don't think that these flags need a bitmask.

Since we are discussing this, I personally prefer the bitmask here.

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services