Subject: Re: [HACKERS] logical decoding of two-phase transactions
To: Nikhil Sontakke <nikhils@2ndquadrant.com>
Cc: Andres Freund <andres@anarazel.de>,
 Tomas Vondra <tomas.vondra@2ndquadrant.com>,
 Craig Ringer <craig@2ndquadrant.com>,
 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
 Simon Riggs <simon@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 Hackers <pgsql-hackers@postgresql.org>,
 Robert Haas <robertmhaas@gmail.com>
References: 
 <CAMGcDxc9p7envO8t+29j=NdQXHoba6P1btfU0d3Xeiz7zT-Mvw@mail.gmail.com>
 <20180212162040.rsnlnysmf7n4yo7i@alap3.anarazel.de>
 <CAMGcDxeViP+R-OL7QhzUV9eKCVjURobuY1Zijik4Ay_Ddwo4Cg@mail.gmail.com>
 <20180302005343.zv6nsqimcujzjrcd@alap3.anarazel.de>
 <CAMsr+YEb-f1oG+1wGji_sgswRsVQYtVXuTbVH=nnY6thMfiCqA@mail.gmail.com>
 <CAMGcDxcD2fWqUwiPWGL_d-ie-CoGTVFAsgQvQ91H93q2Jc-mhA@mail.gmail.com>
 <CAMGcDxf9HHmDOti4-DSQTtGo48R4OMki+BktPn2uTOtGr4XomQ@mail.gmail.com>
 <c167340c-0751-d54b-0f32-fa83fd414863@2ndquadrant.com>
 <CAMGcDxcGtRXtF1r3VqddzHba+BTm7q8wbgEsyhmOFuDckMTZFQ@mail.gmail.com>
 <dd5a9cb7-bb10-cb5c-834e-08dc3ffc057f@2ndquadrant.com>
 <20180329215815.36xxakgujx4luo4m@alap3.anarazel.de>
 <ff89e6f0-baae-fd4f-6565-13e7c2f23b90@2ndquadrant.com>
 <CAMGcDxdboSU8cyNoCav7Qpa_U5FVqhTnb+cbnDa6=kL=3KWoXg@mail.gmail.com>
From: Petr Jelinek <petr.jelinek@2ndquadrant.com>
Openpgp: preference=signencrypt
Autocrypt: addr=petr.jelinek@2ndquadrant.com; keydata=
 xsFNBFawwSABEAC45Vua8rSggpM2kkMMmoOHz51a+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+L21ORB82QYkzm3M7ebXQARAQABzSNQZXRyIEplbMOt
 bmVrIDxwam1vZG9zQHBqbW9kb3MubmV0PsLBgAQTAQoAKgIbAwUJBaOagAULCQgHAwUVCgkI
 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
 HDaXLK3BPNOdH87BTQRWsMEgARAA6Z3NrDLdcFF/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/SMAEQEAAcLB
 ZQQYAQoADwUCVrDBIAIbDAUJBaOagAAKCRBci2l0giH4yyWpD/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: <f92d74fe-0e55-4c9b-a342-4c134591f1be@2ndquadrant.com>
Date: Fri, 30 Mar 2018 19:27:18 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.6.0
MIME-Version: 1.0
In-Reply-To: 
 <CAMGcDxdboSU8cyNoCav7Qpa_U5FVqhTnb+cbnDa6=kL=3KWoXg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 30/03/18 09:56, Nikhil Sontakke wrote:
> 
>> I as wondering how to hide this. Best idea I had so far would be to put
>> it in heap_beginscan (and index_beginscan given that catalog scans use
>> is as well) behind some condition. That would also improve performance
>> because locking would not need to happen for syscache hits. The problem
>> is however how to inform the heap_beginscan about the fact that we are
>> in 2PC decoding. We definitely don't want to change all the scan apis
>> for this. I wonder if we could add some kind of property to Snapshot
>> which would indicate this fact - logical decoding is using it's own
>> snapshots it could inject the information about being inside the 2PC
>> decoding.
>>
> 
> The idea of adding that info in the Snapshot itself is interesting. We
> could introduce a logicalxid field in SnapshotData to point to the XID
> that the decoding backend is interested in. This could be added only
> for the 2PC case. Support in the future for in-progress transactions
> could use this field as well. If it's a valid XID, we could call
> LogicalLockTransaction/LogicalUnlockTransaction on that XID from
> heap_beginscan/head_endscan respectively. I can also look at what
> other *_beginscan APIs would need this as well.
> 

So I have spent some significant time today thinking about this (the
issue in general not this specific idea). And I think this proposal does
not work either.

The problem is that we fundamentally want two things, not one. It's true
we want to block ABORT from finishing while we are reading catalogs, but
the other important part is that we want to bail gracefully when ABORT
happened for the transaction being decoded.

In other words,, if we do the locking transparently somewhere in the
scan or catalog read or similar there is no way to let the plugin know
that it should bail. So the locking code that's called from several
layers deep would have only one option, to ERROR. I don't think we want
to throw ERRORs when transaction which is being decoded has been aborted
as that disrupts the replication.

I think that we basically only have two options here that can satisfy
both blocking ABORT and bailing gracefully in case ABORT has happened.
Either the plugin has full control over locking (as in the patch), so
that it can bail when the locking function reports that the transaction
has aborted. Or we do the locking around the plugin calls, ie directly
in logical decoding callback wrappers or similar.

Both of these options have some disadvantages. Locking inside plugin
make the plugin code much more complex if it wants to support this. For
example if I as plugin author call any function that somewhere access
syscache, I have to do the locking around that function call. Locking
around plugin callbacks can hold he lock for longer periods of time
since plugins usually end up writing to network. I think for most
use-cases of 2PC decoding the latter is more useful as plugin should be
connected to some kind transaction management solution. Also the time
should be bounded by things like wal_sender_timeout (or
statement_timeout for SQL variant of decoding).

Note that I was initially advocating against locking around whole
callbacks when Nikhil originally came up with the idea, but after we
went over several other options here and given it a lot of thought I now
think it's probably least bad way we have available. At least until
somebody figures out how to solve all the issues around reading aborted
catalog changes, but that does seem like rather large project on its
own. And if we do locking around plugin callbacks now then we can easily
switch to that solution if it ever happens without anybody having to
rewrite the plugins.

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