Subject: Re: [HACKERS] logical decoding of two-phase transactions
To: Nikhil Sontakke <nikhils@2ndquadrant.com>,
 Andres Freund <andres@anarazel.de>
Cc: Simon Riggs <simon@2ndquadrant.com>, Craig Ringer
 <craig@2ndquadrant.com>, Peter Eisentraut
 <peter.eisentraut@2ndquadrant.com>,
 Petr Jelinek <petr.jelinek@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: 
 <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>
 <0925358a-e11d-51bc-c3c1-959ded75f604@2ndquadrant.com>
 <20180329222441.xxo2zxkjydpgjs4n@alap3.anarazel.de>
 <CANP8+jLZy6Pxqp2Vxo20OuR5v_B1hEywR6Hp7DHwNn6Yi3B-Dw@mail.gmail.com>
 <20180402180451.ip3uuvadhgxjvefo@alap3.anarazel.de>
 <CAMGcDxchx=0PeQBVLzrgYG2AQ49QSRxHj5DCp7yy0QrJR0S0nA@mail.gmail.com>
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Openpgp: preference=signencrypt
Autocrypt: addr=tomas.vondra@2ndquadrant.com; prefer-encrypt=mutual; keydata=
 xsFNBFSsTToBEADfyqVlb3XsQt4zllL1UTXFr4VVbyuZYggxL9UpMekIUG84wEjrzf+RJVWp
 WAjrZ6kHt4zomKdylow8/h0ympc7RzMkWtRE+0RGiN3ssfxXB+qqaINEgL71g/bGELq4S15B
 kZi+H6sEGrVQ2j3yuycWtNRsrzFiUZL92M/rfBduDfZwA42AymDzt9EsErbZvmck3DS7Sjm1
 kcsyNRrFeN7hCKrGRRRW5CEWX/45ESd4mTh/jcIXPfS0sWvqRwcaBOHimufW7T5xdhNy83Ed
 YJxyMcEHPg8FlPSi3PII9vc3ZqQcFv+3/ChxoITjGgmS72jQ1uuKgvWFeYjLxXcyRN552QV1
 dK7xclRsvLlKeR3ub9YU1KP4PbZMX+Fd6aZIxwMW25la2wGiyTb2okjsldz9xpGAyUccj0ZN
 1w/XX03heRRg2heG3QGA0z0Mo5UuOn8l340zNRKycRGso7bczdyUYm//sLiEwDJMNi2XF65J
 BDCikt0uGNxZmnjxoi6ECAkATeG5q4SK4uTX7hUomn6a6IBpvoaceNjJ6MNE8uzXuWSYpntf
 T/e3sF+SIwfzkVdc4ASD7a796sddyUMhr5e/KrG2RMERhMZ9OOkJXWOBFBr+lek1wzNvBGtL
 aT2S1oEz77eRPxDm8yh46b1d5tEkcXBw7tdNjEhtLOIjBvJj8wARAQABzStUb21hcyBWb25k
 cmEgPHRvbWFzLnZvbmRyYUAybmRxdWFkcmFudC5jb20+wsF9BBMBCgAnAhsDBQsJCAcDBRUK
 CQgLBRYCAwEAAh4BAheABQJavO66BQkH8dUAAAoJEHr8T34Q28YxgtsP/3P2IpqIQ+NJ5NwX
 YU7VTtutZOkUFCDmkuaIR1f9aAUOVXPO225sSB51m686Ro5JFnJVtty9o3eBT3TJwDsf8RKX
 WoUpye1MToGuRURMCHwyD7DLh+vWz7oMRQRVVbO87Quh5cpz1GGFZ1rh9MmpHJlP6/TzT2gH
 w8KlOLmIiitvv5nR6uK1lYPQX3wg5HSq0uDEReyEOsDKe7JWqBi2512savPfBJ7SR4917H3n
 6vQHId2s4dgaYzlC09csqvRKEB5JFbQ6cGGQ2vLjhlWmii+njXCT3Aiq0GKHYIpl/wiT4PhZ
 TMd9H8ZitMVAQEJF8BrBLAKth9Ydqs8Yt2SS/FX8Z+NNIIS1PzIzB+c14GilHnP9qgBDeMSe
 w5gQ51ejiHTt4uqziWEqFEM+2tdDtm53e0BgfM0sAnhtToKpZ588otD8VJ6tapdNGt/JxZN6
 9xNFE66BeAEVDwY25L1k7Sd8RDfeTlS9+GvlGmtLACrm41FjDkg+xaWs2MPXXOK0cZFwtFJw
 6lZohAswSyfeXtvrsfNFWV3ww9DUsS46IqRkU4pZWonPR+MTxL98RwzhVNO84lcDbXM3WYsZ
 kmy8pvqdh6pbBHIRV3VmH1D6i9qovttLZWKZeD0FBIIPtUl7hOKyNG+WJXpiFRquJIXW9MSa
 vwLQd+FGy9PxXfvyAmsJzsFNBFSsTToBEADImGvEViP6kfZktPad6AddDNiSkI0hXnx+EijS
 84aAcUdZ0ry16cq4ywH+0aAxcP8JgmYXdk1o1/c4wWuL5GOLvES+3/xuZjy0i6W3akJus0Ne
 q0kvO8hYP/+omXvjdmF3Wppd+QYYjn1OvH7dzlJWCVoBuvkctpKKDFi79RqAjfGv5CQ3PB0M
 +w2++3n87nFUt5mW21xsYBOZ1wb5WPzVMSEW2ocGf206kX5zQzWgQksig6Hha9IHC9lcDqIC
 oJcXhX9mPXfTv3LWFu6+zn1e80Q16p9xQMpGw64JzeCvEyuwBQwdBQvlsmEh/0bXqlr9qF81
 4abNn2HAv/abOWOIxb8dgAoariJb9/LnReYndpkhKvPUrWx/GXvZ+iRAH4reaNe55AXTn3vR
 /dyl2hINFfKzUQ7HiniLjZQ0+I99EhaitB7fXJrkoJDyovs7xTDJV4oa2HBYT40F5wTRQuJ3
 2fs5qHH7GyRYASTC9UpkxYhfJJ8RHx/aJCqxx9kaGahk/IVQcBA1QM6x1T4j/Kt45wtqI61e
 u6fJVIo4+Ety91XMggw28vZWKnsKuamNA0Umt5/r5czlvdJqJaMgdZlURLQfzX6LijqRilUM
 oRNtegOCsdRp2g9t2imHsbla5CVXHUeBajfy32PQCx1AJhGROv9YdfFv86S++kdUZfktHwAR
 AQABwsFlBBgBCgAPBQJUrE06AhsMBQkDwmcAAAoJEHr8T34Q28YxoUIP/3m7w4/OKYIHlCUF
 eqOr9QJ9dLZu45yJZ+OHGGjjSsFEomCcch1O1AyGOGAalCy11eP9d8/pq42DZ4U884LZRDJi
 AsOC1v7aIbTsHTgNfVZEOfP61Irq6ExHM63yE5SUVM2pGV8BjkDTC+1+Bwnhc6fIOG/V1oEP
 gDuesd9TnkSYbBXnsFD1XKmmPeQZFbH4FF6bHQnDpBUsHuD1+CpbsbqLHsLrGmsCGKJB2Bfh
 0GfuhX7pv6/195YRUNNWdiQEwcvVHn/SaEWholx2ZDIWNDHo6ki+Y/KUYuKJ+iAlWYyj/pU6
 dUX56T0QvQlLN9LSAtaDeDltGm7f7JpfrLGqbG4FKC1xJt1crMDonOm8BCgCQjVu5/JV6v+y
 HsGbydn53gBzz3Kg2Ig4MkO36jU4lhp6rg7tAc1SrrKfwyns5Q3k1hFAp3yaa0epQySG/nZF
 snXvTYvZ/wAS4S8JK9rclxR7OOmwx3S+6SBptGsN0KY63B8wOqNohAEMitWqrp/aRcKfY6Hm
 7bAT5VA0Sl1F2Xwf6ISalG+7iVcc6n9NXByO4QO60KbIHC2131rdpnDwOpGEcRKeUw1h3mvH
 S072tMtixCvFuiJ22JzAXdLXa1C0gZiZY/usDcY+oJsLIiQyFOl4SU8ACW43dBMdAjZ2Wamg
 aHGJaAIX4NfWQQwk5FoEwsFlBBgBCgAPAhsMBQJavO7BBQkH8dUHAAoJEHr8T34Q28Yxj6EQ
 AIXEEsxvnoc6KkE0pMhumwjaOhqk72pAE8dQx0j69NhvlncOynvHSSV41IhjmDvi1i5xMQ/z
 JNMzjHlzyn4B4xNdv9HNnRl649chWDgrE23r+nGdsF7jbvZ7/75TMKDcypc5B4kwbTsgNr/4
 DrhC3Cgp9RSb1bLOrKrArhkStRZiVDplA3b4cv9BhHTV2yz791KXI2u7m0JP+kWBInsG8LTf
 WTEwHsscZQn4fOpPupz5B79z5ByClILAD4pc51pDqg0jijEXctiYjFsd/nX/ohTNnku0FopX
 EowzYEsckvyAEItUzIOqtSuM9Y1H/vz90tHM0DzfUl7lnPHo4SQ8MJ9Pjr6SGHtFSUs/aGv+
 7YXUrEUzzFBUjYDhpaX/tRbm7i9FP27w0XD6kyAmUoBMKmajxLlnGW73xeXgtB0NHne3hi5Q
 95AhcGRNqz+AFeDOLgOKjhya1JX9790Sv0uMElNDZaqzwHXuemSlZuvxnvqi/eFqr8N8RN0D
 ly2MMqQ0hzsPSgYHIFBibBW3bmrVfIQXfcFckrfoeQaHeVPgrNMh5A6NdIK2ONQUDvq84FHi
 6jMMjGopc4PsbrO5q0/G7/FdX1whaLVx94St3XJ5s6ZGu9CRZDDY/k9hDbimBHHVcChnwjay
 vYQVbtJ4LvfohZZKzzu7kxGBXFPWgBby2mlK
