MIME-Version: 1.0
References: <CAA4eK1LGmZsevrqJra0V4O8oBU_eKyzm2VMpSAYQaDgC6n4fkA@mail.gmail.com>
 <CAFiTN-uhnJkrUsQByBHMK268T-GEx_D8DQ6b2T+aW6RiU75pbQ@mail.gmail.com>
 <OS0PR01MB571621ED532C2D7C3E01625894FB9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAFiTN-t5+_zTf86EzgCwObs1ED-P_5Ab1KMw340d3oPY_+Cwpw@mail.gmail.com>
 <OS3PR01MB5718AE486227CE3ACB844C5E94F89@OS3PR01MB5718.jpnprd01.prod.outlook.com>
 <CAFiTN-sgQ-K9BnMWKLR7Hm36QNjS7ZOBF6hsXJmQz5ChC0Desw@mail.gmail.com>
 <CAA4eK1+P9WOFBupaTZCzYtranSH=8TSUYV=kk5xQJ-VNKnvgmQ@mail.gmail.com>
 <OS0PR01MB571685A645E509267AC5F30894F99@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <OS0PR01MB57168A5A1C31BC316CB63D3C94F99@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1KK7Xty7SaCReY_b5moPURowMGOsfvhOb6xQ1EAaJAh6A@mail.gmail.com> <984ff689-adde-9977-affe-cd6029e850be@enterprisedb.com>
In-Reply-To: <984ff689-adde-9977-affe-cd6029e850be@enterprisedb.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Mon, 16 Jan 2023 11:49:36 +0530
Message-ID: <CAA4eK1K9+pchebQ47zstp-cmK0epML=UbWvs=enECHDB2vDPvQ@mail.gmail.com>
Subject: Re: Perform streaming logical transactions by background workers and
 parallel apply
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>, Masahiko Sawada <sawada.mshk@gmail.com>, 
	"wangw.fnst@fujitsu.com" <wangw.fnst@fujitsu.com>, Peter Smith <smithpb2250@gmail.com>, 
	"shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Dilip Kumar <dilipbalaut@gmail.com>
Content-Type: multipart/mixed; boundary="000000000000feda4905f25b91eb"
Archived-At: <https://www.postgresql.org/message-id/CAA4eK1K9%2BpchebQ47zstp-cmK0epML%3DUbWvs%3DenECHDB2vDPvQ%40mail.gmail.com>
Precedence: bulk

--000000000000feda4905f25b91eb
Content-Type: text/plain; charset="UTF-8"

On Sun, Jan 15, 2023 at 10:39 PM Tomas Vondra
<tomas.vondra@enterprisedb.com> wrote:
>
> I think there's a bug in how get_transaction_apply_action() interacts
> with handle_streamed_transaction() to decide whether the transaction is
> streamed or not. Originally, the code was simply:
>
>     /* not in streaming mode */
>     if (!in_streamed_transaction)
>         return false;
>
> But now this decision was moved to get_transaction_apply_action(), which
> does this:
>
>     if (am_parallel_apply_worker())
>     {
>         return TRANS_PARALLEL_APPLY;
>     }
>     else if (in_remote_transaction)
>     {
>         return TRANS_LEADER_APPLY;
>     }
>
> and handle_streamed_transaction() then uses the result like this:
>
>     /* not in streaming mode */
>     if (apply_action == TRANS_LEADER_APPLY)
>         return false;
>
> Notice this is not equal to the original behavior, because the two flags
> (in_remote_transaction and in_streamed_transaction) are not inverse.
> That is,
>
>    in_remote_transaction=false
>
> does not imply we're processing streamed transaction. It's allowed both
> flags are false, i.e. a change may be "non-transactional" and not
> streamed, though the only example of such thing in the protocol are
> logical messages. Which are however ignored in the apply worker, so I'm
> not surprised no existing test failed on this.
>

Right, this is the reason we didn't catch it in our testing.

> So I think get_transaction_apply_action() should do this:
>
>     if (am_parallel_apply_worker())
>     {
>         return TRANS_PARALLEL_APPLY;
>     }
>     else if (!in_streamed_transaction)
>     {
>         return TRANS_LEADER_APPLY;
>     }
>

Yeah, something like this would work but some of the callers other
than handle_streamed_transaction() also need to be changed. See
attached.

> FWIW I've noticed this after rebasing the sequence decoding patch, which
> adds another type of protocol message with the transactional vs.
> non-transactional behavior, similar to "logical messages" except that in
> this case the worker does not ignore that.
>
> Also, I think get_transaction_apply_action() would deserve better
> comments explaining how/why it makes the decisions.
>

Okay, I have added the comments in get_transaction_apply_action() and
updated the comments to refer to the enum TransApplyAction where all
the actions are explained.

-- 
With Regards,
Amit Kapila.

--000000000000feda4905f25b91eb
Content-Type: application/octet-stream; 
	name="v1-0001-Fix-the-code-to-decide-the-apply-action.patch"
Content-Disposition: attachment; 
	filename="v1-0001-Fix-the-code-to-decide-the-apply-action.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_lcyey6h90>
X-Attachment-Id: f_lcyey6h90

RnJvbSAxODQ5MTY2NjcyOWY5MDI4Y2RhYmE2MTFiZDkwODc0Zjc1YzJlMTMyIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBBbWl0IEthcGlsYSA8YWthcGlsYUBwb3N0Z3Jlc3FsLm9yZz4K
RGF0ZTogTW9uLCAxNiBKYW4gMjAyMyAxMDo0OTozOCArMDUzMApTdWJqZWN0OiBbUEFUQ0ggdjFd
IEZpeCB0aGUgY29kZSB0byBkZWNpZGUgdGhlIGFwcGx5IGFjdGlvbi4KClRoZSBjb2RlIG1pc3Nl
ZCB0byBoYW5kbGUgbm9uLXRyYW5zYWN0aW9uYWwgbWVzc2FnZXMgYW5kIHdlIGRpZG4ndCBjYXRj
aAppdCBpbiBvdXIgdGVzdGluZyBhcyBjdXJyZW50bHkgc3VjaCBtZXNzYWdlcyBhcmUgc2ltcGx5
IGlnbm9yZWQgYnkgdGhlCmFwcGx5IHdvcmtlci4gVGhpcyB3YXMgaW50cm9kdWNlZCBieSBjaGFu
Z2VzIGluIGNvbW1pdCAyMTZhNzg0ODI5LgoKV2hpbGUgdGVzdGluZyB0aGlzLCBJIG5vdGljZWQg
dGhhdCB3ZSBmb3Jnb3QgdG8gcmVzZXQgc3RyZWFtX3hpZCBhZnRlcgpwcm9jZXNzaW5nIHN0cmVh
bSBzdG9wIG1lc3NhZ2Ugd2hpY2ggY291bGQgYWxzbyByZXN1bHQgaW4gdGhlIHdyb25nIGFwcGx5
CmFjdGlvbiBhZnRlciB0aGUgZml4IGZvciBub24tdHJhbnNhY3Rpb25hbCBtZXNzYWdlcy4KClJl
cG9ydGVkLWJ5OiBUb21hcyBWb25kcmEKRGlzY3Vzc2lvbjogaHR0cHM6Ly9wb3N0Z3IuZXMvbS85
ODRmZjY4OS1hZGRlLTk5NzctYWZmZS1jZDYwMjllODUwYmVAZW50ZXJwcmlzZWRiLmNvbQotLS0K
IHNyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL2xvZ2ljYWwvd29ya2VyLmMgfCA1MyArKysrKysrKysr
KysrKysrKysrKy0tLS0tLS0tLS0tLQogMSBmaWxlIGNoYW5nZWQsIDMzIGluc2VydGlvbnMoKyks
IDIwIGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL2xv
Z2ljYWwvd29ya2VyLmMgYi9zcmMvYmFja2VuZC9yZXBsaWNhdGlvbi9sb2dpY2FsL3dvcmtlci5j
CmluZGV4IDc5Y2RhMzkuLjdkMWIxYTggMTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL3JlcGxpY2F0
aW9uL2xvZ2ljYWwvd29ya2VyLmMKKysrIGIvc3JjL2JhY2tlbmQvcmVwbGljYXRpb24vbG9naWNh
bC93b3JrZXIuYwpAQCAtMjQ3LDggKzI0NywxMCBAQCB0eXBlZGVmIHN0cnVjdCBBcHBseUVycm9y
Q2FsbGJhY2tBcmcKICAqIFRoZSBhY3Rpb24gdG8gYmUgdGFrZW4gZm9yIHRoZSBjaGFuZ2VzIGlu
IHRoZSB0cmFuc2FjdGlvbi4KICAqCiAgKiBUUkFOU19MRUFERVJfQVBQTFk6Ci0gKiBUaGlzIGFj
dGlvbiBtZWFucyB0aGF0IHdlIGFyZSBpbiB0aGUgbGVhZGVyIGFwcGx5IHdvcmtlciBhbmQgY2hh
bmdlcyBvZiB0aGUKLSAqIHRyYW5zYWN0aW9uIGFyZSBhcHBsaWVkIGRpcmVjdGx5IGJ5IHRoZSB3
b3JrZXIuCisgKiBUaGlzIGFjdGlvbiBtZWFucyB0aGF0IHdlIGFyZSBpbiB0aGUgbGVhZGVyIGFw
cGx5IHdvcmtlciBvciB0YWJsZSBzeW5jCisgKiB3b3JrZXIuIFRoZSBjaGFuZ2VzIG9mIHRoZSB0
cmFuc2FjdGlvbiBhcmUgZWl0aGVyIGRpcmVjdGx5IGFwcGxpZWQgb3IKKyAqIGFyZSByZWFkIGZy
b20gdGVtcG9yYXJ5IGZpbGVzIChmb3Igc3RyZWFtaW5nIHRyYW5zYWN0aW9ucykgYW5kIHRoZW4K
KyAqIGFwcGxpZWQgYnkgdGhlIHdvcmtlci4KICAqCiAgKiBUUkFOU19MRUFERVJfU0VSSUFMSVpF
OgogICogVGhpcyBhY3Rpb24gbWVhbnMgdGhhdCB3ZSBhcmUgaW4gdGhlIGxlYWRlciBhcHBseSB3
b3JrZXIgb3IgdGFibGUgc3luYwpAQCAtMTMwMSw3ICsxMzAzLDcgQEAgYXBwbHlfaGFuZGxlX3N0
cmVhbV9wcmVwYXJlKFN0cmluZ0luZm8gcykKIAogCXN3aXRjaCAoYXBwbHlfYWN0aW9uKQogCXsK
LQkJY2FzZSBUUkFOU19MRUFERVJfU0VSSUFMSVpFOgorCQljYXNlIFRSQU5TX0xFQURFUl9BUFBM
WToKIAogCQkJLyoKIAkJCSAqIFRoZSB0cmFuc2FjdGlvbiBoYXMgYmVlbiBzZXJpYWxpemVkIHRv
IGZpbGUsIHNvIHJlcGxheSBhbGwgdGhlCkBAIC0xNzEwLDYgKzE3MTIsNyBAQCBhcHBseV9oYW5k
bGVfc3RyZWFtX3N0b3AoU3RyaW5nSW5mbyBzKQogCX0KIAogCWluX3N0cmVhbWVkX3RyYW5zYWN0
aW9uID0gZmFsc2U7CisJc3RyZWFtX3hpZCA9IEludmFsaWRUcmFuc2FjdGlvbklkOwogCiAJLyoK
IAkgKiBUaGUgcGFyYWxsZWwgYXBwbHkgd29ya2VyIGNvdWxkIGJlIGluIGEgdHJhbnNhY3Rpb24g
aW4gd2hpY2ggY2FzZSB3ZQpAQCAtMTg0Miw3ICsxODQ1LDcgQEAgYXBwbHlfaGFuZGxlX3N0cmVh
bV9hYm9ydChTdHJpbmdJbmZvIHMpCiAKIAlzd2l0Y2ggKGFwcGx5X2FjdGlvbikKIAl7Ci0JCWNh
c2UgVFJBTlNfTEVBREVSX1NFUklBTElaRToKKwkJY2FzZSBUUkFOU19MRUFERVJfQVBQTFk6CiAK
IAkJCS8qCiAJCQkgKiBXZSBhcmUgaW4gdGhlIGxlYWRlciBhcHBseSB3b3JrZXIgYW5kIHRoZSB0
cmFuc2FjdGlvbiBoYXMgYmVlbgpAQCAtMjE2NCw3ICsyMTY3LDcgQEAgYXBwbHlfaGFuZGxlX3N0
cmVhbV9jb21taXQoU3RyaW5nSW5mbyBzKQogCiAJc3dpdGNoIChhcHBseV9hY3Rpb24pCiAJewot
CQljYXNlIFRSQU5TX0xFQURFUl9TRVJJQUxJWkU6CisJCWNhc2UgVFJBTlNfTEVBREVSX0FQUExZ
OgogCiAJCQkvKgogCQkJICogVGhlIHRyYW5zYWN0aW9uIGhhcyBiZWVuIHNlcmlhbGl6ZWQgdG8g
ZmlsZSwgc28gcmVwbGF5IGFsbCB0aGUKQEAgLTQ5OTYsMTAgKzQ5OTksMTIgQEAgc2V0X2FwcGx5
X2Vycm9yX2NvbnRleHRfb3JpZ2luKGNoYXIgKm9yaWdpbm5hbWUpCiB9CiAKIC8qCi0gKiBSZXR1
cm4gdGhlIGFjdGlvbiB0byBiZSB0YWtlbiBmb3IgdGhlIGdpdmVuIHRyYW5zYWN0aW9uLiAqd2lu
Zm8gaXMKLSAqIGFzc2lnbmVkIHRvIHRoZSBkZXN0aW5hdGlvbiBwYXJhbGxlbCB3b3JrZXIgaW5m
byB3aGVuIHRoZSBsZWFkZXIgYXBwbHkKLSAqIHdvcmtlciBoYXMgdG8gcGFzcyBhbGwgdGhlIHRy
YW5zYWN0aW9uJ3MgY2hhbmdlcyB0byB0aGUgcGFyYWxsZWwgYXBwbHkKLSAqIHdvcmtlci4KKyAq
IFJldHVybiB0aGUgYWN0aW9uIHRvIGJlIHRha2VuIGZvciB0aGUgZ2l2ZW4gdHJhbnNhY3Rpb24u
IFNlZQorICogVHJhbnNBcHBseUFjdGlvbiBmb3IgaW5mb3JtYXRpb24gb24gZWFjaCBvZiB0aGUg
YWN0aW9ucy4KKyAqCisgKiAqd2luZm8gaXMgYXNzaWduZWQgdG8gdGhlIGRlc3RpbmF0aW9uIHBh
cmFsbGVsIHdvcmtlciBpbmZvIHdoZW4gdGhlIGxlYWRlcgorICogYXBwbHkgd29ya2VyIGhhcyB0
byBwYXNzIGFsbCB0aGUgdHJhbnNhY3Rpb24ncyBjaGFuZ2VzIHRvIHRoZSBwYXJhbGxlbAorICog
YXBwbHkgd29ya2VyLgogICovCiBzdGF0aWMgVHJhbnNBcHBseUFjdGlvbgogZ2V0X3RyYW5zYWN0
aW9uX2FwcGx5X2FjdGlvbihUcmFuc2FjdGlvbklkIHhpZCwgUGFyYWxsZWxBcHBseVdvcmtlcklu
Zm8gKip3aW5mbykKQEAgLTUwMTAsMjcgKzUwMTUsMzUgQEAgZ2V0X3RyYW5zYWN0aW9uX2FwcGx5
X2FjdGlvbihUcmFuc2FjdGlvbklkIHhpZCwgUGFyYWxsZWxBcHBseVdvcmtlckluZm8gKip3aW5m
bykKIAl7CiAJCXJldHVybiBUUkFOU19QQVJBTExFTF9BUFBMWTsKIAl9Ci0JZWxzZSBpZiAoaW5f
cmVtb3RlX3RyYW5zYWN0aW9uKQotCXsKLQkJcmV0dXJuIFRSQU5TX0xFQURFUl9BUFBMWTsKLQl9
CiAKIAkvKgotCSAqIENoZWNrIGlmIHdlIGFyZSBwcm9jZXNzaW5nIHRoaXMgdHJhbnNhY3Rpb24g
dXNpbmcgYSBwYXJhbGxlbCBhcHBseQotCSAqIHdvcmtlci4KKwkgKiBJZiB3ZSBhcmUgcHJvY2Vz
c2luZyB0aGlzIHRyYW5zYWN0aW9uIHVzaW5nIGEgcGFyYWxsZWwgYXBwbHkgd29ya2VyIHRoZW4K
KwkgKiBlaXRoZXIgd2Ugc2VuZCB0aGUgY2hhbmdlcyB0byB0aGUgcGFyYWxsZWwgd29ya2VyIG9y
IGlmIHRoZSB3b3JrZXIgaXMgYnVzeQorCSAqIHRoZW4gc2VyaWFsaXplIHRoZSBjaGFuZ2VzIHRv
IHRoZSBmaWxlIHdoaWNoIHdpbGwgbGF0ZXIgYmUgcHJvY2Vzc2VkIGJ5CisJICogdGhlIHBhcmFs
bGVsIHdvcmtlci4KIAkgKi8KIAkqd2luZm8gPSBwYV9maW5kX3dvcmtlcih4aWQpOwogCi0JaWYg
KCEqd2luZm8pCisJaWYgKCp3aW5mbyAmJiAoKndpbmZvKS0+c2VyaWFsaXplX2NoYW5nZXMpCiAJ
ewotCQlyZXR1cm4gVFJBTlNfTEVBREVSX1NFUklBTElaRTsKKwkJcmV0dXJuIFRSQU5TX0xFQURF
Ul9QQVJUSUFMX1NFUklBTElaRTsKIAl9Ci0JZWxzZSBpZiAoKCp3aW5mbyktPnNlcmlhbGl6ZV9j
aGFuZ2VzKQorCWVsc2UgaWYgKCp3aW5mbykKIAl7Ci0JCXJldHVybiBUUkFOU19MRUFERVJfUEFS
VElBTF9TRVJJQUxJWkU7CisJCXJldHVybiBUUkFOU19MRUFERVJfU0VORF9UT19QQVJBTExFTDsK
Kwl9CisKKwkvKgorCSAqIElmIHRoZXJlIGlzIG5vIHBhcmFsbGVsIHdvcmtlciBpbnZvbHZlZCB0
byBwcm9jZXNzIHRoaXMgdHJhbnNhY3Rpb24gdGhlbgorCSAqIHdlIGVpdGhlciBkaXJlY3RseSBh
cHBseSB0aGUgY2hhbmdlIG9yIHNlcmlhbGl6ZSBpdCB0byBhIGZpbGUgd2hpY2ggd2lsbAorCSAq
IGxhdGVyIGJlIGFwcGxpZWQgd2hlbiB0aGUgdHJhbnNhY3Rpb24gZmluaXNoIG1lc3NhZ2UgaXMg
cHJvY2Vzc2VkLgorCSAqLworCWVsc2UgaWYgKGluX3N0cmVhbWVkX3RyYW5zYWN0aW9uKQorCXsK
KwkJcmV0dXJuIFRSQU5TX0xFQURFUl9TRVJJQUxJWkU7CiAJfQogCWVsc2UKIAl7Ci0JCXJldHVy
biBUUkFOU19MRUFERVJfU0VORF9UT19QQVJBTExFTDsKKwkJcmV0dXJuIFRSQU5TX0xFQURFUl9B
UFBMWTsKIAl9CiB9Ci0tIAoxLjguMy4xCgo=
--000000000000feda4905f25b91eb--