MIME-Version: 1.0
References: 
 <CAA4eK1JQTDXTfvJ5d+L0ggG4+doyd0Xji=e0OJsb=qcn_jWALA@mail.gmail.com>
 <OS0PR01MB5716D6D2765E54DC739F288E940A9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <OS0PR01MB5716997A7115715F9E4EE520940D9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1JEFVZcymteMYXhYghiX7Lb=MKuEsfxgD1VfaRrLCHyzg@mail.gmail.com>
 <OS0PR01MB571663F65904D00895AD159994109@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1JuRPUY2Dx=vUToRksTmW0ptqMVT3K32g0368f_ZCk-zg@mail.gmail.com>
 <OS0PR01MB5716B802A1733548A99761AE94129@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1LGKYUDFZ_jFPrU497wQf2HNvt5a+tCTpqSeWSG6kfpSA@mail.gmail.com>
 <CAA4eK1Jwh7j86Egk1cye=x2R_yrTjzXGj7Fx12wVybBAEq91kA@mail.gmail.com>
 <OS0PR01MB571690D6FF24E9D000ECBDBD94199@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <OS0PR01MB571639E4141EBF8A2C501E6994189@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1+ELh2vPre3JHyoeKV0A9_V7aXQD0QBPv86WEn7P_rK-g@mail.gmail.com>
 <OS0PR01MB57166A377638E57823219E32941A9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1KpN=dZPcCj=TLwh3wk9uvPB-M1Jrjw7wt+-L62YrDHFQ@mail.gmail.com>
 <OS0PR01MB571604D401DE7A0659BD6609941D9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1LJMpBYbgDVz=g4qig8C2bM10PK=DgW7o1zn8X426vJ+Q@mail.gmail.com>
 <OS0PR01MB571663B4EC5D8BE749AACD98941E9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAD21AoD_3ta_29sopuf_KJCExverP5808HjwDeqaHDrbqbxYww@mail.gmail.com>
 <OS0PR01MB5716D38323B07EB0A0D097F194E09@OS0PR01MB5716.jpnprd01.prod.outlook.com>
In-Reply-To: 
 <OS0PR01MB5716D38323B07EB0A0D097F194E09@OS0PR01MB5716.jpnprd01.prod.outlook.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Wed, 14 Dec 2022 12:18:41 +0530
Message-ID: 
 <CAA4eK1Kumnc+AR86AGJ666+4Ebfny+nMbs_JZSW3O__UEiGp6w@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: Masahiko Sawada <sawada.mshk@gmail.com>,
	"wangw.fnst@fujitsu.com" <wangw.fnst@fujitsu.com>,
 Peter Smith <smithpb2250@gmail.com>,
	Dilip Kumar <dilipbalaut@gmail.com>,
 "shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/mixed; boundary="000000000000446f7205efc42101"
Archived-At: 
 <https://www.postgresql.org/message-id/CAA4eK1Kumnc%2BAR86AGJ666%2B4Ebfny%2BnMbs_JZSW3O__UEiGp6w%40mail.gmail.com>
Precedence: bulk

--000000000000446f7205efc42101
Content-Type: text/plain; charset="UTF-8"

On Wed, Dec 14, 2022 at 9:50 AM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Tuesday, December 13, 2022 11:25 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > Here are comments on v59 0001, 0002 patches:
>
> Thanks for the comments!
>
> > +void
> > +pa_increment_stream_block(ParallelApplyWorkerShared *wshared) {
> > +        while (1)
> > +        {
> > +                SpinLockAcquire(&wshared->mutex);
> > +
> > +                /*
> > +                 * Don't try to increment the count if the parallel
> > apply worker is
> > +                 * taking the stream lock. Otherwise, there would be
> > a race condition
> > +                 * that the parallel apply worker checks there is no
> > pending streaming
> > +                 * block and before it actually starts waiting on a
> > lock, the leader
> > +                 * sends another streaming block and take the stream
> > lock again. In
> > +                 * this case, the parallel apply worker will start
> > waiting for the next
> > +                 * streaming block whereas there is actually a
> > pending streaming block
> > +                 * available.
> > +                 */
> > +                if (!wshared->pa_wait_for_stream)
> > +                {
> > +                        wshared->pending_stream_count++;
> > +                        SpinLockRelease(&wshared->mutex);
> > +                        break;
> > +                }
> > +
> > +                SpinLockRelease(&wshared->mutex);
> > +        }
> > +}
> >
> > I think we should add an assertion to check if we don't hold the stream lock.
> >
> > I think that waiting for pa_wait_for_stream to be false in a busy loop is not a
> > good idea. It's not interruptible and there is not guarantee that we can break
> > from this loop in a short time. For instance, if PA executes
> > pa_decr_and_wait_stream_block() a bit earlier than LA executes
> > pa_increment_stream_block(), LA has to wait for PA to acquire and release the
> > stream lock in a busy loop. It should not be long in normal cases but the
> > duration LA needs to wait for PA depends on PA, which could be long. Also
> > what if PA raises an error in
> > pa_lock_stream() due to some reasons? I think LA won't be able to detect the
> > failure.
> >
> > I think we should at least make it interruptible and maybe need to add some
> > sleep. Or perhaps we can use the condition variable for this case.
>

Or we can leave this while (true) logic altogether for the first
version and have a comment to explain this race. Anyway, after
restarting, it will probably be solved. We can always change this part
of the code later if this really turns out to be problematic.

> Thanks for the analysis, I will research this part.
>
> > ---
> > In worker.c, we have the following common pattern:
> >
> > case TRANS_LEADER_PARTIAL_SERIALIZE:
> >     write change to the file;
> >     do some work;
> >     break;
> >
> > case TRANS_LEADER_SEND_TO_PARALLEL:
> >     pa_send_data();
> >
> >     if (winfo->serialize_changes)
> >     {
> >         do some worker required after writing changes to the file.
> >     }
> >     :
> >     break;
> >
> > IIUC there are two different paths for partial serialization: (a) where
> > apply_action is TRANS_LEADER_PARTIAL_SERIALIZE, and (b) where
> > apply_action is TRANS_LEADER_PARTIAL_SERIALIZE and
> > winfo->serialize_changes became true. And we need to match what we do
> > in (a) and (b). Rather than having two different paths for the same case, how
> > about falling through TRANS_LEADER_PARTIAL_SERIALIZE when we could not
> > send the changes? That is, pa_send_data() just returns false when the timeout
> > exceeds and we need to switch to serialize changes, otherwise returns true. If it
> > returns false, we prepare for switching to serialize changes such as initializing
> > fileset, and fall through TRANS_LEADER_PARTIAL_SERIALIZE case. The code
> > would be like:
> >
> > case TRANS_LEADER_SEND_TO_PARALLEL:
> >     ret = pa_send_data();
> >
> >     if (ret)
> >     {
> >         do work for sending changes to PA.
> >         break;
> >     }
> >
> >     /* prepare for switching to serialize changes */
> >     winfo->serialize_changes = true;
> >     initialize fileset;
> >     acquire stream lock if necessary;
> >
> >     /* FALLTHROUGH */
> > case TRANS_LEADER_PARTIAL_SERIALIZE:
> >     do work for serializing changes;
> >     break;
>
> I think that the suggestion is to extract the code that switch to serialize
> mode out of the pa_send_data(), and then we need to add that logic in all the
> functions which call pa_send_data(), I am not sure if it looks better as it
> might introduce some more codes in each handling function.
>

How about extracting the common code from apply_handle_stream_commit
and apply_handle_stream_prepare to a separate function say
pa_xact_finish_common()? I see there is a lot of common code (unlock
the stream, wait for the finish, store flush location, free worker
info) in both the functions for TRANS_LEADER_PARTIAL_SERIALIZE and
TRANS_LEADER_SEND_TO_PARALLEL cases.

>
> > ---
> >  void
> > pa_lock_stream(TransactionId xid, LOCKMODE lockmode) {
> >     LockApplyTransactionForSession(MyLogicalRepWorker->subid, xid,
> >                                    PARALLEL_APPLY_LOCK_STREAM,
> > lockmode); }
> >
> > I think since we don't need to let the caller to specify the lock mode but need
> > only shared and exclusive modes, we can make it simple by having a boolean
> > argument say shared instead of lockmode.
>
> I personally think passing the lockmode would make the code more clear
> than passing a Boolean value.
>

+1.

I have made a few changes in the newly added comments and function
name in the attached patch. Kindly include this if you find the
changes okay.

-- 
With Regards,
Amit Kapila.

--000000000000446f7205efc42101
Content-Type: application/octet-stream; name="changes_amit_v60.patch"
Content-Disposition: attachment; filename="changes_amit_v60.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_lbnajyd80>
X-Attachment-Id: f_lbnajyd80

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL3JlcGxpY2F0aW9uL2xvZ2ljYWwvYXBwbHlwYXJhbGxl
bHdvcmtlci5jIGIvc3JjL2JhY2tlbmQvcmVwbGljYXRpb24vbG9naWNhbC9hcHBseXBhcmFsbGVs
d29ya2VyLmMKaW5kZXggMmUyY2QwZDUzNS4uMTU4ZGJkOTNjOSAxMDA2NDQKLS0tIGEvc3JjL2Jh
Y2tlbmQvcmVwbGljYXRpb24vbG9naWNhbC9hcHBseXBhcmFsbGVsd29ya2VyLmMKKysrIGIvc3Jj
L2JhY2tlbmQvcmVwbGljYXRpb24vbG9naWNhbC9hcHBseXBhcmFsbGVsd29ya2VyLmMKQEAgLTcw
NywxMCArNzA3LDEwIEBAIHBhX3N0b3BfaWRsZV93b3JrZXJzKHZvaWQpCiB9CiAKIC8qCi0gKiBD
aGVjayBpZiB0aGUgcGFyYWxsZWwgYXBwbHkgd29ya2VyIGlzIHBlbmRpbmcgZHVlIHRvIHNwb29s
ZWQgbWVzc2FnZXMuCisgKiBSZXR1cm5zIHRydWUsIGlmIHRoZXJlIGlzIGFueSBwZW5kaW5nIHNw
b29sZWQgbWVzc2FnZSwgZmFsc2Ugb3RoZXJ3aXNlLgogICovCiBzdGF0aWMgYm9vbAotcGFfaGFz
X3Nwb29sZWRfbWVzc2FnZV9wZW5kaW5nKCkKK3BhX2lzX3Nwb29sZWRfbWVzc2FnZV9wZW5kaW5n
KCkKIHsKIAlQYXJ0aWFsRmlsZVNldFN0YXRlIGZpbGVzZXRfc3RhdGU7CiAKQEAgLTg3MCw3ICs4
NzAsNyBAQCBMb2dpY2FsUGFyYWxsZWxBcHBseUxvb3Aoc2htX21xX2hhbmRsZSAqbXFoKQogCQll
bHNlIGlmIChzaG1xX3JlcyA9PSBTSE1fTVFfV09VTERfQkxPQ0spCiAJCXsKIAkJCS8qIENoZWNr
IGlmIGNoYW5nZXMgaGF2ZSBiZWVuIHNlcmlhbGl6ZWQgdG8gYSBmaWxlLiAqLwotCQkJaWYgKHBh
X2hhc19zcG9vbGVkX21lc3NhZ2VfcGVuZGluZygpKQorCQkJaWYgKHBhX2lzX3Nwb29sZWRfbWVz
c2FnZV9wZW5kaW5nKCkpCiAJCQl7CiAJCQkJcGFfc3Bvb2xlZF9tZXNzYWdlcygpOwogCQkJfQpk
aWZmIC0tZ2l0IGEvc3JjL2JhY2tlbmQvcmVwbGljYXRpb24vbG9naWNhbC93b3JrZXIuYyBiL3Ny
Yy9iYWNrZW5kL3JlcGxpY2F0aW9uL2xvZ2ljYWwvd29ya2VyLmMKaW5kZXggZmFlYmNlYjRjNy4u
NTlhNDM2NmZlYyAxMDA2NDQKLS0tIGEvc3JjL2JhY2tlbmQvcmVwbGljYXRpb24vbG9naWNhbC93
b3JrZXIuYworKysgYi9zcmMvYmFja2VuZC9yZXBsaWNhdGlvbi9sb2dpY2FsL3dvcmtlci5jCkBA
IC0xNTU4LDE1ICsxNTU4LDE3IEBAIGFwcGx5X2hhbmRsZV9zdHJlYW1fc3RhcnQoU3RyaW5nSW5m
byBzKQogCiAJCQlwYV9zZW5kX2RhdGEod2luZm8sIHMtPmxlbiwgcy0+ZGF0YSwgIWZpcnN0X3Nl
Z21lbnQpOwogCisJCQkvKgorCQkJICogT25jZSB3ZSBzdGFydCBzZXJpYWxpemluZyB0aGUgY2hh
bmdlcywgdGhlIHBhcmFsbGVsIGFwcGx5IHdvcmtlcgorCQkJICogd2lsbCB3YWl0IGZvciB0aGUg
bGVhZGVyIHRvIHJlbGVhc2UgdGhlIHN0cmVhbSBsb2NrIHVudGlsIHRoZQorCQkJICogZW5kIG9m
IHRoZSB0cmFuc2FjdGlvbi4gU28sIHdlIGRvbid0IG5lZWQgdG8gcmVsZWFzZSB0aGUgbG9jayBv
cgorCQkJICogaW5jcmVtZW50IHRoZSBzdHJlYW0gY291bnQgaW4gdGhhdCBjYXNlLgorCQkJICov
CiAJCQlpZiAoIXdpbmZvLT5zZXJpYWxpemVfY2hhbmdlcykKIAkJCXsKIAkJCQkvKgotCQkJCSAq
IFVubG9jayB0aGUgc2hhcmVkIG9iamVjdCBsb2NrIHdoZW4gbm8gY2hhbmdlIHdhcyBzZXJpYWxp
emVkCi0JCQkJICogc28gdGhhdCB0aGUgcGFyYWxsZWwgYXBwbHkgd29ya2VyIGNhbiBjb250aW51
ZSB0byByZWNlaXZlCi0JCQkJICogY2hhbmdlcy4gTm8gbmVlZCB0byB0YWtlIHRoZSBsb2NrIGlm
IHN3aXRjaGVkIHRvIHNlcmlhbGl6ZQotCQkJCSAqIG1vZGUgYmVjYXVzZSB0aGUgcGFyYWxsZWwg
YXBwbHkgd29ya2VyIHNob3VsZCB3YWl0IGZvciB0aGUKLQkJCQkgKiBsZWFkZXIgdG8gcmVsZWFz
ZSB0aGUgc3RyZWFtIGxvY2sgdW50aWwgdGhlIGVuZCBvZiB0aGUKLQkJCQkgKiB0cmFuc2FjdGlv
bi4KKwkJCQkgKiBVbmxvY2sgdGhlIHNoYXJlZCBvYmplY3QgbG9jayBzbyB0aGF0IHRoZSBwYXJh
bGxlbCBhcHBseQorCQkJCSAqIHdvcmtlciBjYW4gY29udGludWUgdG8gcmVjZWl2ZSBjaGFuZ2Vz
LgogCQkJCSAqLwogCQkJCWlmICghZmlyc3Rfc2VnbWVudCkKIAkJCQkJcGFfdW5sb2NrX3N0cmVh
bSh3aW5mby0+c2hhcmVkLT54aWQsIEFjY2Vzc0V4Y2x1c2l2ZUxvY2spOwo=
--000000000000446f7205efc42101--