Message-ID: <782517ca-1e99-06fe-eba9-34f826118289@2ndquadrant.com>
Date: Tue, 3 Apr 2018 16:59:27 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: 
 <CAMGcDxchx=0PeQBVLzrgYG2AQ49QSRxHj5DCp7yy0QrJR0S0nA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

FWIW, a couple of additional comments based on eyeballing the diffs:

1) twophase.c
---------

I think this comment is slightly inaccurate:

 /*
 * Coordinate with logical decoding backends that may be already
 * decoding this prepared transaction. When aborting a transaction,
 * we need to wait for all of them to leave the decoding group. If
 * committing, we simply remove all members from the group.
 */

Strictly speaking, we're not waiting for the workers to leave the
decoding group, but to set decodeLocked=false. That is, we may proceed
when there still are members, but they must be in unlocked state.


2) reorderbuffer.c
------------------

I've already said it before, I find the "flags" bitmask and rbtxn_*
macros way less readable than individual boolean flags. It was claimed
this was done on Andres' request, but I don't see that in the thread. I
admit it's rather subjective, though.

I see ReorederBuffer only does the lock/unlock around apply_change and
RelationIdGetRelation. That seems insufficient - RelidByRelfilenode can
do heap_open on pg_class, for example. And I guess we need to protect
rb->message too, because who knows what the plugin does in the callback?

Also, we should not allocate gid[GIDSIZE] for every transaction. For
example subxacts never need it, and it seems rather wasteful to allocate
200B when the rest of the struct has only ~100B. This is particularly
problematic considering ReorderBufferTXN is not spilled to disk when
reaching the memory limit. It needs to be allocated ad-hoc only when
actually needed.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services