Dear Hou,
Thanks for updating the patch! Followings are my comments.
===
01. applyparallelworker.c - SIZE_STATS_MESSAGE
```
/*
* There are three fields in each message received by the parallel apply
* worker: start_lsn, end_lsn and send_time. Because we have updated these
* statistics in the
On Tue, Oct 18, 2022 at 5:22 PM Dilip Kumar wrote:
>
> On Thu, Oct 6, 2022 at 1:37 PM Masahiko Sawada wrote:
> >
>
> > While looking at v35 patch, I realized that there are some cases where
> > the logical replication gets stuck depending on partitioned table
> > structure. For instance, there ar
On Thu, Oct 6, 2022 at 1:37 PM Masahiko Sawada wrote:
>
> While looking at v35 patch, I realized that there are some cases where
> the logical replication gets stuck depending on partitioned table
> structure. For instance, there are following tables, publication, and
> subscription:
>
> * On pub
On Tue, Oct 18, 2022 at 8:06 AM Peter Smith wrote:
>
> Hi, here are my review comments for patch v38-0001.
>
> 3.
>
> + /* Ensure we are reading the data into our memory context. */
> + oldctx = MemoryContextSwitchTo(ApplyMessageContext);
>
> Doesn't something need to switch back to this 'oldctx'
On Tuesday, October 18, 2022 10:36 AM Peter Smith wrote:
>
> Hi, here are my review comments for patch v38-0001.
Thanks for the comments.
> ~~~
>
> 12. get_transaction_apply_action
>
> I still felt like there should be some tablesync checks/comments in
> this function, just for sanity, even i
Hi, here are my review comments for patch v38-0001.
==
.../replication/logical/applyparallelworker.c
1. parallel_apply_start_worker
+ /* Try to get a free parallel apply worker. */
+ foreach(lc, ParallelApplyWorkersList)
+ {
+ ParallelApplyWorkerInfo *tmp_winfo;
+
+ tmp_winfo = (ParallelApp
On Wed, Oct 12, 2022 at 18:11 PM Peter Smith wrote:
> Here are some review comments for v36-0001.
Thanks for your comments.
> ==
>
> 1. GENERAL
>
> Houzj wrote ([1] #3a):
> The word 'streaming' is the same as the actual option name, so
> personally I think it's fine. But if others also ag
On Wed, Oct 12, 2022 at 7:41 AM wangw.f...@fujitsu.com
wrote:
>
> On Fri, Oct 7, 2022 at 14:18 PM Hou, Zhijie/侯 志杰
> wrote:
> > Attach the new version patch set which addressed most of the comments.
>
> Rebased the patch set because the new change in HEAD (776e1c8).
>
> Attach the new patch set.
On Wed, Oct 12, 2022 at 3:41 PM Peter Smith wrote:
>
> Here are some review comments for v36-0001.
>
>
> 6. LogicalParallelApplyLoop
>
> + for (;;)
> + {
> + void*data;
> + Size len;
> + int c;
> + StringInfoData s;
> + MemoryContext oldctx;
> +
> + CHECK_FOR_INTERRUPTS();
> +
> + /* Ensure we
On Thu, Oct 6, 2022 at 6:09 PM kuroda.hay...@fujitsu.com
wrote:
>
> ~~~
> 10. worker.c - apply_handle_stream_start
>
> ```
> + *
> + * XXX We can avoid sending pair of the START/STOP messages to the parallel
> + * worker because unlike apply worker it will process only one
> + * transaction-at-a-t
Here are some review comments for v36-0001.
==
1. GENERAL
Houzj wrote ([1] #3a):
The word 'streaming' is the same as the actual option name, so
personally I think it's fine. But if others also agreed that the name
can be improved, I can change it.
~
Sure, I was not really complaining that
On Tue, Oct 11, 2022 at 5:52 AM Masahiko Sawada wrote:
>
> On Fri, Oct 7, 2022 at 2:00 PM Amit Kapila wrote:
> >
> > About your point that having different partition structures for
> > publisher and subscriber, I don't know how common it will be once we
> > have DDL replication. Also, the default
On Fri, Oct 7, 2022 at 2:00 PM Amit Kapila wrote:
>
> On Fri, Oct 7, 2022 at 8:47 AM Masahiko Sawada wrote:
> >
> > On Thu, Oct 6, 2022 at 9:04 PM houzj.f...@fujitsu.com
> > wrote:
> > >
> > > I think the root reason for this kind of deadlock problems is the table
> > > structure difference betw
On Thursday, October 6, 2022 8:40 PM Kuroda, Hayato/黒田 隼人
wrote:
>
> Dear Hou,
>
> I put comments for v35-0001.
Thanks for the comments.
> 01. catalog.sgml
>
> ```
> + Controls how to handle the streaming of in-progress transactions:
> + f = disallow streaming of in-progress tran
On Fri, Oct 7, 2022 at 8:38 AM Peter Smith wrote:
>
> On Thu, Oct 6, 2022 at 10:38 PM Amit Kapila wrote:
> >
> > On Fri, Sep 30, 2022 at 1:56 PM Peter Smith wrote:
> > >
> > > Here are my review comments for the v35-0001 patch:
> > >
> > > ==
> > >
> > > 3. GENERAL
> > >
> > > (this comment
On Fri, Oct 7, 2022 at 8:47 AM Masahiko Sawada wrote:
>
> On Thu, Oct 6, 2022 at 9:04 PM houzj.f...@fujitsu.com
> wrote:
> >
> > I think the root reason for this kind of deadlock problems is the table
> > structure difference between publisher and subscriber(similar to the unique
> > difference r
ilip
> > Kumar ; Shi, Yu/侍 雨 ;
> > PostgreSQL Hackers
> > Subject: Re: Perform streaming logical transactions by background workers
> > and
> > parallel apply
> >
> > On Tue, Sep 27, 2022 at 9:26 PM houzj.f...@fujitsu.com
> > wrote:
> > >
On Thu, Oct 6, 2022 at 10:38 PM Amit Kapila wrote:
>
> On Fri, Sep 30, 2022 at 1:56 PM Peter Smith wrote:
> >
> > Here are my review comments for the v35-0001 patch:
> >
> > ==
> >
> > 3. GENERAL
> >
> > (this comment was written after I wrote all the other ones below so
> > there might be so
On Thursday, October 6, 2022 9:00 PM Kuroda, Hayato/黒田 隼人
wrote:
>
> Dear Hou,
>
> > Thanks for the suggestion.
> >
> > I tried to add a WaitLatch, but it seems affect the performance
> > because the Latch might not be set when leader send some message to
> > parallel apply worker which means i
Dear Hou,
> Thanks for the suggestion.
>
> I tried to add a WaitLatch, but it seems affect the performance
> because the Latch might not be set when leader send some
> message to parallel apply worker which means it will wait until
> timeout.
Yes, currently it leader does not notify anything.
To
Dear Hou,
I put comments for v35-0001.
01. catalog.sgml
```
+ Controls how to handle the streaming of in-progress transactions:
+ f = disallow streaming of in-progress transactions,
+ t = spill the changes of in-progress transactions to
+ disk and apply at once after the
On Thursday, October 6, 2022 6:54 PM Kuroda, Hayato/黒田 隼人
wrote:
>
> Dear Amit,
>
> > Can't we use WaitLatch in the case of SHM_MQ_WOULD_BLOCK as we are
> > using it for the same case at some other place in the code? We can use
> > the same nap time as we are using in the leader apply worker.
>
> -Original Message-
> From: Masahiko Sawada
> Sent: Thursday, October 6, 2022 4:07 PM
> To: Hou, Zhijie/侯 志杰
> Cc: Amit Kapila ; Wang, Wei/王 威
> ; Peter Smith ; Dilip
> Kumar ; Shi, Yu/侍 雨 ;
> PostgreSQL Hackers
> Subject: Re: Perform streaming logical
On Fri, Sep 30, 2022 at 1:56 PM Peter Smith wrote:
>
> Here are my review comments for the v35-0001 patch:
>
> ==
>
> 3. GENERAL
>
> (this comment was written after I wrote all the other ones below so
> there might be some unintended overlaps...)
>
> I found the mixed use of the same member na
Dear Amit,
> Can't we use WaitLatch in the case of SHM_MQ_WOULD_BLOCK as we are
> using it for the same case at some other place in the code? We can use
> the same nap time as we are using in the leader apply worker.
I'm not sure whether such a short nap time is needed or not.
Because unlike lead
On Thu, Sep 29, 2022 at 3:20 PM kuroda.hay...@fujitsu.com
wrote:
>
> Dear Hou,
>
> Thanks for updating patch. I will review yours soon, but I reply to your
> comment.
>
> > > 04. applyparallelworker.c - LogicalParallelApplyLoop()
> > >
> > > ```
> > > + shmq_res = shm_mq_receive(mqh
On Tue, Sep 27, 2022 at 9:26 PM houzj.f...@fujitsu.com
wrote:
>
> On Saturday, September 24, 2022 7:40 PM Amit Kapila
> wrote:
> >
> > On Thu, Sep 22, 2022 at 3:41 PM Amit Kapila
> > wrote:
> > >
> > > On Thu, Sep 22, 2022 at 8:59 AM wangw.f...@fujitsu.com
> > > wrote:
> > > >
> > >
> > > Few
Here are my review comments for the v35-0001 patch:
==
1. Commit message
Currently, for large transactions, the publisher sends the data in multiple
streams (changes divided into chunks depending upon logical_decoding_work_mem),
and then on the subscriber-side, the apply worker writes the ch
Dear Hou,
Thanks for updating patch. I will review yours soon, but I reply to your
comment.
> > 04. applyparallelworker.c - LogicalParallelApplyLoop()
> >
> > ```
> > + shmq_res = shm_mq_receive(mqh, &len, &data, false);
> > ...
> > + if (ConfigReloadPending)
> > +
On Tuesday, September 27, 2022 2:32 PM Kuroda, Hayato/黒田 隼人
>
> Dear Wang,
>
> Followings are comments for your patchset.
Thanks for the comments.
>
> 0001
>
>
> 01. launcher.c - logicalrep_worker_stop_internal()
>
> ```
> +
> + Assert(LWLockHeldByMe(LogicalRepWorkerLock));
> +
On Monday, September 26, 2022 6:58 PM Amit Kapila
wrote:
>
> On Mon, Sep 26, 2022 at 8:41 AM wangw.f...@fujitsu.com
> wrote:
> >
> > On Thur, Sep 22, 2022 at 18:12 PM Amit Kapila
> wrote:
> >
> > > 3.
> > > ApplyWorkerMain()
> > > {
> > > ...
> > > ...
> > > +
> > > + if (server_version >= 160
Dear Wang,
Followings are comments for your patchset.
0001
01. launcher.c - logicalrep_worker_stop_internal()
```
+
+ Assert(LWLockHeldByMe(LogicalRepWorkerLock));
+
```
I think it should be Assert(LWLockHeldByMeInMode(LogicalRepWorkerLock,
LW_SHARED))
because the lock is released
On Mon, Sep 26, 2022 at 8:41 AM wangw.f...@fujitsu.com
wrote:
>
> On Thur, Sep 22, 2022 at 18:12 PM Amit Kapila wrote:
>
> > 3.
> > ApplyWorkerMain()
> > {
> > ...
> > ...
> > +
> > + if (server_version >= 16 &&
> > + MySubscription->stream == SUBSTREAM_PARALLEL)
> > + options.proto.logical.s
Dear Wang,
Thanks for updating patch!... but cfbot says that it cannot be accepted [1].
I thought the header should be included, like miscadmin.h.
[1]: https://cirrus-ci.com/task/5909508684775424
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
On Thur, Sep 22, 2022 at 18:12 PM Amit Kapila wrote:
> Few comments on v33-0001
> ===
Thanks for your comments.
> 1.
> + else if (data->streaming == SUBSTREAM_PARALLEL &&
> + data->protocol_version <
> LOGICALREP_PROTO_STREAM_PARALLEL_VERSION_NUM)
> + ereport(ERROR,
> + (errc
On Thu, Sep 22, 2022 at 3:41 PM Amit Kapila wrote:
>
> On Thu, Sep 22, 2022 at 8:59 AM wangw.f...@fujitsu.com
> wrote:
> >
>
> Few comments on v33-0001
> ===
>
Some more comments on v33-0001
=
1.
+ /* Information from the corresponding LogicalRepWo
On Thu, Sep 22, 2022 at 4:50 PM kuroda.hay...@fujitsu.com
wrote:
>
> Hi Amit,
>
> > > I checked other flags that are set by signal handlers, their datatype
> > > seemed to
> > be sig_atomic_t.
> > > Is there any reasons that you use normal bool? It should be changed if
> > > not.
> > >
> >
> > I
Hi Amit,
> > I checked other flags that are set by signal handlers, their datatype
> > seemed to
> be sig_atomic_t.
> > Is there any reasons that you use normal bool? It should be changed if not.
> >
>
> It follows the logic similar to ParallelMessagePending. Do you see any
> problem with it?
H
On Thu, Sep 22, 2022 at 8:59 AM wangw.f...@fujitsu.com
wrote:
>
Few comments on v33-0001
===
1.
+ else if (data->streaming == SUBSTREAM_PARALLEL &&
+ data->protocol_version < LOGICALREP_PROTO_STREAM_PARALLEL_VERSION_NUM)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VA
On Thu, Sep 22, 2022 at 1:37 PM kuroda.hay...@fujitsu.com
wrote:
>
> ===
> applyparallelworker.c
>
> 03. declaration
>
> ```
> +/*
> + * Is there a message pending in parallel apply worker which we need to
> + * receive?
> + */
> +volatile bool ParallelApplyMessagePending = false;
> ```
>
> I chec
On Thursday, September 22, 2022 4:08 PM Kuroda, Hayato/黒田 隼人
wrote:
>
> Thanks for updating the patch! Followings are comments for v33-0001.
Thanks for the comments.
> 04. HandleParallelApplyMessages()
>
> ```
> + if (winfo->error_mq_handle == NULL)
> + con
Dear Wang,
Thanks for updating the patch! Followings are comments for v33-0001.
===
libpqwalreceiver.c
01. inclusion
```
+#include "catalog/pg_subscription.h"
```
We don't have to include it because the analysis of parameters is done at
caller.
===
launcher.c
02. logicalrep_worker_launch()
On Wed, Sep 21, 2022 at 2:55 PM Peter Smith wrote:
>
> ==
>
> 3. .../replication/logical/applyparallelworker.c - parallel_apply_can_start
>
> +/*
> + * Returns true, if it is allowed to start a parallel apply worker, false,
> + * otherwise.
> + */
> +static bool
> +parallel_apply_can_start(Tra
Here are some review comments for patch v30-0001.
==
1. Commit message
In addition, the patch extends the logical replication STREAM_ABORT message so
that abort_time and abort_lsn can also be sent which can be used to update the
replication origin in parallel apply worker when the streaming
> FYI -
>
> The latest patch 30-0001 fails to apply, it seems due to a recent commit [1].
>
> [postgres@CentOS7-x64 oss_postgres_misc]$ git apply
> ../patches_misc/v30-0001-Perform-streaming-logical-transactions-by-
> parall.patch
> error: patch failed: src/include/replication/logicalproto.h:246
FYI -
The latest patch 30-0001 fails to apply, it seems due to a recent commit [1].
[postgres@CentOS7-x64 oss_postgres_misc]$ git apply
../patches_misc/v30-0001-Perform-streaming-logical-transactions-by-parall.patch
error: patch failed: src/include/replication/logicalproto.h:246
error: src/includ
On Mon, Sept 19, 2022 11:26 AM Wang, Wei/王 威 wrote:
>
>
> Improved as suggested.
>
Thanks for updating the patch. Here are some comments on 0001 patch.
1.
+ case TRANS_LEADER_SERIALIZE:
- oldctx = MemoryContextSwitchTo(ApplyContext);
+ /*
+
On Thu, Sep 15, 2022 at 10:45 AM wangw.f...@fujitsu.com
wrote:
>
> Attach the new patch set.
>
Review of v29-0001*
==
1.
+parallel_apply_find_worker(TransactionId xid)
{
...
+ entry = hash_search(ParallelApplyWorkersHash, &xid, HASH_FIND, &found);
+ if (found)
+ {
+ /* If any work
On Tues, Sep 13, 2022 at 20:02 PM Kuroda, Hayato/黒田 隼人
wrote:
> Dear Hou-san,
>
> > I will dig your patch more, but I send partially to keep the activity of
> > the thread.
>
> More minor comments about v28.
Thanks for your comments.
> ===
> About 0002
>
> For 015_stream.pl
>
> 14. check_p
On Wed, Sep 13, 2022 at 18:26 PM Amit Kapila wrote:
>
Thanks for your comments.
> On Fri, Sep 9, 2022 at 12:32 PM Peter Smith wrote:
> >
> > 29. src/backend/replication/logical/worker.c - TransactionApplyAction
> >
> > /*
> > * What action to take for the transaction.
> > *
> > * TA_APPLY_IN
On Tues, Sep 13, 2022 at 17:49 PM Amit Kapila wrote:
>
Thanks for your comments.
> On Fri, Sep 9, 2022 at 2:31 PM houzj.f...@fujitsu.com
> wrote:
> >
> > On Friday, September 9, 2022 3:02 PM Peter Smith
> wrote:
> > >
> >
> > > 3.
> > >
> > > max_logical_replication_workers (integer)
> > >
On Mon, Sep 12, 2022 at 18:58 PM Kuroda, Hayato/黒田 隼人
wrote:
> Dear Hou-san,
>
> Thank you for updating the patch! Followings are comments for v28-0001.
> I will dig your patch more, but I send partially to keep the activity of the
> thread.
Thanks for your comments.
> ===
> For applyparallel
On Fri, Sep 9, 2022 at 15:02 PM Peter Smith wrote:
> Here are my review comments for the v28-0001 patch:
>
> (There may be some overlap with other people's review comments and/or
> some fixes already made).
Thanks for your comments.
> 5. src/backend/libpq/pqmq.c
>
> + {
> + if (IsParallelWorke
On Thur, Sep 8, 2022 at 19:25 PM Amit Kapila wrote:
> On Thu, Sep 8, 2022 at 12:21 PM Amit Kapila wrote:
> >
> > On Mon, Sep 5, 2022 at 6:34 PM houzj.f...@fujitsu.com
> > wrote:
> > >
> > > Attach the correct patch set this time.
> > >
> >
> > Few comments on v28-0001*:
> > =
Hi,
> > 01. filename
> > The word-ordering of filename seems not good
> > because you defined the new worker as "parallel apply worker".
> >
>
> I think in the future we may have more files for apply work (like
> applyddl.c for DDL apply work), so it seems okay to name all apply
> related files i
Dear Hou-san,
> I will dig your patch more, but I send partially to keep the activity of the
> thread.
More minor comments about v28.
===
About 0002
For 015_stream.pl
14. check_parallel_log
```
+# Check the log that the streamed transaction was completed successfully
+# reported by parallel
On Mon, Sep 12, 2022 at 4:27 PM kuroda.hay...@fujitsu.com
wrote:
>
> Dear Hou-san,
>
> Thank you for updating the patch! Followings are comments for v28-0001.
> I will dig your patch more, but I send partially to keep the activity of the
> thread.
>
> ===
> For applyparallelworker.c
>
> 01. filen
On Fri, Sep 9, 2022 at 12:32 PM Peter Smith wrote:
>
> 29. src/backend/replication/logical/worker.c - TransactionApplyAction
>
> /*
> * What action to take for the transaction.
> *
> * TA_APPLY_IN_LEADER_WORKER means that we are in the leader apply worker and
> * changes of the transaction are
On Fri, Sep 9, 2022 at 2:31 PM houzj.f...@fujitsu.com
wrote:
>
> On Friday, September 9, 2022 3:02 PM Peter Smith
> wrote:
> >
>
> > 3.
> >
> > max_logical_replication_workers (integer)
> > Specifies maximum number of logical replication workers. This
> > includes apply leader workers, paral
Dear Hou-san,
Thank you for updating the patch! Followings are comments for v28-0001.
I will dig your patch more, but I send partially to keep the activity of the
thread.
===
For applyparallelworker.c
01. filename
The word-ordering of filename seems not good
because you defined the new worker a
On Friday, September 9, 2022 3:02 PM Peter Smith wrote:
>
> Here are my review comments for the v28-0001 patch:
>
> (There may be some overlap with other people's review comments and/or
> some fixes already made).
>
Thanks for the comments.
> 3.
>
> max_logical_replication_workers (integer)
Here are my review comments for the v28-0001 patch:
(There may be some overlap with other people's review comments and/or
some fixes already made).
==
1. Commit Message
In addition, the patch extends the logical replication STREAM_ABORT message so
that abort_time and abort_lsn can also be s
On Thu, Sep 8, 2022 at 12:21 PM Amit Kapila wrote:
>
> On Mon, Sep 5, 2022 at 6:34 PM houzj.f...@fujitsu.com
> wrote:
> >
> > Attach the correct patch set this time.
> >
>
> Few comments on v28-0001*:
> ===
>
Some suggestions for comments in v28-0001*
1.
+/*
+ * Entry for a h
On Mon, Sep 5, 2022 at 6:34 PM houzj.f...@fujitsu.com
wrote:
>
> Attach the correct patch set this time.
>
Few comments on v28-0001*:
===
1.
+ /* Whether the worker is processing a transaction. */
+ bool in_use;
I think this same comment applies to in_parallel_apply_xact flag
On Thu, Sep 1, 2022 at 4:53 PM houzj.f...@fujitsu.com
wrote:
>
Review of v27-0001*:
1. I feel the usage of in_remote_transaction and in_use flags is
slightly complex. IIUC, the patch uses in_use flag to ensure commit
ordering by waiting for it to become false before proceeding in
On Tue, Aug 30, 2022 at 12:12 PM Amit Kapila wrote:
>
> Few other comments on v25-0001*
>
>
Some more comments on v25-0001*:
=
1.
+static void
+apply_handle_stream_abort(StringInfo s)
...
...
+ else if (apply_action == TA_SEND_TO_PARALLEL_W
On Mon, Aug 29, 2022 at 5:01 PM houzj.f...@fujitsu.com
wrote:
>
> On Thursday, August 25, 2022 7:33 PM Amit Kapila
> wrote:
> >
>
> > 11.
> > + /*
> > + * Attach to the message queue.
> > + */
> > + mq = shm_toc_lookup(toc, APPLY_BGWORKER_KEY_ERROR_QUEUE, false);
> > + shm_mq_set_sender(mq, MyPr
On Tues, Aug 24, 2022 16:41 PM Kuroda, Hayato/黒田 隼人
wrote:
> Dear Wang,
>
> Followings are my comments about v23-0003. Currently I do not have any
> comments about 0002 and 0004.
Thanks for your comments.
> 09. general
>
> It seems that logicalrep_rel_mark_parallel_apply() is always called
On Fri, Aug 26, 2022 at 9:30 AM Dilip Kumar wrote:
>
> On Thu, Aug 11, 2022 at 12:13 PM Amit Kapila wrote:
> > > Since we will later consider applying non-streamed transactions in
> > > parallel, I
> > > think "apply streaming worker" might not be very suitable. I think
> > > PostgreSQL
> > > a
On Thu, Aug 11, 2022 at 12:13 PM Amit Kapila wrote:
> > Since we will later consider applying non-streamed transactions in
> > parallel, I
> > think "apply streaming worker" might not be very suitable. I think
> > PostgreSQL
> > also has the worker "parallel worker", so for "apply parallel worke
On Wed, Aug 24, 2022 at 7:17 PM houzj.f...@fujitsu.com
wrote:
>
> On Friday, August 19, 2022 4:49 PM Amit Kapila
> >
>
> > 8. It is not clear to me how APPLY_BGWORKER_EXIT status is used. Is it
> > required
> > for the cases where bgworker exists due to some error and then apply worker
> > uses
On Thur, Aug 18, 2022 11:44 AM Peter Smith wrote:
> Here are my review comments for patch v21-0001:
>
> Note - There are some "general" comments which will result in lots of
> smaller changes. The subsequent "detailed" review comments have some
> overlap with these general comments but I expect
On Mon, Aug 22, 2022 20:50 PM Kuroda, Hayato/黒田 隼人
wrote:
> Dear Wang,
>
> Thank you for updating the patch! Followings are comments about
> v23-0001 and v23-0005.
Thanks for your comments.
> v23-0001
>
> 01. logical-replication.sgml
>
> +
> + When the streaming mode is parallel, the fi
Dear Wang,
Followings are my comments about v23-0003. Currently I do not have any comments
about 0002 and 0004.
09. general
It seems that logicalrep_rel_mark_parallel_apply() is always called when
relations are opened on the subscriber-side,
but is it really needed? There checks are required o
On Mon, Aug 22, 2022 at 10:49 PM kuroda.hay...@fujitsu.com
wrote:
>
> 04. launcher.c
>
> 04.a
>
> + worker->main_worker_pid = is_subworker ? MyProcPid : 0;
>
> You can use InvalidPid instead of 0.
> (I thought pid should be represented by the datatype pid_t, but in some codes
> it is defin
On Mon, Aug 22, 2022 at 7:01 PM Amit Kapila wrote:
>
> On Mon, Aug 22, 2022 at 4:42 AM Peter Smith wrote:
> >
> > On Fri, Aug 19, 2022 at 7:55 PM Amit Kapila wrote:
> > >
> > > On Fri, Aug 19, 2022 at 3:05 PM Peter Smith wrote:
> > > >
> > > > On Fri, Aug 19, 2022 at 7:10 PM Amit Kapila
> > >
Dear Wang,
Thank you for updating the patch! Followings are comments about v23-0001 and
v23-0005.
v23-0001
01. logical-replication.sgml
+
+ When the streaming mode is parallel, the finish LSN of
+ failed transactions may not be logged. In that case, it may be necessary to
+ change the
On Mon, Aug 22, 2022 at 4:42 AM Peter Smith wrote:
>
> On Fri, Aug 19, 2022 at 7:55 PM Amit Kapila wrote:
> >
> > On Fri, Aug 19, 2022 at 3:05 PM Peter Smith wrote:
> > >
> > > On Fri, Aug 19, 2022 at 7:10 PM Amit Kapila
> > > wrote:
> > > >
> > > > On Fri, Aug 19, 2022 at 2:36 PM Peter Smith
On Fri, Aug 19, 2022 at 7:55 PM Amit Kapila wrote:
>
> On Fri, Aug 19, 2022 at 3:05 PM Peter Smith wrote:
> >
> > On Fri, Aug 19, 2022 at 7:10 PM Amit Kapila wrote:
> > >
> > > On Fri, Aug 19, 2022 at 2:36 PM Peter Smith wrote:
> > > >
> > > > Here are my review comments for the v23-0005 patch:
On Fri, Aug 19, 2022 at 3:05 PM Peter Smith wrote:
>
> On Fri, Aug 19, 2022 at 7:10 PM Amit Kapila wrote:
> >
> > On Fri, Aug 19, 2022 at 2:36 PM Peter Smith wrote:
> > >
> > > Here are my review comments for the v23-0005 patch:
> > >
> > > ==
> > >
> > > Commit Message says:
> > > main_work
On Fri, Aug 19, 2022 at 7:10 PM Amit Kapila wrote:
>
> On Fri, Aug 19, 2022 at 2:36 PM Peter Smith wrote:
> >
> > Here are my review comments for the v23-0005 patch:
> >
> > ==
> >
> > Commit Message says:
> > main_worker_pid is Process ID of the main apply worker, if this process is a
> > ap
On Fri, Aug 19, 2022 at 2:36 PM Peter Smith wrote:
>
> Here are my review comments for the v23-0005 patch:
>
> ==
>
> Commit Message says:
> main_worker_pid is Process ID of the main apply worker, if this process is a
> apply background worker. NULL if this process is a main apply worker or a
Here are my review comments for the v23-0005 patch:
==
Commit Message says:
main_worker_pid is Process ID of the main apply worker, if this process is a
apply background worker. NULL if this process is a main apply worker or a
synchronization worker.
The new column can make it easier to disti
On Thu, Aug 18, 2022 at 5:14 PM Amit Kapila wrote:
>
> On Wed, Aug 17, 2022 at 11:58 AM wangw.f...@fujitsu.com
> wrote:
> >
> > Attach the new patches.
> >
>
> Few comments on v23-0001
> ===
>
Some more comments on v23-0001
1.
static bool
handle_
Here are some review comments for the patch v23-0004:
==
4.1 src/test/subscription/t/032_streaming_apply.pl
This test file was introduced in patch 0003, but I think there are a
few changes in this 0004 patch which are really have nothing to do
with 0004 and should have been included in the o
Here are my review comments for the patch v23-0003:
==
3.1. src/backend/replication/logical/applybgworker.c -
apply_bgworker_relation_check
+ * Although the commit order is maintained by only allowing one process to
+ * commit at a time, the access order to the relation has changed. This cou
On Fri, Aug 19, 2022 at 4:36 AM Peter Smith wrote:
>
> Hi Wang-san,
>
> Here is some more information about my v21-0001 review [2] posted yesterday.
>
> ~~
>
> If the streaming=parallel will be disallowed for publishers not using
> protocol 4 (see Amit's post [1]), then please ignore all my previo
Hi Wang-san,
Here is some more information about my v21-0001 review [2] posted yesterday.
~~
If the streaming=parallel will be disallowed for publishers not using
protocol 4 (see Amit's post [1]), then please ignore all my previous
review comments about the protocol descriptions (see [2] comment
On Wed, Aug 17, 2022 at 11:58 AM wangw.f...@fujitsu.com
wrote:
>
> Attach the new patches.
>
Few comments on v23-0001
===
1.
+ /*
+ * Attach to the dynamic shared memory segment for the parallel query, and
+ * find its table of contents.
+ *
+ * Note: at this point, we have no
On Thu, Aug 18, 2022 at 3:40 PM Peter Smith wrote:
>
> On Thu, Aug 18, 2022 at 6:57 PM Amit Kapila wrote:
> >
> > > 47. src/include/replication/logicalproto.h
> > >
> > > @@ -32,12 +32,17 @@
> > > *
> > > * LOGICALREP_PROTO_TWOPHASE_VERSION_NUM is the minimum protocol version
> > > with
> >
On Thu, Aug 18, 2022 at 6:57 PM Amit Kapila wrote:
>
> On Thu, Aug 18, 2022 at 11:59 AM Peter Smith wrote:
> >
> > Here are my review comments for patch v21-0001:
> >
> > 4. Commit message
> >
> > In addition, the patch extends the logical replication STREAM_ABORT message
> > so
> > that abort_t
On Thu, Aug 18, 2022 at 11:59 AM Peter Smith wrote:
>
> Here are my review comments for patch v21-0001:
>
> 4. Commit message
>
> In addition, the patch extends the logical replication STREAM_ABORT message so
> that abort_time and abort_lsn can also be sent which can be used to update the
> replic
Hi Wang-san,
FYI, I also checked the latest patch v23-0001 but found that the
v21-0001/v23-0001 differences are minimal, so all my v21* review
comments are still applicable for the patch v23-0001.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
Here are my review comments for patch v21-0001:
Note - There are some "general" comments which will result in lots of
smaller changes. The subsequent "detailed" review comments have some
overlap with these general comments but I expect some will be missed
so please search/replace to fix all code r
On Wed, Aug 17, 2022 2:28 PM Wang, Wei/王 威 wrote:
>
> On Tues, August 16, 2022 15:33 PM I wrote:
> > Attach the new patches.
>
> I found that cfbot has a failure.
> After investigation, I think it is because the worker's exit state is not set
> correctly. So I made some slight modifications.
>
On Fri, August 12, 2022 17:22 PM Peter Smith wrote:
> Here are some review comments for v20-0004:
>
> (This completes my reviews of the v20* patch set. Sorry, the reviews
> are time consuming, so I am lagging slightly behind the latest posted
> version)
Thanks for your comments.
> 1. doc/src/sg
Here are some review comments for v20-0004:
(This completes my reviews of the v20* patch set. Sorry, the reviews
are time consuming, so I am lagging slightly behind the latest posted
version)
==
1. doc/src/sgml/ref/create_subscription.sgml
@@ -245,6 +245,11 @@ CREATE SUBSCRIPTION subscripti
Here are some review comments for v20-0003:
(Sorry - the reviews are time consuming, so I am lagging slightly
behind the latest posted version)
==
1.
1a.
There are a few comment modifications in this patch (e.g. changing
FROM "in an apply background worker" TO "using an apply background
wo
On Wednesday, August 10, 2022 5:40 PM Peter Smith wrote:
>
> Here are some review comments for the patch v20-0001:
> ==
>
> 1. doc/src/sgml/catalogs.sgml
>
> + p = apply changes directly using a background
> + worker, if available, otherwise, it behaves the same as 't'
>
> The
On Tuesday, August 9, 2022 4:49 PM Kuroda, Hayato/黒田 隼人
wrote:
> Dear Wang,
>
> Thanks for updating patch sets! Followings are comments about v20-0001.
>
> 1. config.sgml
>
> ```
>
> Specifies maximum number of logical replication workers. This includes
> both apply wor
301 - 400 of 488 matches
Mail list logo