MIME-Version: 1.0
References: <CAA4eK1KdVRffYEYiv9=4tVWhP=Jkfh1h6FM0A1R0NEe8cLT8+g@mail.gmail.com>
 <CAA4eK1JU4J3-Z=fcMcyQw-iG-DNr=u6w_g-iM_4xwg1pKupkFQ@mail.gmail.com>
 <OS0PR01MB57163693482C5470C41914AF94EA9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAHut+PuYJz-26QWMSRN_HwiJNfYG4ZxEV3RGE-VyZtXXeFm+cw@mail.gmail.com>
 <OS0PR01MB5716C1C1D8244BA86311193894EB9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1LrADDNXKRVYox4Q+zSFQvtiQu-7piyd_PCmVjg1gsN2w@mail.gmail.com>
 <OS0PR01MB57169EF567017861EACBD10D94E99@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <OS0PR01MB571623EFD8903C93424EFADC94E99@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <OS0PR01MB57166C4E79559C9A5570838C94EC9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1+NV9BDPjh5XB9PnZYNdnQht8+2JGRcerJpSUXtYPtQsg@mail.gmail.com>
 <OS3PR01MB6275B61076717E4CE9E079D19EED9@OS3PR01MB6275.jpnprd01.prod.outlook.com>
 <CAA4eK1LXch0EDB9RCDiqqmM3S9_cam=svMMK08i8Fmspms=6Xg@mail.gmail.com>
 <OS3PR01MB62756CC6FFBDD431F2878ACE9EF29@OS3PR01MB6275.jpnprd01.prod.outlook.com>
 <CAA4eK1Konv5gm2bZhej_CZqotr=qhUHnhhiJixKLRmCMsg+Egw@mail.gmail.com>
 <OS3PR01MB627586DA752A9E07F02FB7609EF09@OS3PR01MB6275.jpnprd01.prod.outlook.com>
 <CAA4eK1+63gaq1ruaUTvhM6-GRWi0waZT7X+TJL2tx+dWzisveQ@mail.gmail.com>
 <OS3PR01MB6275D4B22266412FEEBDB3AE9EF49@OS3PR01MB6275.jpnprd01.prod.outlook.com>
 <CAD21AoC=PCkeURevhZdNjcKHYws=paPcotgG1fAnq9g8GdwGXw@mail.gmail.com>
 <OS0PR01MB5716087C537FA8311F208B6094F59@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAFiTN-vy5aD2qEe2g8ssh+n=iyusFm84FxPsUyWOMeQdvbN3ow@mail.gmail.com>
 <CAA4eK1LdtevYSW_Lk5MJJeCMWqQtUgKCtPkX4FNKVS-ptEBGxw@mail.gmail.com>
 <CAFiTN-u_hFNEB6iQTtY9sLV_0fYX87OGH5BWbjTALYgfoHnxhw@mail.gmail.com>
 <OS0PR01MB5716C2CC59810717EC9601EE94FA9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAFiTN-sTYk=h75Jn1a7ee+5hOcdQFjKGDvF_0NWQQXmoBv4A+A@mail.gmail.com>
 <OS0PR01MB57162495C97CAEA88AC8E9DB94FA9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAFiTN-srQ2qZsx2uK6egT_jz9weBGm-Ff+gE8ObMu3sWYNKpFQ@mail.gmail.com> <OS0PR01MB5716BA08EABE25F07B61EB6D94FB9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
In-Reply-To: <OS0PR01MB5716BA08EABE25F07B61EB6D94FB9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
From: Dilip Kumar <dilipbalaut@gmail.com>
Date: Fri, 6 Jan 2023 11:24:25 +0530
Message-ID: <CAFiTN-sjpOm50q8SfoywuKQAqJAaE9FtjvwHBWHvi+pn39OWQQ@mail.gmail.com>
Subject: Re: Perform streaming logical transactions by background workers and
 parallel apply
To: "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>
Cc: Amit Kapila <amit.kapila16@gmail.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>
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CAFiTN-sjpOm50q8SfoywuKQAqJAaE9FtjvwHBWHvi%2Bpn39OWQQ%40mail.gmail.com>
Precedence: bulk

On Fri, Jan 6, 2023 at 9:37 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Thursday, January 5, 2023 7:54 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> Thanks, I have started another thread[1]
>
> Attach the parallel apply patch set here again. I didn't change the patch set,
> attach it here just to let the CFbot keep testing it.

I have completed the review and some basic testing and it mostly looks
fine to me.  Here is my last set of comments/suggestions.

1.
    /*
     * Don't start a new parallel worker if user has set skiplsn as it's
     * possible that they want to skip the streaming transaction. For
     * streaming transactions, we need to serialize the transaction to a file
     * so that we can get the last LSN of the transaction to judge whether to
     * skip before starting to apply the change.
     */
    if (!XLogRecPtrIsInvalid(MySubscription->skiplsn))
        return false;


I think this is fine to block parallelism in this case, but it is also
possible to make it less restrictive, basically, only if the first lsn
of the transaction is <= skiplsn, then only it is possible that the
final_lsn might match with skiplsn otherwise that is not possible. And
if we want then we can allow parallelism in that case.

I understand that currently we do not have first_lsn of the
transaction in stream start message but I think that should be easy to
do?  Although I am not sure if it is worth it, it's good to make a
note at least.

2.

+     * XXX Additionally, we also stop the worker if the leader apply worker
+     * serialize part of the transaction data due to a send timeout. This is
+     * because the message could be partially written to the queue and there
+     * is no way to clean the queue other than resending the message until it
+     * succeeds. Instead of trying to send the data which anyway would have
+     * been serialized and then letting the parallel apply worker deal with
+     * the spurious message, we stop the worker.
+     */
+    if (winfo->serialize_changes ||
+        list_length(ParallelApplyWorkerPool) >
+        (max_parallel_apply_workers_per_subscription / 2))

IMHO this reason (XXX Additionally, we also stop the worker if the
leader apply worker serialize part of the transaction data due to a
send timeout) for stopping the worker looks a bit hackish to me.  It
may be a rare case so I am not talking about the performance but the
reasoning behind stopping is not good. Ideally we should be able to
clean up the message queue and reuse the worker.

3.
+        else if (shmq_res == SHM_MQ_WOULD_BLOCK)
+        {
+            /* Replay the changes from the file, if any. */
+            if (pa_has_spooled_message_pending())
+            {
+                pa_spooled_messages();
+            }

I think we do not need this pa_has_spooled_message_pending() function.
Because this function is just calling pa_get_fileset_state() which is
acquiring mutex and getting filestate then if the filestate is not
FS_EMPTY then we call pa_spooled_messages() that will again call
pa_get_fileset_state() which will again acquire mutex.  I think when
the state is FS_SERIALIZE_IN_PROGRESS it will frequently call
pa_get_fileset_state() consecutively 2 times, and I think we can
easily achieve the same behavior with just one call.

4.

+     * leader, or when there there is an error. None of these cases will allow
+     * the code to reach here.

/when there there is an error/when there is an error


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com