From: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
---|---|
To: | Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com> |
Cc: | "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, "wangw(dot)fnst(at)fujitsu(dot)com" <wangw(dot)fnst(at)fujitsu(dot)com>, Peter Smith <smithpb2250(at)gmail(dot)com>, "shiy(dot)fnst(at)fujitsu(dot)com" <shiy(dot)fnst(at)fujitsu(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Dilip Kumar <dilipbalaut(at)gmail(dot)com> |
Subject: | Re: Perform streaming logical transactions by background workers and parallel apply |
Date: | 2023-01-16 06:19:36 |
Message-ID: | CAA4eK1K9+pchebQ47zstp-cmK0epML=UbWvs=enECHDB2vDPvQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Sun, Jan 15, 2023 at 10:39 PM Tomas Vondra
<tomas(dot)vondra(at)enterprisedb(dot)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.
Attachment | Content-Type | Size |
---|---|---|
v1-0001-Fix-the-code-to-decide-the-apply-action.patch | application/octet-stream | 4.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | 金 | 2023-01-16 06:21:26 | If there are more than two functions in different schemas, the functions have the same name and same arguments, \df[+] only display the function that schema first appeared in the search_path. |
Previous Message | Pavel Stehule | 2023-01-16 06:09:51 | Re: On login trigger: take three |