public inbox for [email protected]  

help / color / mirror / Atom feed

From: Hayato Kuroda (Fujitsu) <[email protected]>
To: [email protected] <[email protected]>
To: Amit Kapila <[email protected]>
Cc: Masahiko Sawada <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Peter Smith <[email protected]>
Cc: Dilip Kumar <[email protected]>
Cc: [email protected] <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: RE: Perform streaming logical transactions by background workers and parallel apply
Date: Fri, 2 Dec 2022 11:27:19 +0000
Message-ID: <TYAPR01MB5866235A3B754897C03BCA1AF5179@TYAPR01MB5866.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <OS0PR01MB5716A1BCD72574DF420174FA94159@OS0PR01MB5716.jpnprd01.prod.outlook.com>
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>
	<CAA4eK1Kx76kakoz-YWxH1We_Ck_x7OAYPkmS2x5-sDeYkR1gAg@mail.gmail.com>
	<OS0PR01MB57161603FC9CB886F2CEE53194159@OS0PR01MB5716.jpnprd01.prod.outlook.com>
	<OS0PR01MB5716A1BCD72574DF420174FA94159@OS0PR01MB5716.jpnprd01.prod.outlook.com>

Dear Hou,

Thanks for making the patch. Followings are my comments for v54-0003 and 0004.

0003

pa_free_worker()

+       /* Unlink any files that were needed to serialize partial changes. */
+       if (winfo->serialize_changes)
+               stream_cleanup_files(MyLogicalRepWorker->subid, winfo->shared->xid);
+

I think this part is not needed, because the LA cannot reach here if winfo->serialize_changes is true. Moreover stream_cleanup_files() is done in pa_free_worker_info().

LogicalParallelApplyLoop()

The parallel apply worker wakes up every 0.1s even if we are in the PARTIAL_SERIALIZE mode. Do you have idea to reduce that?

```
+                       pa_spooled_messages();
```

Comments are needed here, like "Changes may be serialize...".

pa_stream_abort()

```
+                               /*
+                                * Reopen the file and set the file position to the saved
+                                * position.
+                                */
+                               if (reopen_stream_fd)
+                               {
+                                       char            path[MAXPGPATH];
+
+                                       changes_filename(path, MyLogicalRepWorker->subid, xid);
+                                       stream_fd = BufFileOpenFileSet(&MyParallelShared->fileset,
+                                                                                                  path, O_RDONLY, false);
+                                       BufFileSeek(stream_fd, fileno, offset, SEEK_SET);
+                               }
```

MyParallelShared->serialize_changes may be used instead of reopen_stream_fd.


worker.c

```
-#include "storage/buffile.h"
```

I think this include should not be removed.


handle_streamed_transaction()

```
+                       if (apply_action == TRANS_LEADER_SEND_TO_PARALLEL)
+                               pa_send_data(winfo, s->len, s->data);
+                       else
+                               stream_write_change(action, &original_msg);
```

Comments are needed here, 0001 has that bu removed in 0002.
There are some similar lines.


```
+                       /*
+                        * It is possible that while sending this change to parallel apply
+                        * worker we need to switch to serialize mode.
+                        */
+                       if (winfo->serialize_changes)
+                               pa_set_fileset_state(winfo->shared, FS_READY);
```

There are three same parts in the code, can we combine them to common part?

apply_spooled_messages()

```
+               /*
+                * Break the loop if the parallel apply worker has finished applying
+                * the transaction. The parallel apply worker should have closed the
+                * file before committing.
+                */
+               if (am_parallel_apply_worker() &&
+                       MyParallelShared->xact_state == PARALLEL_TRANS_FINISHED)
+                       goto done;
```

I thnk pfree(buffer) and pfree(s2.data) should not be skippied.
And this part should be at below "nchanges++;"


0004

set_subscription_retry()

```
+       LockSharedObject(SubscriptionRelationId, MySubscription->oid, 0,
+                                        AccessShareLock);
+
```

I think AccessExclusiveLock should be aquired instead of AccessShareLock.
In AlterSubscription(), LockSharedObject(AccessExclusiveLock) seems to be used.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

view thread (625+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: RE: Perform streaming logical transactions by background workers and parallel apply
  In-Reply-To: <TYAPR01MB5866235A3B754897C03BCA1AF5179@TYAPR01MB5866.jpnprd01.prod.outlook.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox