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
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
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
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 =
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
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
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
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
> + *
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
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
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
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
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
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
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.
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
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
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 =
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
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, , , 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 >=
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
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)
> > +
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,
> +
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
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.
> > >
> >
> >
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?
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,
+
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
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)
> +
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
>
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:
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, , HASH_FIND, );
+ if (found)
+ {
+ /* If any workers (or
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.
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.
> > *
> > *
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
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
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
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
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.
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
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,
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
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
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
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
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
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
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 ==
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,
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
> > >
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
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
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
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
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
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
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:
> > >
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
> >
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
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
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
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
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
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]
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
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
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
>
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
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.
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
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
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
301 - 400 of 488 matches
Mail list logo
