From: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
---|---|
To: | "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com> |
Cc: | Dilip Kumar <dilipbalaut(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, "shiy(dot)fnst(at)fujitsu(dot)com" <shiy(dot)fnst(at)fujitsu(dot)com> |
Subject: | Re: Avoid streaming the transaction which are skipped (in corner cases) |
Date: | 2022-12-03 11:37:20 |
Message-ID: | CAA4eK1+aqRRSk2aJ9-34rmJq5KF4GHNgvh3UP3E17c9Gy510pQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Nov 29, 2022 at 12:23 PM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Tuesday, November 29, 2022 12:08 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> I have few comments about the patch.
>
> 1.
>
> 1.1.
> - /* For streamed transactions notify the remote node about the abort. */
> - if (rbtxn_is_streamed(txn))
> - rb->stream_abort(rb, txn, lsn);
> + /* the transaction which is being skipped shouldn't have been streamed */
> + Assert(!rbtxn_is_streamed(txn));
>
> 1.2
> - rbtxn_is_serialized(txn))
> + rbtxn_is_serialized(txn) &&
> + rbtxn_has_streamable_change(txn))
> ReorderBufferStreamTXN(rb, toptxn);
>
> In the above two places, I think we should do the check for the top-level
> transaction(e.g. toptxn) because the patch only set flag for the top-level
> transaction.
>
Among these, the first one seems okay because it will check both the
transaction and its subtransactions from that path and none of those
should be marked as streamed. I have fixed the second one in the
attached patch.
> 2.
>
> + /*
> + * If there are any streamable changes getting queued then get the top
> + * transaction and mark it has streamable change. This is required for
> + * streaming in-progress transactions, the in-progress transaction will
> + * not be selected for streaming unless it has at least one streamable
> + * change.
> + */
> + if (change->action == REORDER_BUFFER_CHANGE_INSERT ||
> + change->action == REORDER_BUFFER_CHANGE_UPDATE ||
> + change->action == REORDER_BUFFER_CHANGE_DELETE ||
> + change->action == REORDER_BUFFER_CHANGE_INTERNAL_SPEC_INSERT ||
> + change->action == REORDER_BUFFER_CHANGE_TRUNCATE)
>
> I think that a transaction that contains REORDER_BUFFER_CHANGE_MESSAGE can also be
> considered as streamable. Is there a reason that we don't check it here ?
>
No, I don't see any reason not to do this check for
REORDER_BUFFER_CHANGE_MESSAGE.
Apart from the above, I have slightly adjusted the comments in the
attached. Do let me know what you think of the attached.
--
With Regards,
Amit Kapila.
Attachment | Content-Type | Size |
---|---|---|
v4-0001-Avoid-unnecessary-streaming-of-transactions-durin.patch | application/octet-stream | 6.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2022-12-03 14:33:35 | Re: Traversing targetlist to find accessed columns |
Previous Message | Drouvot, Bertrand | 2022-12-03 09:31:19 | Re: Generate pg_stat_get_* functions with Macros |