Re: Perform streaming logical transactions by background workers and parallel apply
On Tue, May 9, 2023 at 7:50 AM Masahiko Sawada wrote: > > On Mon, May 8, 2023 at 8:09 PM Amit Kapila wrote: > > > > > > I think it is only possible for the leader apply can worker to try to > > receive the error message from an error queue after your 0002 patch. > > Because another place already detached from the queue before stopping > > the parallel apply workers. So, I combined both the patches and > > changed a few comments and a commit message. Let me know what you > > think of the attached. > > I have one comment on the detaching error queue part: > > + /* > +* Detach from the error_mq_handle for the parallel apply worker > before > +* stopping it. This prevents the leader apply worker from trying to > +* receive the message from the error queue that might already > be detached > +* by the parallel apply worker. > +*/ > + shm_mq_detach(winfo->error_mq_handle); > + winfo->error_mq_handle = NULL; > > In pa_detach_all_error_mq(), we try to detach error queues of all > workers in the pool. I think we should check if the queue is already > detached (i.e. is NULL) there. Otherwise, we will end up a SEGV if an > error happens after detaching the error queue and before removing the > worker from the pool. > Agreed, I have made this change, added the same check at one other place for the sake of consistency, and pushed the patch. -- With Regards, Amit Kapila.
Re: Perform streaming logical transactions by background workers and parallel apply
On Mon, May 8, 2023 at 8:09 PM Amit Kapila wrote: > > On Fri, May 5, 2023 at 9:14 AM Zhijie Hou (Fujitsu) > wrote: > > > > On Wednesday, May 3, 2023 3:17 PM Amit Kapila > > wrote: > > > > > > > Attach another patch to fix the problem that pa_shutdown will access invalid > > MyLogicalRepWorker. I personally want to avoid introducing new static > > variable, > > so I only reorder the callback registration in this version. > > > > When testing this, I notice a rare case that the leader is possible to > > receive > > the worker termination message after the leader stops the parallel worker. > > This > > is unnecessary and have a risk that the leader would try to access the > > detached > > memory queue. This is more likely to happen and sometimes cause the failure > > in > > regression tests after the registration reorder patch because the dsm is > > detached earlier after applying the patch. > > > > I think it is only possible for the leader apply can worker to try to > receive the error message from an error queue after your 0002 patch. > Because another place already detached from the queue before stopping > the parallel apply workers. So, I combined both the patches and > changed a few comments and a commit message. Let me know what you > think of the attached. I have one comment on the detaching error queue part: + /* +* Detach from the error_mq_handle for the parallel apply worker before +* stopping it. This prevents the leader apply worker from trying to +* receive the message from the error queue that might already be detached +* by the parallel apply worker. +*/ + shm_mq_detach(winfo->error_mq_handle); + winfo->error_mq_handle = NULL; In pa_detach_all_error_mq(), we try to detach error queues of all workers in the pool. I think we should check if the queue is already detached (i.e. is NULL) there. Otherwise, we will end up a SEGV if an error happens after detaching the error queue and before removing the worker from the pool. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Re: Perform streaming logical transactions by background workers and parallel apply
On Mon, May 8, 2023 at 3:34 PM Amit Kapila wrote:
>
> On Mon, May 8, 2023 at 11:08 AM Masahiko Sawada wrote:
> >
> > On Mon, May 8, 2023 at 12:52 PM Zhijie Hou (Fujitsu)
> > wrote:
> > >
> > > On Monday, May 8, 2023 11:08 AM Masahiko Sawada
> > >
> > > Hi,
> > >
> > > >
> > > > On Tue, May 2, 2023 at 12:22 PM Amit Kapila
> > > > wrote:
> > > > >
> > > > > On Fri, Apr 28, 2023 at 11:48 AM Masahiko Sawada
> > > > wrote:
> > > > > >
> > > > > > While investigating this issue, I've reviewed the code around
> > > > > > callbacks and worker termination etc and I found a problem.
> > > > > >
> > > > > > A parallel apply worker calls the before_shmem_exit callbacks in the
> > > > > > following order:
> > > > > >
> > > > > > 1. ShutdownPostgres()
> > > > > > 2. logicalrep_worker_onexit()
> > > > > > 3. pa_shutdown()
> > > > > >
> > > > > > Since the worker is detached during logicalrep_worker_onexit(),
> > > > > > MyLogicalReplication->leader_pid is an invalid when we call
> > > > > > pa_shutdown():
> > > > > >
> > > > > > static void
> > > > > > pa_shutdown(int code, Datum arg)
> > > > > > {
> > > > > > Assert(MyLogicalRepWorker->leader_pid != InvalidPid);
> > > > > > SendProcSignal(MyLogicalRepWorker->leader_pid,
> > > > > >PROCSIG_PARALLEL_APPLY_MESSAGE,
> > > > > >InvalidBackendId);
> > > > > >
> > > > > > Also, if the parallel apply worker fails shm_toc_lookup() during the
> > > > > > initialization, it raises an error (because of noError = false) but
> > > > > > ends up a SEGV as MyLogicalRepWorker is still NULL.
> > > > > >
> > > > > > I think that we should not use MyLogicalRepWorker->leader_pid in
> > > > > > pa_shutdown() but instead store the leader's pid to a static
> > > > > > variable
> > > > > > before registering pa_shutdown() callback.
> > > > > >
> > > > >
> > > > > Why not simply move the registration of pa_shutdown() to someplace
> > > > > after logicalrep_worker_attach()?
> > > >
> > > > If we do that, the worker won't call dsm_detach() if it raises an
> > > > ERROR in logicalrep_worker_attach(), is that okay? It seems that it's
> > > > no practically problem since we call dsm_backend_shutdown() in
> > > > shmem_exit(), but if so why do we need to call it in pa_shutdown()?
> > >
> > > I think the dsm_detach in pa_shutdown was intended to fire on_dsm_detach
> > > callbacks to give callback a chance to report stat before the stat system
> > > is
> > > shutdown, following what we do in ParallelWorkerShutdown() (e.g.
> > > sharedfileset.c callbacks cause fd.c to do ReportTemporaryFileUsage(), so
> > > we
> > > need to fire that earlier).
> > >
> > > But for parallel apply, we currently only have one on_dsm_detach
> > > callback(shm_mq_detach_callback) which doesn't report extra stats. So the
> > > dsm_detach in pa_shutdown is only used to make it a bit future-proof in
> > > case
> > > we add some other on_dsm_detach callbacks in the future which need to
> > > report
> > > stats.
> >
> > Make sense . Given that it's possible that we add other callbacks that
> > report stats in the future, I think it's better not to move the
> > position to register pa_shutdown() callback.
> >
>
> Hmm, what kind of stats do we expect to be collected before we
> register pa_shutdown? I think if required we can register such a
> callback after pa_shutdown. I feel without reordering the callbacks,
> the fix would be a bit complicated as explained in my previous email,
> so I don't think it is worth complicating this code unless really
> required.
Fair point. I agree that the issue can be resolved by carefully
ordering the callback registration.
Another thing I'm concerned about is that since both the leader worker
and parallel worker detach DSM before logicalrep_worker_onexit(),
cleaning up work that touches DSM cannot be done in
logicalrep_worker_onexit(). If we need to do something in the future,
we would need to have another callback called before detaching DSM.
But I'm fine as it's not a problem for now.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Re: Perform streaming logical transactions by background workers and parallel apply
On Fri, May 5, 2023 at 9:14 AM Zhijie Hou (Fujitsu) wrote: > > On Wednesday, May 3, 2023 3:17 PM Amit Kapila wrote: > > > > Attach another patch to fix the problem that pa_shutdown will access invalid > MyLogicalRepWorker. I personally want to avoid introducing new static > variable, > so I only reorder the callback registration in this version. > > When testing this, I notice a rare case that the leader is possible to receive > the worker termination message after the leader stops the parallel worker. > This > is unnecessary and have a risk that the leader would try to access the > detached > memory queue. This is more likely to happen and sometimes cause the failure in > regression tests after the registration reorder patch because the dsm is > detached earlier after applying the patch. > I think it is only possible for the leader apply can worker to try to receive the error message from an error queue after your 0002 patch. Because another place already detached from the queue before stopping the parallel apply workers. So, I combined both the patches and changed a few comments and a commit message. Let me know what you think of the attached. -- With Regards, Amit Kapila. v2-0001-Fix-invalid-memory-access-during-the-shutdown-of-.patch Description: Binary data
Re: Perform streaming logical transactions by background workers and parallel apply
On Mon, May 8, 2023 at 11:08 AM Masahiko Sawada wrote:
>
> On Mon, May 8, 2023 at 12:52 PM Zhijie Hou (Fujitsu)
> wrote:
> >
> > On Monday, May 8, 2023 11:08 AM Masahiko Sawada
> >
> > Hi,
> >
> > >
> > > On Tue, May 2, 2023 at 12:22 PM Amit Kapila
> > > wrote:
> > > >
> > > > On Fri, Apr 28, 2023 at 11:48 AM Masahiko Sawada
> > > wrote:
> > > > >
> > > > > While investigating this issue, I've reviewed the code around
> > > > > callbacks and worker termination etc and I found a problem.
> > > > >
> > > > > A parallel apply worker calls the before_shmem_exit callbacks in the
> > > > > following order:
> > > > >
> > > > > 1. ShutdownPostgres()
> > > > > 2. logicalrep_worker_onexit()
> > > > > 3. pa_shutdown()
> > > > >
> > > > > Since the worker is detached during logicalrep_worker_onexit(),
> > > > > MyLogicalReplication->leader_pid is an invalid when we call
> > > > > pa_shutdown():
> > > > >
> > > > > static void
> > > > > pa_shutdown(int code, Datum arg)
> > > > > {
> > > > > Assert(MyLogicalRepWorker->leader_pid != InvalidPid);
> > > > > SendProcSignal(MyLogicalRepWorker->leader_pid,
> > > > >PROCSIG_PARALLEL_APPLY_MESSAGE,
> > > > >InvalidBackendId);
> > > > >
> > > > > Also, if the parallel apply worker fails shm_toc_lookup() during the
> > > > > initialization, it raises an error (because of noError = false) but
> > > > > ends up a SEGV as MyLogicalRepWorker is still NULL.
> > > > >
> > > > > I think that we should not use MyLogicalRepWorker->leader_pid in
> > > > > pa_shutdown() but instead store the leader's pid to a static variable
> > > > > before registering pa_shutdown() callback.
> > > > >
> > > >
> > > > Why not simply move the registration of pa_shutdown() to someplace
> > > > after logicalrep_worker_attach()?
> > >
> > > If we do that, the worker won't call dsm_detach() if it raises an
> > > ERROR in logicalrep_worker_attach(), is that okay? It seems that it's
> > > no practically problem since we call dsm_backend_shutdown() in
> > > shmem_exit(), but if so why do we need to call it in pa_shutdown()?
> >
> > I think the dsm_detach in pa_shutdown was intended to fire on_dsm_detach
> > callbacks to give callback a chance to report stat before the stat system is
> > shutdown, following what we do in ParallelWorkerShutdown() (e.g.
> > sharedfileset.c callbacks cause fd.c to do ReportTemporaryFileUsage(), so we
> > need to fire that earlier).
> >
> > But for parallel apply, we currently only have one on_dsm_detach
> > callback(shm_mq_detach_callback) which doesn't report extra stats. So the
> > dsm_detach in pa_shutdown is only used to make it a bit future-proof in case
> > we add some other on_dsm_detach callbacks in the future which need to report
> > stats.
>
> Make sense . Given that it's possible that we add other callbacks that
> report stats in the future, I think it's better not to move the
> position to register pa_shutdown() callback.
>
Hmm, what kind of stats do we expect to be collected before we
register pa_shutdown? I think if required we can register such a
callback after pa_shutdown. I feel without reordering the callbacks,
the fix would be a bit complicated as explained in my previous email,
so I don't think it is worth complicating this code unless really
required.
--
With Regards,
Amit Kapila.
Re: Perform streaming logical transactions by background workers and parallel apply
On Mon, May 8, 2023 at 12:52 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Monday, May 8, 2023 11:08 AM Masahiko Sawada
>
> Hi,
>
> >
> > On Tue, May 2, 2023 at 12:22 PM Amit Kapila
> > wrote:
> > >
> > > On Fri, Apr 28, 2023 at 11:48 AM Masahiko Sawada
> > wrote:
> > > >
> > > > While investigating this issue, I've reviewed the code around
> > > > callbacks and worker termination etc and I found a problem.
> > > >
> > > > A parallel apply worker calls the before_shmem_exit callbacks in the
> > > > following order:
> > > >
> > > > 1. ShutdownPostgres()
> > > > 2. logicalrep_worker_onexit()
> > > > 3. pa_shutdown()
> > > >
> > > > Since the worker is detached during logicalrep_worker_onexit(),
> > > > MyLogicalReplication->leader_pid is an invalid when we call
> > > > pa_shutdown():
> > > >
> > > > static void
> > > > pa_shutdown(int code, Datum arg)
> > > > {
> > > > Assert(MyLogicalRepWorker->leader_pid != InvalidPid);
> > > > SendProcSignal(MyLogicalRepWorker->leader_pid,
> > > >PROCSIG_PARALLEL_APPLY_MESSAGE,
> > > >InvalidBackendId);
> > > >
> > > > Also, if the parallel apply worker fails shm_toc_lookup() during the
> > > > initialization, it raises an error (because of noError = false) but
> > > > ends up a SEGV as MyLogicalRepWorker is still NULL.
> > > >
> > > > I think that we should not use MyLogicalRepWorker->leader_pid in
> > > > pa_shutdown() but instead store the leader's pid to a static variable
> > > > before registering pa_shutdown() callback.
> > > >
> > >
> > > Why not simply move the registration of pa_shutdown() to someplace
> > > after logicalrep_worker_attach()?
> >
> > If we do that, the worker won't call dsm_detach() if it raises an
> > ERROR in logicalrep_worker_attach(), is that okay? It seems that it's
> > no practically problem since we call dsm_backend_shutdown() in
> > shmem_exit(), but if so why do we need to call it in pa_shutdown()?
>
> I think the dsm_detach in pa_shutdown was intended to fire on_dsm_detach
> callbacks to give callback a chance to report stat before the stat system is
> shutdown, following what we do in ParallelWorkerShutdown() (e.g.
> sharedfileset.c callbacks cause fd.c to do ReportTemporaryFileUsage(), so we
> need to fire that earlier).
>
> But for parallel apply, we currently only have one on_dsm_detach
> callback(shm_mq_detach_callback) which doesn't report extra stats. So the
> dsm_detach in pa_shutdown is only used to make it a bit future-proof in case
> we add some other on_dsm_detach callbacks in the future which need to report
> stats.
Make sense . Given that it's possible that we add other callbacks that
report stats in the future, I think it's better not to move the
position to register pa_shutdown() callback.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
RE: Perform streaming logical transactions by background workers and parallel apply
On Monday, May 8, 2023 11:08 AM Masahiko Sawada
Hi,
>
> On Tue, May 2, 2023 at 12:22 PM Amit Kapila
> wrote:
> >
> > On Fri, Apr 28, 2023 at 11:48 AM Masahiko Sawada
> wrote:
> > >
> > > While investigating this issue, I've reviewed the code around
> > > callbacks and worker termination etc and I found a problem.
> > >
> > > A parallel apply worker calls the before_shmem_exit callbacks in the
> > > following order:
> > >
> > > 1. ShutdownPostgres()
> > > 2. logicalrep_worker_onexit()
> > > 3. pa_shutdown()
> > >
> > > Since the worker is detached during logicalrep_worker_onexit(),
> > > MyLogicalReplication->leader_pid is an invalid when we call
> > > pa_shutdown():
> > >
> > > static void
> > > pa_shutdown(int code, Datum arg)
> > > {
> > > Assert(MyLogicalRepWorker->leader_pid != InvalidPid);
> > > SendProcSignal(MyLogicalRepWorker->leader_pid,
> > >PROCSIG_PARALLEL_APPLY_MESSAGE,
> > >InvalidBackendId);
> > >
> > > Also, if the parallel apply worker fails shm_toc_lookup() during the
> > > initialization, it raises an error (because of noError = false) but
> > > ends up a SEGV as MyLogicalRepWorker is still NULL.
> > >
> > > I think that we should not use MyLogicalRepWorker->leader_pid in
> > > pa_shutdown() but instead store the leader's pid to a static variable
> > > before registering pa_shutdown() callback.
> > >
> >
> > Why not simply move the registration of pa_shutdown() to someplace
> > after logicalrep_worker_attach()?
>
> If we do that, the worker won't call dsm_detach() if it raises an
> ERROR in logicalrep_worker_attach(), is that okay? It seems that it's
> no practically problem since we call dsm_backend_shutdown() in
> shmem_exit(), but if so why do we need to call it in pa_shutdown()?
I think the dsm_detach in pa_shutdown was intended to fire on_dsm_detach
callbacks to give callback a chance to report stat before the stat system is
shutdown, following what we do in ParallelWorkerShutdown() (e.g.
sharedfileset.c callbacks cause fd.c to do ReportTemporaryFileUsage(), so we
need to fire that earlier).
But for parallel apply, we currently only have one on_dsm_detach
callback(shm_mq_detach_callback) which doesn't report extra stats. So the
dsm_detach in pa_shutdown is only used to make it a bit future-proof in case
we add some other on_dsm_detach callbacks in the future which need to report
stats.
Best regards,
Hou zj
Re: Perform streaming logical transactions by background workers and parallel apply
On Tue, May 2, 2023 at 12:22 PM Amit Kapila wrote:
>
> On Fri, Apr 28, 2023 at 11:48 AM Masahiko Sawada
> wrote:
> >
> > While investigating this issue, I've reviewed the code around
> > callbacks and worker termination etc and I found a problem.
> >
> > A parallel apply worker calls the before_shmem_exit callbacks in the
> > following order:
> >
> > 1. ShutdownPostgres()
> > 2. logicalrep_worker_onexit()
> > 3. pa_shutdown()
> >
> > Since the worker is detached during logicalrep_worker_onexit(),
> > MyLogicalReplication->leader_pid is an invalid when we call
> > pa_shutdown():
> >
> > static void
> > pa_shutdown(int code, Datum arg)
> > {
> > Assert(MyLogicalRepWorker->leader_pid != InvalidPid);
> > SendProcSignal(MyLogicalRepWorker->leader_pid,
> >PROCSIG_PARALLEL_APPLY_MESSAGE,
> >InvalidBackendId);
> >
> > Also, if the parallel apply worker fails shm_toc_lookup() during the
> > initialization, it raises an error (because of noError = false) but
> > ends up a SEGV as MyLogicalRepWorker is still NULL.
> >
> > I think that we should not use MyLogicalRepWorker->leader_pid in
> > pa_shutdown() but instead store the leader's pid to a static variable
> > before registering pa_shutdown() callback.
> >
>
> Why not simply move the registration of pa_shutdown() to someplace
> after logicalrep_worker_attach()?
If we do that, the worker won't call dsm_detach() if it raises an
ERROR in logicalrep_worker_attach(), is that okay? It seems that it's
no practically problem since we call dsm_backend_shutdown() in
shmem_exit(), but if so why do we need to call it in pa_shutdown()?
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
RE: Perform streaming logical transactions by background workers and parallel apply
On Wednesday, May 3, 2023 3:17 PM Amit Kapila wrote: > > On Tue, May 2, 2023 at 9:46 AM Amit Kapila > wrote: > > > > On Tue, May 2, 2023 at 9:06 AM Zhijie Hou (Fujitsu) > > wrote: > > > > > > On Friday, April 28, 2023 2:18 PM Masahiko Sawada > wrote: > > > > > > > > > > > > > > Alexander, does the proposed patch fix the problem you are facing? > > > > > Sawada-San, and others, do you see any better way to fix it than > > > > > what has been proposed? > > > > > > > > I'm concerned that the idea of relying on IsNormalProcessingMode() > > > > might not be robust since if we change the meaning of > > > > IsNormalProcessingMode() some day it would silently break again. > > > > So I prefer using something like InitializingApplyWorker, or > > > > another idea would be to do cleanup work (e.g., fileset deletion > > > > and lock release) in a separate callback that is registered after > > > > connecting to the database. > > > > > > Thanks for the review. I agree that it’s better to use a new variable > > > here. > > > Attach the patch for the same. > > > > > > > + * > > + * However, if the worker is being initialized, there is no need to > > + release > > + * locks. > > */ > > - LockReleaseAll(DEFAULT_LOCKMETHOD, true); > > + if (!InitializingApplyWorker) > > + LockReleaseAll(DEFAULT_LOCKMETHOD, true); > > > > Can we slightly reword this comment as: "The locks will be acquired > > once the worker is initialized."? > > > > After making this modification, I pushed your patch. Thanks! Thanks for pushing. Attach another patch to fix the problem that pa_shutdown will access invalid MyLogicalRepWorker. I personally want to avoid introducing new static variable, so I only reorder the callback registration in this version. When testing this, I notice a rare case that the leader is possible to receive the worker termination message after the leader stops the parallel worker. This is unnecessary and have a risk that the leader would try to access the detached memory queue. This is more likely to happen and sometimes cause the failure in regression tests after the registration reorder patch because the dsm is detached earlier after applying the patch. So, put the patch that detach the error queue before stopping worker as 0001 and the registration reorder patch as 0002. Best Regards, Hou zj 0002-adjust-the-order-of-callback-registration-to-avoid-a.patch Description: 0002-adjust-the-order-of-callback-registration-to-avoid-a.patch 0001-Detach-the-error-queue-before-stopping-parallel-appl.patch Description: 0001-Detach-the-error-queue-before-stopping-parallel-appl.patch
Re: Perform streaming logical transactions by background workers and parallel apply
On Tue, May 2, 2023 at 9:46 AM Amit Kapila wrote: > > On Tue, May 2, 2023 at 9:06 AM Zhijie Hou (Fujitsu) > wrote: > > > > On Friday, April 28, 2023 2:18 PM Masahiko Sawada > > wrote: > > > > > > > > > > > Alexander, does the proposed patch fix the problem you are facing? > > > > Sawada-San, and others, do you see any better way to fix it than what > > > > has been proposed? > > > > > > I'm concerned that the idea of relying on IsNormalProcessingMode() > > > might not be robust since if we change the meaning of > > > IsNormalProcessingMode() some day it would silently break again. So I > > > prefer using something like InitializingApplyWorker, or another idea > > > would be to do cleanup work (e.g., fileset deletion and lock release) > > > in a separate callback that is registered after connecting to the > > > database. > > > > Thanks for the review. I agree that it’s better to use a new variable here. > > Attach the patch for the same. > > > > + * > + * However, if the worker is being initialized, there is no need to release > + * locks. > */ > - LockReleaseAll(DEFAULT_LOCKMETHOD, true); > + if (!InitializingApplyWorker) > + LockReleaseAll(DEFAULT_LOCKMETHOD, true); > > Can we slightly reword this comment as: "The locks will be acquired > once the worker is initialized."? > After making this modification, I pushed your patch. Thanks! -- With Regards, Amit Kapila.
Re: Perform streaming logical transactions by background workers and parallel apply
On Tue, May 2, 2023 at 9:06 AM Zhijie Hou (Fujitsu) wrote: > > On Friday, April 28, 2023 2:18 PM Masahiko Sawada > wrote: > > > > > > > > Alexander, does the proposed patch fix the problem you are facing? > > > Sawada-San, and others, do you see any better way to fix it than what > > > has been proposed? > > > > I'm concerned that the idea of relying on IsNormalProcessingMode() > > might not be robust since if we change the meaning of > > IsNormalProcessingMode() some day it would silently break again. So I > > prefer using something like InitializingApplyWorker, or another idea > > would be to do cleanup work (e.g., fileset deletion and lock release) > > in a separate callback that is registered after connecting to the > > database. > > Thanks for the review. I agree that it’s better to use a new variable here. > Attach the patch for the same. > + * + * However, if the worker is being initialized, there is no need to release + * locks. */ - LockReleaseAll(DEFAULT_LOCKMETHOD, true); + if (!InitializingApplyWorker) + LockReleaseAll(DEFAULT_LOCKMETHOD, true); Can we slightly reword this comment as: "The locks will be acquired once the worker is initialized."? -- With Regards, Amit Kapila.
RE: Perform streaming logical transactions by background workers and parallel apply
On Friday, April 28, 2023 2:18 PM Masahiko Sawada wrote:
>
> On Fri, Apr 28, 2023 at 11:51 AM Amit Kapila wrote:
> >
> > On Wed, Apr 26, 2023 at 4:11 PM Zhijie Hou (Fujitsu)
> > wrote:
> > >
> > > On Wednesday, April 26, 2023 5:00 PM Alexander Lakhin
> wrote:
> > > >
> > > > IIUC, that assert will fail in case of any error raised between
> > > >
> ApplyWorkerMain()->logicalrep_worker_attach()->before_shmem_exit() and
> > > >
> ApplyWorkerMain()->InitializeApplyWorker()->BackgroundWorkerInitializeC
> > > > onnectionByOid()->InitPostgres().
> > >
> > > Thanks for reporting the issue.
> > >
> > > I think the problem is that it tried to release locks in
> > > logicalrep_worker_onexit() before the initialization of the process is
> complete
> > > because this callback function was registered before the init phase. So I
> think we
> > > can add a conditional statement before releasing locks. Please find an
> attached
> > > patch.
> > >
> >
> > Alexander, does the proposed patch fix the problem you are facing?
> > Sawada-San, and others, do you see any better way to fix it than what
> > has been proposed?
>
> I'm concerned that the idea of relying on IsNormalProcessingMode()
> might not be robust since if we change the meaning of
> IsNormalProcessingMode() some day it would silently break again. So I
> prefer using something like InitializingApplyWorker, or another idea
> would be to do cleanup work (e.g., fileset deletion and lock release)
> in a separate callback that is registered after connecting to the
> database.
Thanks for the review. I agree that it’s better to use a new variable here.
Attach the patch for the same.
>
> FWIW, we might need to be careful about the timing when we call
> logicalrep_worker_detach() in the worker's termination process. Since
> we rely on IsLogicalParallelApplyWorker() for the parallel apply
> worker to send ERROR messages to the leader apply worker, if an ERROR
> happens after logicalrep_worker_detach(), we will end up with the
> assertion failure.
>
> if (IsLogicalParallelApplyWorker())
> SendProcSignal(pq_mq_parallel_leader_pid,
>PROCSIG_PARALLEL_APPLY_MESSAGE,
>pq_mq_parallel_leader_backend_id);
> else
> {
> Assert(IsParallelWorker());
>
> It normally would be a should-no-happen case, though.
Yes, I think currently PA sends ERROR message before exiting,
so the callback functions are always fired after the above code which
looks fine to me.
Best Regards,
Hou zj
v2-0001-Fix-assert-failure-in-logical-replication-apply-w.patch
Description: v2-0001-Fix-assert-failure-in-logical-replication-apply-w.patch
Re: Perform streaming logical transactions by background workers and parallel apply
On Fri, Apr 28, 2023 at 11:48 AM Masahiko Sawada wrote:
>
> While investigating this issue, I've reviewed the code around
> callbacks and worker termination etc and I found a problem.
>
> A parallel apply worker calls the before_shmem_exit callbacks in the
> following order:
>
> 1. ShutdownPostgres()
> 2. logicalrep_worker_onexit()
> 3. pa_shutdown()
>
> Since the worker is detached during logicalrep_worker_onexit(),
> MyLogicalReplication->leader_pid is an invalid when we call
> pa_shutdown():
>
> static void
> pa_shutdown(int code, Datum arg)
> {
> Assert(MyLogicalRepWorker->leader_pid != InvalidPid);
> SendProcSignal(MyLogicalRepWorker->leader_pid,
>PROCSIG_PARALLEL_APPLY_MESSAGE,
>InvalidBackendId);
>
> Also, if the parallel apply worker fails shm_toc_lookup() during the
> initialization, it raises an error (because of noError = false) but
> ends up a SEGV as MyLogicalRepWorker is still NULL.
>
> I think that we should not use MyLogicalRepWorker->leader_pid in
> pa_shutdown() but instead store the leader's pid to a static variable
> before registering pa_shutdown() callback.
>
Why not simply move the registration of pa_shutdown() to someplace
after logicalrep_worker_attach()? BTW, it seems we don't have access
to MyLogicalRepWorker->leader_pid till we attach to the worker slot
via logicalrep_worker_attach(), so we anyway need to do what you are
suggesting after attaching to the worker slot.
--
With Regards,
Amit Kapila.
Re: Perform streaming logical transactions by background workers and parallel apply
On Fri, Apr 28, 2023 at 6:01 PM Amit Kapila wrote: > > On Fri, Apr 28, 2023 at 11:48 AM Masahiko Sawada > wrote: > > > > On Fri, Apr 28, 2023 at 11:51 AM Amit Kapila > > wrote: > > > > > > On Wed, Apr 26, 2023 at 4:11 PM Zhijie Hou (Fujitsu) > > > wrote: > > > > > > > > On Wednesday, April 26, 2023 5:00 PM Alexander Lakhin > > > > wrote: > > > > > > > > > > IIUC, that assert will fail in case of any error raised between > > > > > ApplyWorkerMain()->logicalrep_worker_attach()->before_shmem_exit() and > > > > > ApplyWorkerMain()->InitializeApplyWorker()->BackgroundWorkerInitializeC > > > > > onnectionByOid()->InitPostgres(). > > > > > > > > Thanks for reporting the issue. > > > > > > > > I think the problem is that it tried to release locks in > > > > logicalrep_worker_onexit() before the initialization of the process is > > > > complete > > > > because this callback function was registered before the init phase. So > > > > I think we > > > > can add a conditional statement before releasing locks. Please find an > > > > attached > > > > patch. > > > > > > > > > > Alexander, does the proposed patch fix the problem you are facing? > > > Sawada-San, and others, do you see any better way to fix it than what > > > has been proposed? > > > > I'm concerned that the idea of relying on IsNormalProcessingMode() > > might not be robust since if we change the meaning of > > IsNormalProcessingMode() some day it would silently break again. So I > > prefer using something like InitializingApplyWorker, > > > > I think if we change the meaning of IsNormalProcessingMode() then it > could also break the other places the similar check is being used. Right, but I think it's unclear the relationship between the processing modes and releasing session locks. If non-normal-processing mode means we're still in the process initialization phase, why we don't skip other cleanup works such as walrcv_disconnect() and FileSetDeleteAll()? > However, I am fine with InitializingApplyWorker as that could be used > at other places as well. I just want to avoid adding another variable > by using IsNormalProcessingMode. I think it's less confusing. > > > or another idea > > would be to do cleanup work (e.g., fileset deletion and lock release) > > in a separate callback that is registered after connecting to the > > database. > > > > Yeah, but not sure if it's worth having multiple callbacks for cleanup work. Fair point. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Re: Perform streaming logical transactions by background workers and parallel apply
On Fri, Apr 28, 2023 at 11:48 AM Masahiko Sawada wrote: > > On Fri, Apr 28, 2023 at 11:51 AM Amit Kapila wrote: > > > > On Wed, Apr 26, 2023 at 4:11 PM Zhijie Hou (Fujitsu) > > wrote: > > > > > > On Wednesday, April 26, 2023 5:00 PM Alexander Lakhin > > > wrote: > > > > > > > > IIUC, that assert will fail in case of any error raised between > > > > ApplyWorkerMain()->logicalrep_worker_attach()->before_shmem_exit() and > > > > ApplyWorkerMain()->InitializeApplyWorker()->BackgroundWorkerInitializeC > > > > onnectionByOid()->InitPostgres(). > > > > > > Thanks for reporting the issue. > > > > > > I think the problem is that it tried to release locks in > > > logicalrep_worker_onexit() before the initialization of the process is > > > complete > > > because this callback function was registered before the init phase. So I > > > think we > > > can add a conditional statement before releasing locks. Please find an > > > attached > > > patch. > > > > > > > Alexander, does the proposed patch fix the problem you are facing? > > Sawada-San, and others, do you see any better way to fix it than what > > has been proposed? > > I'm concerned that the idea of relying on IsNormalProcessingMode() > might not be robust since if we change the meaning of > IsNormalProcessingMode() some day it would silently break again. So I > prefer using something like InitializingApplyWorker, > I think if we change the meaning of IsNormalProcessingMode() then it could also break the other places the similar check is being used. However, I am fine with InitializingApplyWorker as that could be used at other places as well. I just want to avoid adding another variable by using IsNormalProcessingMode. > or another idea > would be to do cleanup work (e.g., fileset deletion and lock release) > in a separate callback that is registered after connecting to the > database. > Yeah, but not sure if it's worth having multiple callbacks for cleanup work. -- With Regards, Amit Kapila.
Re: Perform streaming logical transactions by background workers and parallel apply
On Fri, Apr 28, 2023 at 11:51 AM Amit Kapila wrote:
>
> On Wed, Apr 26, 2023 at 4:11 PM Zhijie Hou (Fujitsu)
> wrote:
> >
> > On Wednesday, April 26, 2023 5:00 PM Alexander Lakhin
> > wrote:
> > >
> > > IIUC, that assert will fail in case of any error raised between
> > > ApplyWorkerMain()->logicalrep_worker_attach()->before_shmem_exit() and
> > > ApplyWorkerMain()->InitializeApplyWorker()->BackgroundWorkerInitializeC
> > > onnectionByOid()->InitPostgres().
> >
> > Thanks for reporting the issue.
> >
> > I think the problem is that it tried to release locks in
> > logicalrep_worker_onexit() before the initialization of the process is
> > complete
> > because this callback function was registered before the init phase. So I
> > think we
> > can add a conditional statement before releasing locks. Please find an
> > attached
> > patch.
> >
>
> Alexander, does the proposed patch fix the problem you are facing?
> Sawada-San, and others, do you see any better way to fix it than what
> has been proposed?
I'm concerned that the idea of relying on IsNormalProcessingMode()
might not be robust since if we change the meaning of
IsNormalProcessingMode() some day it would silently break again. So I
prefer using something like InitializingApplyWorker, or another idea
would be to do cleanup work (e.g., fileset deletion and lock release)
in a separate callback that is registered after connecting to the
database.
While investigating this issue, I've reviewed the code around
callbacks and worker termination etc and I found a problem.
A parallel apply worker calls the before_shmem_exit callbacks in the
following order:
1. ShutdownPostgres()
2. logicalrep_worker_onexit()
3. pa_shutdown()
Since the worker is detached during logicalrep_worker_onexit(),
MyLogicalReplication->leader_pid is an invalid when we call
pa_shutdown():
static void
pa_shutdown(int code, Datum arg)
{
Assert(MyLogicalRepWorker->leader_pid != InvalidPid);
SendProcSignal(MyLogicalRepWorker->leader_pid,
PROCSIG_PARALLEL_APPLY_MESSAGE,
InvalidBackendId);
Also, if the parallel apply worker fails shm_toc_lookup() during the
initialization, it raises an error (because of noError = false) but
ends up a SEGV as MyLogicalRepWorker is still NULL.
I think that we should not use MyLogicalRepWorker->leader_pid in
pa_shutdown() but instead store the leader's pid to a static variable
before registering pa_shutdown() callback. And probably we can
remember the backend id of the leader apply worker to speed up
SendProcSignal().
FWIW, we might need to be careful about the timing when we call
logicalrep_worker_detach() in the worker's termination process. Since
we rely on IsLogicalParallelApplyWorker() for the parallel apply
worker to send ERROR messages to the leader apply worker, if an ERROR
happens after logicalrep_worker_detach(), we will end up with the
assertion failure.
if (IsLogicalParallelApplyWorker())
SendProcSignal(pq_mq_parallel_leader_pid,
PROCSIG_PARALLEL_APPLY_MESSAGE,
pq_mq_parallel_leader_backend_id);
else
{
Assert(IsParallelWorker());
It normally would be a should-no-happen case, though.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
Re: Perform streaming logical transactions by background workers and parallel apply
Hello Amit and Zhijie, 28.04.2023 05:51, Amit Kapila wrote: On Wed, Apr 26, 2023 at 4:11 PM Zhijie Hou (Fujitsu) wrote: I think the problem is that it tried to release locks in logicalrep_worker_onexit() before the initialization of the process is complete because this callback function was registered before the init phase. So I think we can add a conditional statement before releasing locks. Please find an attached patch. Alexander, does the proposed patch fix the problem you are facing? Sawada-San, and others, do you see any better way to fix it than what has been proposed? Yes, the patch definitely fixes it. Maybe some other onexit actions can be skipped in the non-normal mode, but the assert-triggering LockReleaseAll() not called now. Thank you! Best regards, Alexander
Re: Perform streaming logical transactions by background workers and parallel apply
On Wed, Apr 26, 2023 at 4:11 PM Zhijie Hou (Fujitsu) wrote: > > On Wednesday, April 26, 2023 5:00 PM Alexander Lakhin > wrote: > > > > IIUC, that assert will fail in case of any error raised between > > ApplyWorkerMain()->logicalrep_worker_attach()->before_shmem_exit() and > > ApplyWorkerMain()->InitializeApplyWorker()->BackgroundWorkerInitializeC > > onnectionByOid()->InitPostgres(). > > Thanks for reporting the issue. > > I think the problem is that it tried to release locks in > logicalrep_worker_onexit() before the initialization of the process is > complete > because this callback function was registered before the init phase. So I > think we > can add a conditional statement before releasing locks. Please find an > attached > patch. > Alexander, does the proposed patch fix the problem you are facing? Sawada-San, and others, do you see any better way to fix it than what has been proposed? -- With Regards, Amit Kapila.
Re: Perform streaming logical transactions by background workers and parallel apply
On Wed, Apr 26, 2023 at 4:11 PM Zhijie Hou (Fujitsu) wrote: > > On Wednesday, April 26, 2023 5:00 PM Alexander Lakhin > wrote: > > Thanks for reporting the issue. > > I think the problem is that it tried to release locks in > logicalrep_worker_onexit() before the initialization of the process is > complete > because this callback function was registered before the init phase. So I > think we > can add a conditional statement before releasing locks. Please find an > attached > patch. > Yeah, this should work. Yet another possibility is to introduce a new variable 'InitializingApplyWorker' similar to 'InitializingParallelWorker' and use that to prevent releasing locks. -- With Regards, Amit Kapila.
RE: Perform streaming logical transactions by background workers and parallel apply
On Wednesday, April 26, 2023 5:00 PM Alexander Lakhin
wrote:
> Please look at a new anomaly that can be observed starting from 216a7848.
>
> The following script:
> echo "CREATE SUBSCRIPTION testsub CONNECTION 'dbname=nodb'
> PUBLICATION testpub WITH (connect = false);
> ALTER SUBSCRIPTION testsub ENABLE;" | psql
>
> sleep 1
> rm $PGINST/lib/libpqwalreceiver.so
> sleep 15
> pg_ctl -D "$PGDB" stop -m immediate
> grep 'TRAP:' server.log
>
> Leads to multiple assertion failures:
> CREATE SUBSCRIPTION
> ALTER SUBSCRIPTION
> waiting for server to shut down done
> server stopped
> TRAP: failed Assert("MyProc->backendId != InvalidBackendId"), File: "lock.c",
> Line: 4439, PID: 2899323
> TRAP: failed Assert("MyProc->backendId != InvalidBackendId"), File: "lock.c",
> Line: 4439, PID: 2899416
> TRAP: failed Assert("MyProc->backendId != InvalidBackendId"), File: "lock.c",
> Line: 4439, PID: 2899427
> TRAP: failed Assert("MyProc->backendId != InvalidBackendId"), File: "lock.c",
> Line: 4439, PID: 2899439
> TRAP: failed Assert("MyProc->backendId != InvalidBackendId"), File: "lock.c",
> Line: 4439, PID: 2899538
> TRAP: failed Assert("MyProc->backendId != InvalidBackendId"), File: "lock.c",
> Line: 4439, PID: 2899547
>
> server.log contains:
> 2023-04-26 11:00:58.797 MSK [2899300] LOG: database system is ready to
> accept connections
> 2023-04-26 11:00:58.821 MSK [2899416] ERROR: could not access file
> "libpqwalreceiver": No such file or directory
> TRAP: failed Assert("MyProc->backendId != InvalidBackendId"), File: "lock.c",
> Line: 4439, PID: 2899416
> postgres: logical replication apply worker for subscription 16385
> (ExceptionalCondition+0x69)[0x558b2ac06d41]
> postgres: logical replication apply worker for subscription 16385
> (VirtualXactLockTableCleanup+0xa4)[0x558b2aa9fd74]
> postgres: logical replication apply worker for subscription 16385
> (LockReleaseAll+0xbb)[0x558b2aa9fe7d]
> postgres: logical replication apply worker for subscription 16385
> (+0x4588c6)[0x558b2aa2a8c6]
> postgres: logical replication apply worker for subscription 16385
> (shmem_exit+0x6c)[0x558b2aa87eb1]
> postgres: logical replication apply worker for subscription 16385
> (+0x4b5faa)[0x558b2aa87faa]
> postgres: logical replication apply worker for subscription 16385
> (proc_exit+0xc)[0x558b2aa88031]
> postgres: logical replication apply worker for subscription 16385
> (StartBackgroundWorker+0x147)[0x558b2aa0b4d9]
> postgres: logical replication apply worker for subscription 16385
> (+0x43fdc1)[0x558b2aa11dc1]
> postgres: logical replication apply worker for subscription 16385
> (+0x43ff3d)[0x558b2aa11f3d]
> postgres: logical replication apply worker for subscription 16385
> (+0x440866)[0x558b2aa12866]
> postgres: logical replication apply worker for subscription 16385
> (+0x440e12)[0x558b2aa12e12]
> postgres: logical replication apply worker for subscription 16385
> (BackgroundWorkerInitializeConnection+0x0)[0x558b2aa14396]
> postgres: logical replication apply worker for subscription 16385
> (main+0x21a)[0x558b2a932e21]
>
> I understand, that removing libpqwalreceiver.so (or whole pginst/) is not
> what happens in a production environment every day, but nonetheless it's a
> new failure mode and it can produce many coredumps when testing.
>
> IIUC, that assert will fail in case of any error raised between
> ApplyWorkerMain()->logicalrep_worker_attach()->before_shmem_exit() and
> ApplyWorkerMain()->InitializeApplyWorker()->BackgroundWorkerInitializeC
> onnectionByOid()->InitPostgres().
Thanks for reporting the issue.
I think the problem is that it tried to release locks in
logicalrep_worker_onexit() before the initialization of the process is complete
because this callback function was registered before the init phase. So I think
we
can add a conditional statement before releasing locks. Please find an attached
patch.
Best Regards,
Hou zj
0001-fix-assert-failure-in-logical-replication-apply-work.patch
Description: 0001-fix-assert-failure-in-logical-replication-apply-work.patch
Re: Perform streaming logical transactions by background workers and parallel apply
Hello hackers,
Please look at a new anomaly that can be observed starting from 216a7848.
The following script:
echo "CREATE SUBSCRIPTION testsub CONNECTION 'dbname=nodb' PUBLICATION testpub
WITH (connect = false);
ALTER SUBSCRIPTION testsub ENABLE;" | psql
sleep 1
rm $PGINST/lib/libpqwalreceiver.so
sleep 15
pg_ctl -D "$PGDB" stop -m immediate
grep 'TRAP:' server.log
Leads to multiple assertion failures:
CREATE SUBSCRIPTION
ALTER SUBSCRIPTION
waiting for server to shut down done
server stopped
TRAP: failed Assert("MyProc->backendId != InvalidBackendId"), File: "lock.c",
Line: 4439, PID: 2899323
TRAP: failed Assert("MyProc->backendId != InvalidBackendId"), File: "lock.c",
Line: 4439, PID: 2899416
TRAP: failed Assert("MyProc->backendId != InvalidBackendId"), File: "lock.c",
Line: 4439, PID: 2899427
TRAP: failed Assert("MyProc->backendId != InvalidBackendId"), File: "lock.c",
Line: 4439, PID: 2899439
TRAP: failed Assert("MyProc->backendId != InvalidBackendId"), File: "lock.c",
Line: 4439, PID: 2899538
TRAP: failed Assert("MyProc->backendId != InvalidBackendId"), File: "lock.c",
Line: 4439, PID: 2899547
server.log contains:
2023-04-26 11:00:58.797 MSK [2899300] LOG: database system is ready to accept
connections
2023-04-26 11:00:58.821 MSK [2899416] ERROR: could not access file
"libpqwalreceiver": No such file or directory
TRAP: failed Assert("MyProc->backendId != InvalidBackendId"), File: "lock.c",
Line: 4439, PID: 2899416
postgres: logical replication apply worker for subscription 16385
(ExceptionalCondition+0x69)[0x558b2ac06d41]
postgres: logical replication apply worker for subscription 16385
(VirtualXactLockTableCleanup+0xa4)[0x558b2aa9fd74]
postgres: logical replication apply worker for subscription 16385
(LockReleaseAll+0xbb)[0x558b2aa9fe7d]
postgres: logical replication apply worker for subscription 16385
(+0x4588c6)[0x558b2aa2a8c6]
postgres: logical replication apply worker for subscription 16385
(shmem_exit+0x6c)[0x558b2aa87eb1]
postgres: logical replication apply worker for subscription 16385
(+0x4b5faa)[0x558b2aa87faa]
postgres: logical replication apply worker for subscription 16385
(proc_exit+0xc)[0x558b2aa88031]
postgres: logical replication apply worker for subscription 16385
(StartBackgroundWorker+0x147)[0x558b2aa0b4d9]
postgres: logical replication apply worker for subscription 16385
(+0x43fdc1)[0x558b2aa11dc1]
postgres: logical replication apply worker for subscription 16385
(+0x43ff3d)[0x558b2aa11f3d]
postgres: logical replication apply worker for subscription 16385
(+0x440866)[0x558b2aa12866]
postgres: logical replication apply worker for subscription 16385
(+0x440e12)[0x558b2aa12e12]
postgres: logical replication apply worker for subscription 16385
(BackgroundWorkerInitializeConnection+0x0)[0x558b2aa14396]
postgres: logical replication apply worker for subscription 16385
(main+0x21a)[0x558b2a932e21]
I understand, that removing libpqwalreceiver.so (or whole pginst/) is not
what happens in a production environment every day, but nonetheless it's a
new failure mode and it can produce many coredumps when testing.
IIUC, that assert will fail in case of any error raised between
ApplyWorkerMain()->logicalrep_worker_attach()->before_shmem_exit() and
ApplyWorkerMain()->InitializeApplyWorker()->BackgroundWorkerInitializeConnectionByOid()->InitPostgres().
Best regards,
Alexander
Re: Perform streaming logical transactions by background workers and parallel apply
On Mon, Apr 24, 2023 at 2:24 PM Amit Kapila wrote: > > On Mon, Apr 24, 2023 at 7:26 AM Masahiko Sawada wrote: > > > > While looking at the worker.c, I realized that we have the following > > code in handle_streamed_transaction(): > > > > default: > > Assert(false); > > return false; / silence compiler warning / > > > > I think it's better to do elog(ERROR) instead of Assert() as it ends > > up returning false in non-assertion builds, which might cause a > > problem. And it's more consistent with other codes in worker.c. Please > > find an attached patch. > > > > I haven't tested it but otherwise, the changes look good to me. Thanks for checking! Pushed. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Re: Perform streaming logical transactions by background workers and parallel apply
On Mon, Apr 24, 2023 at 7:26 AM Masahiko Sawada wrote: > > While looking at the worker.c, I realized that we have the following > code in handle_streamed_transaction(): > > default: > Assert(false); > return false; / silence compiler warning / > > I think it's better to do elog(ERROR) instead of Assert() as it ends > up returning false in non-assertion builds, which might cause a > problem. And it's more consistent with other codes in worker.c. Please > find an attached patch. > I haven't tested it but otherwise, the changes look good to me. -- With Regards, Amit Kapila.
Re: Perform streaming logical transactions by background workers and parallel apply
At Mon, 24 Apr 2023 08:59:07 +0530, Amit Kapila wrote in > > Sorry for posting multiple times in a row, but I'm a bit unceratin > > whether we should use FATAL or ERROR for this situation. The stream is > > not provided by user, and the session or process cannot continue. > > > > I think ERROR should be fine here similar to other cases in worker.c. Sure, I don't have any issues with it. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Perform streaming logical transactions by background workers and parallel apply
On Mon, Apr 24, 2023 at 8:40 AM Kyotaro Horiguchi wrote: > > At Mon, 24 Apr 2023 11:50:37 +0900 (JST), Kyotaro Horiguchi > wrote in > > In my opinion, it is fine to replace the Assert with an ERROR. > > Sorry for posting multiple times in a row, but I'm a bit unceratin > whether we should use FATAL or ERROR for this situation. The stream is > not provided by user, and the session or process cannot continue. > I think ERROR should be fine here similar to other cases in worker.c. -- With Regards, Amit Kapila.
Re: Perform streaming logical transactions by background workers and parallel apply
At Mon, 24 Apr 2023 11:50:37 +0900 (JST), Kyotaro Horiguchi wrote in > In my opinion, it is fine to replace the Assert with an ERROR. Sorry for posting multiple times in a row, but I'm a bit unceratin whether we should use FATAL or ERROR for this situation. The stream is not provided by user, and the session or process cannot continue. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Perform streaming logical transactions by background workers and parallel apply
At Mon, 24 Apr 2023 11:50:37 +0900 (JST), Kyotaro Horiguchi wrote in > I concur that returning false is problematic. > > For assertion builds, Assert typically provides more detailed > information than elog. However, in this case, it wouldn't matter much > since the worker would repeatedly restart even after a server-restart > for the same reason unless cosmic rays are involved. Moreover, the > situation doesn't justify server-restaring, as it would unnecessarily > involve other backends. Please disregard this part, as it's not relavant to non-assertion builds. > In my opinion, it is fine to replace the Assert with an ERROR. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Perform streaming logical transactions by background workers and parallel apply
At Mon, 24 Apr 2023 10:55:44 +0900, Masahiko Sawada wrote in > While looking at the worker.c, I realized that we have the following > code in handle_streamed_transaction(): > > default: > Assert(false); > return false; / silence compiler warning / > > I think it's better to do elog(ERROR) instead of Assert() as it ends > up returning false in non-assertion builds, which might cause a > problem. And it's more consistent with other codes in worker.c. Please > find an attached patch. I concur that returning false is problematic. For assertion builds, Assert typically provides more detailed information than elog. However, in this case, it wouldn't matter much since the worker would repeatedly restart even after a server-restart for the same reason unless cosmic rays are involved. Moreover, the situation doesn't justify server-restaring, as it would unnecessarily involve other backends. In my opinion, it is fine to replace the Assert with an ERROR. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: Perform streaming logical transactions by background workers and parallel apply
On Mon, Jan 9, 2023 at 5:51 PM Amit Kapila wrote: > > On Sun, Jan 8, 2023 at 11:32 AM houzj.f...@fujitsu.com > wrote: > > > > On Sunday, January 8, 2023 11:59 AM houzj.f...@fujitsu.com > > wrote: > > > Attach the updated patch set. > > > > Sorry, the commit message of 0001 was accidentally deleted, just attach > > the same patch set again with commit message. > > > > Pushed the first (0001) patch. While looking at the worker.c, I realized that we have the following code in handle_streamed_transaction(): default: Assert(false); return false; / silence compiler warning / I think it's better to do elog(ERROR) instead of Assert() as it ends up returning false in non-assertion builds, which might cause a problem. And it's more consistent with other codes in worker.c. Please find an attached patch. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com 0001-Use-elog-to-report-unexpected-action-in-handle_strea.patch Description: Binary data
Re: Perform streaming logical transactions by background workers and parallel apply
LGTM. My only comment is about the commit message. == Commit message d9d7fe6 reuse existing wait event when sending data in apply worker. But we should have invent a new wait state if we are waiting at a new place, so fix this. ~ SUGGESTION d9d7fe6 made use of an existing wait event when sending data from the apply worker, but we should have invented a new wait state since the code was waiting at a new place. This patch corrects the mistake by using a new wait state "LogicalApplySendData". -- Kind Regards, Peter Smith. Fujitsu Australia
Re: Perform streaming logical transactions by background workers and parallel apply
On Wed, Feb 15, 2023 at 8:55 AM houzj.f...@fujitsu.com wrote: > > On Wednesday, February 15, 2023 10:34 AM Amit Kapila > wrote: > > > > > > > > > > So names like the below seem correct format: > > > > > > > > a) WAIT_EVENT_LOGICAL_APPLY_SEND_DATA > > > > b) WAIT_EVENT_LOGICAL_LEADER_SEND_DATA > > > > c) WAIT_EVENT_LOGICAL_LEADER_APPLY_SEND_DATA > > > > > > Personally I'm fine even without "LEADER" in the wait event name since > > > we don't have "who is waiting" in it. IIUC a row of pg_stat_activity > > > shows who, and the wait event name shows "what the process is > > > waiting". So I prefer (a). > > > > > > > This logic makes sense to me. So, let's go with (a). > > OK, here is patch that change the event name to > WAIT_EVENT_LOGICAL_APPLY_SEND_DATA. > LGTM. -- With Regards, Amit Kapila.
RE: Perform streaming logical transactions by background workers and parallel apply
On Wednesday, February 15, 2023 10:34 AM Amit Kapila wrote: > > On Tue, Feb 14, 2023 at 7:45 PM Masahiko Sawada > wrote: > > > > On Tue, Feb 14, 2023 at 3:58 PM Peter Smith > wrote: > > > > > > On Tue, Feb 14, 2023 at 5:04 PM Amit Kapila > wrote: > > > > > > > > On Fri, Feb 10, 2023 at 8:56 AM Peter Smith > wrote: > > > > > > > > > > My first impression was the > > > > > WAIT_EVENT_LOGICAL_PARALLEL_APPLY_SEND_DATA name seemed > > > > > misleading because that makes it sound like the parallel apply > > > > > worker is doing the sending, but IIUC it's really the opposite. > > > > > > > > > > > > > So, how about WAIT_EVENT_LOGICAL_APPLY_SEND_DATA? > > > > > > > > > > Yes, IIUC all the LR events are named WAIT_EVENT_LOGICAL_xxx. > > > > > > So names like the below seem correct format: > > > > > > a) WAIT_EVENT_LOGICAL_APPLY_SEND_DATA > > > b) WAIT_EVENT_LOGICAL_LEADER_SEND_DATA > > > c) WAIT_EVENT_LOGICAL_LEADER_APPLY_SEND_DATA > > > > Personally I'm fine even without "LEADER" in the wait event name since > > we don't have "who is waiting" in it. IIUC a row of pg_stat_activity > > shows who, and the wait event name shows "what the process is > > waiting". So I prefer (a). > > > > This logic makes sense to me. So, let's go with (a). OK, here is patch that change the event name to WAIT_EVENT_LOGICAL_APPLY_SEND_DATA. Best Regard, Hou zj v2-0001-Add-a-new-wait-state-and-use-it-when-sending-data.patch Description: v2-0001-Add-a-new-wait-state-and-use-it-when-sending-data.patch
Re: Perform streaming logical transactions by background workers and parallel apply
On Tue, Feb 14, 2023 at 7:45 PM Masahiko Sawada wrote: > > On Tue, Feb 14, 2023 at 3:58 PM Peter Smith wrote: > > > > On Tue, Feb 14, 2023 at 5:04 PM Amit Kapila wrote: > > > > > > On Fri, Feb 10, 2023 at 8:56 AM Peter Smith wrote: > > > > > > > > My first impression was the > > > > WAIT_EVENT_LOGICAL_PARALLEL_APPLY_SEND_DATA name seemed misleading > > > > because that makes it sound like the parallel apply worker is doing > > > > the sending, but IIUC it's really the opposite. > > > > > > > > > > So, how about WAIT_EVENT_LOGICAL_APPLY_SEND_DATA? > > > > > > > Yes, IIUC all the LR events are named WAIT_EVENT_LOGICAL_xxx. > > > > So names like the below seem correct format: > > > > a) WAIT_EVENT_LOGICAL_APPLY_SEND_DATA > > b) WAIT_EVENT_LOGICAL_LEADER_SEND_DATA > > c) WAIT_EVENT_LOGICAL_LEADER_APPLY_SEND_DATA > > Personally I'm fine even without "LEADER" in the wait event name since > we don't have "who is waiting" in it. IIUC a row of pg_stat_activity > shows who, and the wait event name shows "what the process is > waiting". So I prefer (a). > This logic makes sense to me. So, let's go with (a). -- With Regards, Amit Kapila.
Re: Perform streaming logical transactions by background workers and parallel apply
On Tue, Feb 14, 2023 at 3:58 PM Peter Smith wrote: > > On Tue, Feb 14, 2023 at 5:04 PM Amit Kapila wrote: > > > > On Fri, Feb 10, 2023 at 8:56 AM Peter Smith wrote: > > > > > > On Fri, Feb 10, 2023 at 1:32 PM houzj.f...@fujitsu.com > > > wrote: > > > > > > > > On Tuesday, February 7, 2023 11:17 AM Amit Kapila > > > > wrote: > > > > > > > > > > On Mon, Feb 6, 2023 at 3:43 PM houzj.f...@fujitsu.com > > > > > wrote: > > > > > > > > > > > > while reading the code, I noticed that in pa_send_data() we set wait > > > > > > event to WAIT_EVENT_LOGICAL_PARALLEL_APPLY_STATE_CHANGE while > > > > > sending > > > > > > the message to the queue. Because this state is used in multiple > > > > > > places, user might not be able to distinguish what they are waiting > > > > > > for. So It seems we'd better to use WAIT_EVENT_MQ_SEND here which > > > > > > will > > > > > > be eaier to distinguish and understand. Here is a tiny patch for > > > > > > that. > > > > > > > > > > > > > > As discussed[1], we'd better invent a new state for this purpose, so > > > > here is the patch > > > > that does the same. > > > > > > > > [1] > > > > https://www.postgresql.org/message-id/CAA4eK1LTud4FLRbS0QqdZ-pjSxwfFLHC1Dx%3D6Q7nyROCvvPSfw%40mail.gmail.com > > > > > > > > > > My first impression was the > > > WAIT_EVENT_LOGICAL_PARALLEL_APPLY_SEND_DATA name seemed misleading > > > because that makes it sound like the parallel apply worker is doing > > > the sending, but IIUC it's really the opposite. > > > > > > > So, how about WAIT_EVENT_LOGICAL_APPLY_SEND_DATA? > > > > Yes, IIUC all the LR events are named WAIT_EVENT_LOGICAL_xxx. > > So names like the below seem correct format: > > a) WAIT_EVENT_LOGICAL_APPLY_SEND_DATA > b) WAIT_EVENT_LOGICAL_LEADER_SEND_DATA > c) WAIT_EVENT_LOGICAL_LEADER_APPLY_SEND_DATA Personally I'm fine even without "LEADER" in the wait event name since we don't have "who is waiting" in it. IIUC a row of pg_stat_activity shows who, and the wait event name shows "what the process is waiting". So I prefer (a). Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Re: Perform streaming logical transactions by background workers and parallel apply
On Tue, Feb 14, 2023 at 5:04 PM Amit Kapila wrote: > > On Fri, Feb 10, 2023 at 8:56 AM Peter Smith wrote: > > > > On Fri, Feb 10, 2023 at 1:32 PM houzj.f...@fujitsu.com > > wrote: > > > > > > On Tuesday, February 7, 2023 11:17 AM Amit Kapila > > > wrote: > > > > > > > > On Mon, Feb 6, 2023 at 3:43 PM houzj.f...@fujitsu.com > > > > wrote: > > > > > > > > > > while reading the code, I noticed that in pa_send_data() we set wait > > > > > event to WAIT_EVENT_LOGICAL_PARALLEL_APPLY_STATE_CHANGE while > > > > sending > > > > > the message to the queue. Because this state is used in multiple > > > > > places, user might not be able to distinguish what they are waiting > > > > > for. So It seems we'd better to use WAIT_EVENT_MQ_SEND here which will > > > > > be eaier to distinguish and understand. Here is a tiny patch for that. > > > > > > > > > > > As discussed[1], we'd better invent a new state for this purpose, so here > > > is the patch > > > that does the same. > > > > > > [1] > > > https://www.postgresql.org/message-id/CAA4eK1LTud4FLRbS0QqdZ-pjSxwfFLHC1Dx%3D6Q7nyROCvvPSfw%40mail.gmail.com > > > > > > > My first impression was the > > WAIT_EVENT_LOGICAL_PARALLEL_APPLY_SEND_DATA name seemed misleading > > because that makes it sound like the parallel apply worker is doing > > the sending, but IIUC it's really the opposite. > > > > So, how about WAIT_EVENT_LOGICAL_APPLY_SEND_DATA? > Yes, IIUC all the LR events are named WAIT_EVENT_LOGICAL_xxx. So names like the below seem correct format: a) WAIT_EVENT_LOGICAL_APPLY_SEND_DATA b) WAIT_EVENT_LOGICAL_LEADER_SEND_DATA c) WAIT_EVENT_LOGICAL_LEADER_APPLY_SEND_DATA Of those, I prefer option c) because saying LEADER_APPLY_xxx matches the name format of the existing WAIT_EVENT_LOGICAL_PARALLEL_APPLY_STATE_CHANGE. -- Kind Regards, Peter Smith. Fujitsu Australia
Re: Perform streaming logical transactions by background workers and parallel apply
On Fri, Feb 10, 2023 at 8:56 AM Peter Smith wrote: > > On Fri, Feb 10, 2023 at 1:32 PM houzj.f...@fujitsu.com > wrote: > > > > On Tuesday, February 7, 2023 11:17 AM Amit Kapila > > wrote: > > > > > > On Mon, Feb 6, 2023 at 3:43 PM houzj.f...@fujitsu.com > > > wrote: > > > > > > > > while reading the code, I noticed that in pa_send_data() we set wait > > > > event to WAIT_EVENT_LOGICAL_PARALLEL_APPLY_STATE_CHANGE while > > > sending > > > > the message to the queue. Because this state is used in multiple > > > > places, user might not be able to distinguish what they are waiting > > > > for. So It seems we'd better to use WAIT_EVENT_MQ_SEND here which will > > > > be eaier to distinguish and understand. Here is a tiny patch for that. > > > > > > > > As discussed[1], we'd better invent a new state for this purpose, so here > > is the patch > > that does the same. > > > > [1] > > https://www.postgresql.org/message-id/CAA4eK1LTud4FLRbS0QqdZ-pjSxwfFLHC1Dx%3D6Q7nyROCvvPSfw%40mail.gmail.com > > > > My first impression was the > WAIT_EVENT_LOGICAL_PARALLEL_APPLY_SEND_DATA name seemed misleading > because that makes it sound like the parallel apply worker is doing > the sending, but IIUC it's really the opposite. > So, how about WAIT_EVENT_LOGICAL_APPLY_SEND_DATA? > And since WAIT_EVENT_LOGICAL_PARALLEL_APPLY_LEADER_SEND_DATA seems too > verbose, how about shortening the prefix for both events? E.g. > > BEFORE > WAIT_EVENT_LOGICAL_PARALLEL_APPLY_SEND_DATA, > WAIT_EVENT_LOGICAL_PARALLEL_APPLY_STATE_CHANGE, > > AFTER > WAIT_EVENT_LOGICAL_PA_LEADER_SEND_DATA, > WAIT_EVENT_LOGICAL_PA_STATE_CHANGE, > I am not sure *_PA_LEADER_* is any better that what Hou-San has proposed. -- With Regards, Amit Kapila.
Re: Perform streaming logical transactions by background workers and parallel apply
On Fri, Feb 10, 2023 at 1:32 PM houzj.f...@fujitsu.com wrote: > > On Tuesday, February 7, 2023 11:17 AM Amit Kapila > wrote: > > > > On Mon, Feb 6, 2023 at 3:43 PM houzj.f...@fujitsu.com > > wrote: > > > > > > while reading the code, I noticed that in pa_send_data() we set wait > > > event to WAIT_EVENT_LOGICAL_PARALLEL_APPLY_STATE_CHANGE while > > sending > > > the message to the queue. Because this state is used in multiple > > > places, user might not be able to distinguish what they are waiting > > > for. So It seems we'd better to use WAIT_EVENT_MQ_SEND here which will > > > be eaier to distinguish and understand. Here is a tiny patch for that. > > > > > As discussed[1], we'd better invent a new state for this purpose, so here is > the patch > that does the same. > > [1] > https://www.postgresql.org/message-id/CAA4eK1LTud4FLRbS0QqdZ-pjSxwfFLHC1Dx%3D6Q7nyROCvvPSfw%40mail.gmail.com > My first impression was the WAIT_EVENT_LOGICAL_PARALLEL_APPLY_SEND_DATA name seemed misleading because that makes it sound like the parallel apply worker is doing the sending, but IIUC it's really the opposite. And since WAIT_EVENT_LOGICAL_PARALLEL_APPLY_LEADER_SEND_DATA seems too verbose, how about shortening the prefix for both events? E.g. BEFORE WAIT_EVENT_LOGICAL_PARALLEL_APPLY_SEND_DATA, WAIT_EVENT_LOGICAL_PARALLEL_APPLY_STATE_CHANGE, AFTER WAIT_EVENT_LOGICAL_PA_LEADER_SEND_DATA, WAIT_EVENT_LOGICAL_PA_STATE_CHANGE, -- Kind Regards, Peter Smith. Fujitsu Australia
RE: Perform streaming logical transactions by background workers and parallel apply
On Tuesday, February 7, 2023 11:17 AM Amit Kapila wrote: > > On Mon, Feb 6, 2023 at 3:43 PM houzj.f...@fujitsu.com > wrote: > > > > while reading the code, I noticed that in pa_send_data() we set wait > > event to WAIT_EVENT_LOGICAL_PARALLEL_APPLY_STATE_CHANGE while > sending > > the message to the queue. Because this state is used in multiple > > places, user might not be able to distinguish what they are waiting > > for. So It seems we'd better to use WAIT_EVENT_MQ_SEND here which will > > be eaier to distinguish and understand. Here is a tiny patch for that. > > As discussed[1], we'd better invent a new state for this purpose, so here is the patch that does the same. [1] https://www.postgresql.org/message-id/CAA4eK1LTud4FLRbS0QqdZ-pjSxwfFLHC1Dx%3D6Q7nyROCvvPSfw%40mail.gmail.com Best Regards, Hou zj 0001-Add-new-wait-event-to-be-used-in-apply-worker.patch Description: 0001-Add-new-wait-event-to-be-used-in-apply-worker.patch
RE: Perform streaming logical transactions by background workers and parallel apply
On Tue, Feb 7, 2023 15:37 PM Amit Kapila wrote: > On Tue, Feb 7, 2023 at 12:41 PM Masahiko Sawada > wrote: > > > > On Fri, Feb 3, 2023 at 6:44 PM Amit Kapila wrote: > > > > > We need to think of a predictable > > > way to test this path which may not be difficult. But I guess it would > > > be better to wait for some feedback from the field about this feature > > > before adding more to it and anyway it shouldn't be a big deal to add > > > this later as well. > > > > Agreed to hear some feedback before adding it. It's not an urgent feature. > > > > Okay, Thanks! AFAIK, there is no pending patch left in this proposal. > If so, I think it is better to close the corresponding CF entry. Yes, I think so. Closed this CF entry. Regards, Wang Wei
Re: Perform streaming logical transactions by background workers and parallel apply
On Tue, Feb 7, 2023 at 12:41 PM Masahiko Sawada wrote: > > On Fri, Feb 3, 2023 at 6:44 PM Amit Kapila wrote: > > > We need to think of a predictable > > way to test this path which may not be difficult. But I guess it would > > be better to wait for some feedback from the field about this feature > > before adding more to it and anyway it shouldn't be a big deal to add > > this later as well. > > Agreed to hear some feedback before adding it. It's not an urgent feature. > Okay, Thanks! AFAIK, there is no pending patch left in this proposal. If so, I think it is better to close the corresponding CF entry. -- With Regards, Amit Kapila.
Re: Perform streaming logical transactions by background workers and parallel apply
On Fri, Feb 3, 2023 at 6:44 PM Amit Kapila wrote: > > On Fri, Feb 3, 2023 at 1:28 PM Masahiko Sawada wrote: > > > > On Fri, Feb 3, 2023 at 12:29 PM houzj.f...@fujitsu.com > > wrote: > > > > > > On Friday, February 3, 2023 11:04 AM Amit Kapila > > > wrote: > > > > > > > > On Thu, Feb 2, 2023 at 4:52 AM Peter Smith > > > > wrote: > > > > > > > > > > Some minor review comments for v91-0001 > > > > > > > > > > > > > Pushed this yesterday after addressing your comments! > > > > > > Thanks for pushing. > > > > > > Currently, we have two remaining patches which we are not sure whether > > > it's worth > > > committing for now. Just share them here for reference. > > > > > > 0001: > > > > > > Based on our discussion[1] on -hackers, it's not clear that if it's > > > necessary > > > to add the sub-feature to stop extra worker when > > > max_apply_workers_per_suibscription is reduced. Because: > > > > > > - it's not clear whether reducing the > > > 'max_apply_workers_per_suibscription' is very > > > common. > > > > A use case I'm concerned about is a temporarily intensive data load, > > for example, a data loading batch job in a maintenance window. In this > > case, the user might want to temporarily increase > > max_parallel_workers_per_subscription in order to avoid a large > > replication lag, and revert the change back to normal after the job. > > If it's unlikely to stream the changes in the regular workload as > > logical_decoding_work_mem is big enough to handle the regular > > transaction data, the excess parallel workers won't exit. > > > > Won't in such a case, it would be better to just switch off the > parallel option for a subscription? Not sure. Changing the parameter would be easier since it doesn't require restarts. > We need to think of a predictable > way to test this path which may not be difficult. But I guess it would > be better to wait for some feedback from the field about this feature > before adding more to it and anyway it shouldn't be a big deal to add > this later as well. Agreed to hear some feedback before adding it. It's not an urgent feature. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Re: Perform streaming logical transactions by background workers and parallel apply
On Mon, Feb 6, 2023 at 3:43 PM houzj.f...@fujitsu.com wrote: > > while reading the code, I noticed that in pa_send_data() we set wait event > to WAIT_EVENT_LOGICAL_PARALLEL_APPLY_STATE_CHANGE while sending the > message to the queue. Because this state is used in multiple places, user > might > not be able to distinguish what they are waiting for. So It seems we'd better > to use WAIT_EVENT_MQ_SEND here which will be eaier to distinguish and > understand. Here is a tiny patch for that. > Thanks for noticing this. The patch LGTM. I'll push this in some time. -- With Regards, Amit Kapila.
RE: Perform streaming logical transactions by background workers and parallel apply
Dear Hou, > I think PARALLEL_APPLY_MAIN waits for two kinds of event: 1) wait for new > message from the queue 2) wait for the partial file state to be set. So, I > think introducing a new general event for them is better and it is also > consistent with the WAIT_EVENT_LOGICAL_APPLY_MAIN which is used in the > main > loop of leader apply worker(LogicalRepApplyLoop). But the event in > pg_send_data() is only for message send, so it seems fine to use > WAIT_EVENT_MQ_SEND, besides MQ_SEND is also unique in parallel apply > worker and > user can distinglish without adding new event. Thank you for your explanation. I think both of you said are reasonable. Best Regards, Hayato Kuroda FUJITSU LIMITED
RE: Perform streaming logical transactions by background workers and parallel apply
On Monday, February 6, 2023 6:34 PM Kuroda, Hayato wrote: > > while reading the code, I noticed that in pa_send_data() we set wait > > event to WAIT_EVENT_LOGICAL_PARALLEL_APPLY_STATE_CHANGE while > sending > > the message to the queue. Because this state is used in multiple > > places, user might not be able to distinguish what they are waiting > > for. So It seems we'd better to use WAIT_EVENT_MQ_SEND here which will > > be eaier to distinguish and understand. Here is a tiny patch for that. > > In LogicalParallelApplyLoop(), we introduced the new wait event > WAIT_EVENT_LOGICAL_PARALLEL_APPLY_MAIN whereas it is practically waits a > shared message queue and it seems to be same as WAIT_EVENT_MQ_RECEIVE. > Do you have a policy to reuse the event instead of adding a new event? I think PARALLEL_APPLY_MAIN waits for two kinds of event: 1) wait for new message from the queue 2) wait for the partial file state to be set. So, I think introducing a new general event for them is better and it is also consistent with the WAIT_EVENT_LOGICAL_APPLY_MAIN which is used in the main loop of leader apply worker(LogicalRepApplyLoop). But the event in pg_send_data() is only for message send, so it seems fine to use WAIT_EVENT_MQ_SEND, besides MQ_SEND is also unique in parallel apply worker and user can distinglish without adding new event. Best Regards, Hou zj
RE: Perform streaming logical transactions by background workers and parallel apply
Dear Hou, > while reading the code, I noticed that in pa_send_data() we set wait event > to WAIT_EVENT_LOGICAL_PARALLEL_APPLY_STATE_CHANGE while sending > the > message to the queue. Because this state is used in multiple places, user > might > not be able to distinguish what they are waiting for. So It seems we'd better > to use WAIT_EVENT_MQ_SEND here which will be eaier to distinguish and > understand. Here is a tiny patch for that. In LogicalParallelApplyLoop(), we introduced the new wait event WAIT_EVENT_LOGICAL_PARALLEL_APPLY_MAIN whereas it is practically waits a shared message queue and it seems to be same as WAIT_EVENT_MQ_RECEIVE. Do you have a policy to reuse the event instead of adding a new event? Best Regards, Hayato Kuroda FUJITSU LIMITED
RE: Perform streaming logical transactions by background workers and parallel apply
Hi, while reading the code, I noticed that in pa_send_data() we set wait event to WAIT_EVENT_LOGICAL_PARALLEL_APPLY_STATE_CHANGE while sending the message to the queue. Because this state is used in multiple places, user might not be able to distinguish what they are waiting for. So It seems we'd better to use WAIT_EVENT_MQ_SEND here which will be eaier to distinguish and understand. Here is a tiny patch for that. Best Regards, Hou zj 0001-Use-appropriate-wait-event-when-sending-data.patch Description: 0001-Use-appropriate-wait-event-when-sending-data.patch
Re: Perform streaming logical transactions by background workers and parallel apply
On Fri, Feb 3, 2023 at 1:28 PM Masahiko Sawada wrote: > > On Fri, Feb 3, 2023 at 12:29 PM houzj.f...@fujitsu.com > wrote: > > > > On Friday, February 3, 2023 11:04 AM Amit Kapila > > wrote: > > > > > > On Thu, Feb 2, 2023 at 4:52 AM Peter Smith > > > wrote: > > > > > > > > Some minor review comments for v91-0001 > > > > > > > > > > Pushed this yesterday after addressing your comments! > > > > Thanks for pushing. > > > > Currently, we have two remaining patches which we are not sure whether it's > > worth > > committing for now. Just share them here for reference. > > > > 0001: > > > > Based on our discussion[1] on -hackers, it's not clear that if it's > > necessary > > to add the sub-feature to stop extra worker when > > max_apply_workers_per_suibscription is reduced. Because: > > > > - it's not clear whether reducing the 'max_apply_workers_per_suibscription' > > is very > > common. > > A use case I'm concerned about is a temporarily intensive data load, > for example, a data loading batch job in a maintenance window. In this > case, the user might want to temporarily increase > max_parallel_workers_per_subscription in order to avoid a large > replication lag, and revert the change back to normal after the job. > If it's unlikely to stream the changes in the regular workload as > logical_decoding_work_mem is big enough to handle the regular > transaction data, the excess parallel workers won't exit. > Won't in such a case, it would be better to just switch off the parallel option for a subscription? We need to think of a predictable way to test this path which may not be difficult. But I guess it would be better to wait for some feedback from the field about this feature before adding more to it and anyway it shouldn't be a big deal to add this later as well. -- With Regards, Amit Kapila.
Re: Perform streaming logical transactions by background workers and parallel apply
On Fri, Feb 3, 2023 at 12:29 PM houzj.f...@fujitsu.com wrote: > > On Friday, February 3, 2023 11:04 AM Amit Kapila > wrote: > > > > On Thu, Feb 2, 2023 at 4:52 AM Peter Smith > > wrote: > > > > > > Some minor review comments for v91-0001 > > > > > > > Pushed this yesterday after addressing your comments! > > Thanks for pushing. > > Currently, we have two remaining patches which we are not sure whether it's > worth > committing for now. Just share them here for reference. > > 0001: > > Based on our discussion[1] on -hackers, it's not clear that if it's necessary > to add the sub-feature to stop extra worker when > max_apply_workers_per_suibscription is reduced. Because: > > - it's not clear whether reducing the 'max_apply_workers_per_suibscription' > is very > common. A use case I'm concerned about is a temporarily intensive data load, for example, a data loading batch job in a maintenance window. In this case, the user might want to temporarily increase max_parallel_workers_per_subscription in order to avoid a large replication lag, and revert the change back to normal after the job. If it's unlikely to stream the changes in the regular workload as logical_decoding_work_mem is big enough to handle the regular transaction data, the excess parallel workers won't exit. Another subscription might want to use parallel workers but there might not be free workers. That's why I thought we need to free the excess workers at some point. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
RE: Perform streaming logical transactions by background workers and parallel apply
On Friday, February 3, 2023 11:04 AM Amit Kapila wrote: > > On Thu, Feb 2, 2023 at 4:52 AM Peter Smith > wrote: > > > > Some minor review comments for v91-0001 > > > > Pushed this yesterday after addressing your comments! Thanks for pushing. Currently, we have two remaining patches which we are not sure whether it's worth committing for now. Just share them here for reference. 0001: Based on our discussion[1] on -hackers, it's not clear that if it's necessary to add the sub-feature to stop extra worker when max_apply_workers_per_suibscription is reduced. Because: - it's not clear whether reducing the 'max_apply_workers_per_suibscription' is very common. - even when the GUC is reduced, at that point in time all the workers might be in use so there may be nothing that can be immediately done. - IIUC the excess workers (for a reduced GUC) are going to get freed naturally anyway over time as more transactions are completed so the pool size will reduce accordingly. And given that the logic of this patch is simple, it would be easy to add this at a later point if we really see a use case for this. 0002: Since all the deadlock errors and other errors that caused by parallel streaming will be logged and user can check this kind of ERROR and disable the parallel streaming mode to resolve this. Besides, for this retry feature, It will be hard to distinguish whether the ERROR is caused by parallel streaming, and we might need to retry in serialize mode for all kinds of ERROR. So, it's not very clear if automatic use serialize mode to retry in case of any ERROR in parallel streaming is necessary or not. And we can also add this when we see a use case. [1] https://www.postgresql.org/message-id/CAA4eK1LotEuPsteuJMNpixxTj6R4B8k93q-6ruRmDzCxKzMNpA%40mail.gmail.com Best Regards, Hou zj v92-0001-Stop-extra-worker-if-GUC-was-changed.patch Description: v92-0001-Stop-extra-worker-if-GUC-was-changed.patch v92-0002-Retry-to-apply-streaming-xact-only-in-apply-work.patch Description: v92-0002-Retry-to-apply-streaming-xact-only-in-apply-work.patch
Re: Perform streaming logical transactions by background workers and parallel apply
On Thu, Feb 2, 2023 at 4:52 AM Peter Smith wrote: > > Some minor review comments for v91-0001 > Pushed this yesterday after addressing your comments! -- With Regards, Amit Kapila.
Re: Perform streaming logical transactions by background workers and parallel apply
Some minor review comments for v91-0001 == doc/src/sgml/config.sgml 1. -Allows streaming or serializing changes immediately in logical decoding. -The allowed values of logical_replication_mode are -buffered and immediate. When set -to immediate, stream each change if +The allowed values are buffered and +immediate. The default is buffered. +This parameter is intended to be used to test logical decoding and +replication of large transactions for which otherwise we need to generate +the changes till logical_decoding_work_mem is +reached. The effect of logical_replication_mode is +different for the publisher and subscriber: + The "for which otherwise..." part is only relevant for the publisher-side. So it seemed slightly strange to give the reason why to use the GUC for one side but not the other side. Maybe we can just to remove that "for which otherwise..." part, since the logical_decoding_work_mem gets mentioned later in the "On the publisher side,..." paragraph anyway. ~~~ 2. -This parameter is intended to be used to test logical decoding and -replication of large transactions for which otherwise we need to -generate the changes till logical_decoding_work_mem -is reached. +On the subscriber side, if the streaming option is set to +parallel, logical_replication_mode +can be used to direct the leader apply worker to send changes to the +shared memory queue or to serialize changes to the file. When set to +buffered, the leader sends changes to parallel apply +workers via a shared memory queue. When set to +immediate, the leader serializes all changes to files +and notifies the parallel apply workers to read and apply them at the +end of the transaction. "or serialize changes to the file." --> "or serialize all changes to files." (just to use same wording as later in this same paragraph, and also same wording as the GUC hint text). -- Kind Regards, Peter Smith. Fujitsu Australia
Re: Perform streaming logical transactions by background workers and parallel apply
On Tue, Jan 31, 2023 at 9:04 AM houzj.f...@fujitsu.com wrote: > > I think your comment makes sense, thanks. > I updated the patch for the same. > The patch looks mostly good to me. I have made a few changes in the comments and docs, see attached. -- With Regards, Amit Kapila. v91-0001-Allow-the-logical_replication_mode-to-be-used-on.patch Description: Binary data
Re: Perform streaming logical transactions by background workers and parallel apply
Thanks for the updates to address all of my previous review comments. Patch v90-0001 LGTM. -- Kind Reagrds, Peter Smith. Fujitsu Australia
RE: Perform streaming logical transactions by background workers and parallel apply
On Tuesday, January 31, 2023 8:23 AM Peter Smith wrote: > > On Mon, Jan 30, 2023 at 5:23 PM houzj.f...@fujitsu.com > wrote: > > > > On Monday, January 30, 2023 12:13 PM Peter Smith > wrote: > > > > > > Here are my review comments for v88-0002. > > > > Thanks for your comments. > > > > > > > > == > > > General > > > > > > 1. > > > The test cases are checking the log content but they are not > > > checking for debug logs or untranslated elogs -- they are expecting > > > a normal ereport LOG that might be translated. I’m not sure if that is > > > OK, or > if it is a potential problem. > > > > We have tests that check the ereport ERROR and ereport WARNING > > message(by search for the ERROR or WARNING keyword for all the tap > > tests), so I think checking the LOG should be fine. > > > > > == > > > doc/src/sgml/config.sgml > > > > > > 2. > > > On the publisher side, logical_replication_mode allows allows > > > streaming or serializing changes immediately in logical decoding. > > > When set to immediate, stream each change if streaming option (see > > > optional parameters set by CREATE SUBSCRIPTION) is enabled, > > > otherwise, serialize each change. When set to buffered, the decoding > > > will stream or serialize changes when logical_decoding_work_mem is > reached. > > > > > > 2a. > > > typo "allows allows" (Kuroda-san reported same) > > > > > > 2b. > > > "if streaming option" --> "if the streaming option" > > > > Changed. > > Although you replied "Changed" for the above, AFAICT my review comment > #2b. was accidentally missed. Fixed. Best Regards, Hou zj
RE: Perform streaming logical transactions by background workers and parallel apply
On Monday, January 30, 2023 10:20 PM Masahiko Sawada wrote: > > > I have one comment on v89 patch: > > + /* > +* Using 'immediate' mode returns false to cause a switch to > +* PARTIAL_SERIALIZE mode so that the remaining changes will > be serialized. > +*/ > + if (logical_replication_mode == LOGICAL_REP_MODE_IMMEDIATE) > + return false; > + > > Probably we might want to add unlikely() here since we could pass through this > path very frequently? I think your comment makes sense, thanks. I updated the patch for the same. Best Regards, Hou zj v90-0001-Extend-the-logical_replication_mode-to-test-the-.patch Description: v90-0001-Extend-the-logical_replication_mode-to-test-the-.patch
Re: Perform streaming logical transactions by background workers and parallel apply
On Mon, Jan 30, 2023 at 5:23 PM houzj.f...@fujitsu.com wrote: > > On Monday, January 30, 2023 12:13 PM Peter Smith > wrote: > > > > Here are my review comments for v88-0002. > > Thanks for your comments. > > > > > == > > General > > > > 1. > > The test cases are checking the log content but they are not checking for > > debug logs or untranslated elogs -- they are expecting a normal ereport LOG > > that might be translated. I’m not sure if that is OK, or if it is a > > potential problem. > > We have tests that check the ereport ERROR and ereport WARNING message(by > search for the ERROR or WARNING keyword for all the tap tests), so I think > checking the LOG should be fine. > > > == > > doc/src/sgml/config.sgml > > > > 2. > > On the publisher side, logical_replication_mode allows allows streaming or > > serializing changes immediately in logical decoding. When set to immediate, > > stream each change if streaming option (see optional parameters set by > > CREATE SUBSCRIPTION) is enabled, otherwise, serialize each change. When set > > to buffered, the decoding will stream or serialize changes when > > logical_decoding_work_mem is reached. > > > > 2a. > > typo "allows allows" (Kuroda-san reported same) > > > > 2b. > > "if streaming option" --> "if the streaming option" > > Changed. Although you replied "Changed" for the above, AFAICT my review comment #2b. was accidentally missed. Otherwise, the patch LGTM. -- Kind Regards, Peter Smith. Fujitsu Australia
Re: Perform streaming logical transactions by background workers and parallel apply
On Mon, Jan 30, 2023 at 3:23 PM houzj.f...@fujitsu.com
wrote:
>
> On Monday, January 30, 2023 12:13 PM Peter Smith
> wrote:
> >
> > Here are my review comments for v88-0002.
>
> Thanks for your comments.
>
> >
> > ==
> > General
> >
> > 1.
> > The test cases are checking the log content but they are not checking for
> > debug logs or untranslated elogs -- they are expecting a normal ereport LOG
> > that might be translated. I’m not sure if that is OK, or if it is a
> > potential problem.
>
> We have tests that check the ereport ERROR and ereport WARNING message(by
> search for the ERROR or WARNING keyword for all the tap tests), so I think
> checking the LOG should be fine.
>
> > ==
> > doc/src/sgml/config.sgml
> >
> > 2.
> > On the publisher side, logical_replication_mode allows allows streaming or
> > serializing changes immediately in logical decoding. When set to immediate,
> > stream each change if streaming option (see optional parameters set by
> > CREATE SUBSCRIPTION) is enabled, otherwise, serialize each change. When set
> > to buffered, the decoding will stream or serialize changes when
> > logical_decoding_work_mem is reached.
> >
> > 2a.
> > typo "allows allows" (Kuroda-san reported same)
> >
> > 2b.
> > "if streaming option" --> "if the streaming option"
>
> Changed.
>
> > ~~~
> >
> > 3.
> > On the subscriber side, if streaming option is set to parallel,
> > logical_replication_mode also allows the leader apply worker to send changes
> > to the shared memory queue or to serialize changes.
> >
> > SUGGESTION
> > On the subscriber side, if the streaming option is set to parallel,
> > logical_replication_mode can be used to direct the leader apply worker to
> > send changes to the shared memory queue or to serialize changes.
>
> Changed.
>
> > ==
> > src/backend/utils/misc/guc_tables.c
> >
> > 4.
> > {
> > {"logical_replication_mode", PGC_USERSET, DEVELOPER_OPTIONS,
> > - gettext_noop("Controls when to replicate each change."),
> > - gettext_noop("On the publisher, it allows streaming or serializing each
> > change in logical decoding."),
> > + gettext_noop("Controls the internal behavior of logical replication
> > publisher and subscriber"),
> > + gettext_noop("On the publisher, it allows streaming or "
> > + "serializing each change in logical decoding. On the "
> > + "subscriber, in parallel streaming mode, it allows "
> > + "the leader apply worker to serialize changes to "
> > + "files and notifies the parallel apply workers to "
> > + "read and apply them at the end of the transaction."),
> > GUC_NOT_IN_SAMPLE
> > },
> > Suggest re-wording the long description (subscriber part) to be more like
> > the
> > documentation text.
> >
> > BEFORE
> > On the subscriber, in parallel streaming mode, it allows the leader apply
> > worker
> > to serialize changes to files and notifies the parallel apply workers to
> > read and
> > apply them at the end of the transaction.
> >
> > SUGGESTION
> > On the subscriber, if the streaming option is set to parallel, it directs
> > the leader
> > apply worker to send changes to the shared memory queue or to serialize
> > changes and apply them at the end of the transaction.
> >
>
> Changed.
>
> Attach the new version patch which addressed all comments so far (the v88-0001
> has been committed, so we only have one remaining patch this time).
>
I have one comment on v89 patch:
+ /*
+* Using 'immediate' mode returns false to cause a switch to
+* PARTIAL_SERIALIZE mode so that the remaining changes will
be serialized.
+*/
+ if (logical_replication_mode == LOGICAL_REP_MODE_IMMEDIATE)
+ return false;
+
Probably we might want to add unlikely() here since we could pass
through this path very frequently?
The rest looks good to me.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
RE: Perform streaming logical transactions by background workers and parallel apply
Dear Hou, Thank you for updating the patch! I checked your replies and new patch, and it seems good. Currently I have no comments Best Regards, Hayato Kuroda FUJITSU LIMITED
RE: Perform streaming logical transactions by background workers and parallel apply
On Thursday, January 26, 2023 11:37 AM Kuroda, Hayato/黒田 隼人 wrote: > > Followings are comments. Thanks for the comments. > In this test the rollback-prepared seems not to be executed. This is because > serializations are finished while handling PREPARE message and the final > state of transaction does not affect that, right? I think it may be helpful > to add a one line comment. Yes, but I am slightly unsure if it would be helpful to add this as we only test basic cases(mainly for code coverage) for partial serialization. > > 1. config.sgml > > ``` > +the changes till logical_decoding_work_mem is reached. It can also > be > ``` > > I think it should be sandwiched by . Added. > > 2. config.sgml > > ``` > +On the publisher side, > logical_replication_mode allows > +allows streaming or serializing changes immediately in logical > decoding. > ``` > > Typo "allows allows" -> "allows" Fixed. > 3. test general > > You confirmed that the leader started to serialize changes, but did not ensure > the endpoint. > IIUC the parallel apply worker exits after applying serialized changes, and > it is > not tested yet. > Can we add polling the log somewhere? I checked other tests and didn't find some examples where we test the exit of apply worker or table sync worker. And if the parallel apply worker doesn't stop in this case, we will fail anyway when reusing this worker to handle the next transaction because the queue is broken. So, I prefer to keep the tests short. > 4. 015_stream.pl > > ``` > +is($result, qq(15000), 'all changes are replayed from file') > ``` > > The statement may be unclear because changes can be also replicated when > streaming = on. > How about: "parallel apply worker replayed all changes from file"? Changed. Best regards, Hou zj
RE: Perform streaming logical transactions by background workers and parallel apply
On Monday, January 30, 2023 12:13 PM Peter Smith wrote:
>
> Here are my review comments for v88-0002.
Thanks for your comments.
>
> ==
> General
>
> 1.
> The test cases are checking the log content but they are not checking for
> debug logs or untranslated elogs -- they are expecting a normal ereport LOG
> that might be translated. I’m not sure if that is OK, or if it is a potential
> problem.
We have tests that check the ereport ERROR and ereport WARNING message(by
search for the ERROR or WARNING keyword for all the tap tests), so I think
checking the LOG should be fine.
> ==
> doc/src/sgml/config.sgml
>
> 2.
> On the publisher side, logical_replication_mode allows allows streaming or
> serializing changes immediately in logical decoding. When set to immediate,
> stream each change if streaming option (see optional parameters set by
> CREATE SUBSCRIPTION) is enabled, otherwise, serialize each change. When set
> to buffered, the decoding will stream or serialize changes when
> logical_decoding_work_mem is reached.
>
> 2a.
> typo "allows allows" (Kuroda-san reported same)
>
> 2b.
> "if streaming option" --> "if the streaming option"
Changed.
> ~~~
>
> 3.
> On the subscriber side, if streaming option is set to parallel,
> logical_replication_mode also allows the leader apply worker to send changes
> to the shared memory queue or to serialize changes.
>
> SUGGESTION
> On the subscriber side, if the streaming option is set to parallel,
> logical_replication_mode can be used to direct the leader apply worker to
> send changes to the shared memory queue or to serialize changes.
Changed.
> ==
> src/backend/utils/misc/guc_tables.c
>
> 4.
> {
> {"logical_replication_mode", PGC_USERSET, DEVELOPER_OPTIONS,
> - gettext_noop("Controls when to replicate each change."),
> - gettext_noop("On the publisher, it allows streaming or serializing each
> change in logical decoding."),
> + gettext_noop("Controls the internal behavior of logical replication
> publisher and subscriber"),
> + gettext_noop("On the publisher, it allows streaming or "
> + "serializing each change in logical decoding. On the "
> + "subscriber, in parallel streaming mode, it allows "
> + "the leader apply worker to serialize changes to "
> + "files and notifies the parallel apply workers to "
> + "read and apply them at the end of the transaction."),
> GUC_NOT_IN_SAMPLE
> },
> Suggest re-wording the long description (subscriber part) to be more like the
> documentation text.
>
> BEFORE
> On the subscriber, in parallel streaming mode, it allows the leader apply
> worker
> to serialize changes to files and notifies the parallel apply workers to read
> and
> apply them at the end of the transaction.
>
> SUGGESTION
> On the subscriber, if the streaming option is set to parallel, it directs the
> leader
> apply worker to send changes to the shared memory queue or to serialize
> changes and apply them at the end of the transaction.
>
Changed.
Attach the new version patch which addressed all comments so far (the v88-0001
has been committed, so we only have one remaining patch this time).
Best Regards,
Hou zj
v89-0001-Extend-the-logical_replication_mode-to-test-the-.patch
Description: v89-0001-Extend-the-logical_replication_mode-to-test-the-.patch
Re: Perform streaming logical transactions by background workers and parallel apply
On Mon, Jan 30, 2023 at 5:40 AM Peter Smith wrote: > > Patch v88-0001 LGTM. > Pushed. -- With Regards, Amit Kapila.
Re: Perform streaming logical transactions by background workers and parallel apply
Here are my review comments for v88-0002.
==
General
1.
The test cases are checking the log content but they are not checking
for debug logs or untranslated elogs -- they are expecting a normal
ereport LOG that might be translated. I’m not sure if that is OK, or
if it is a potential problem.
==
doc/src/sgml/config.sgml
2.
On the publisher side, logical_replication_mode allows allows
streaming or serializing changes immediately in logical decoding. When
set to immediate, stream each change if streaming option (see optional
parameters set by CREATE SUBSCRIPTION) is enabled, otherwise,
serialize each change. When set to buffered, the decoding will stream
or serialize changes when logical_decoding_work_mem is reached.
2a.
typo "allows allows" (Kuroda-san reported same)
2b.
"if streaming option" --> "if the streaming option"
~~~
3.
On the subscriber side, if streaming option is set to parallel,
logical_replication_mode also allows the leader apply worker to send
changes to the shared memory queue or to serialize changes.
SUGGESTION
On the subscriber side, if the streaming option is set to parallel,
logical_replication_mode can be used to direct the leader apply worker
to send changes to the shared memory queue or to serialize changes.
==
src/backend/utils/misc/guc_tables.c
4.
{
{"logical_replication_mode", PGC_USERSET, DEVELOPER_OPTIONS,
- gettext_noop("Controls when to replicate each change."),
- gettext_noop("On the publisher, it allows streaming or serializing
each change in logical decoding."),
+ gettext_noop("Controls the internal behavior of logical replication
publisher and subscriber"),
+ gettext_noop("On the publisher, it allows streaming or "
+ "serializing each change in logical decoding. On the "
+ "subscriber, in parallel streaming mode, it allows "
+ "the leader apply worker to serialize changes to "
+ "files and notifies the parallel apply workers to "
+ "read and apply them at the end of the transaction."),
GUC_NOT_IN_SAMPLE
},
Suggest re-wording the long description (subscriber part) to be more
like the documentation text.
BEFORE
On the subscriber, in parallel streaming mode, it allows the leader
apply worker to serialize changes to files and notifies the parallel
apply workers to read and apply them at the end of the transaction.
SUGGESTION
On the subscriber, if the streaming option is set to parallel, it
directs the leader apply worker to send changes to the shared memory
queue or to serialize changes and apply them at the end of the
transaction.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
Re: Perform streaming logical transactions by background workers and parallel apply
Patch v88-0001 LGTM. Below are just some minor review comments about the commit message. == Commit message 1. We have discussed having this parameter as a subscription option but exposing a parameter that is primarily used for testing/debugging to users didn't seem advisable and there is no other such parameter. The other option we have discussed is to have a separate GUC for subscriber-side testing but it appears that for the current testing existing parameter is sufficient and avoids adding another GUC. SUGGESTION We discussed exposing this parameter as a subscription option, but it did not seem advisable since it is primarily used for testing/debugging and there is no other such developer option. We also discussed having separate GUCs for publisher/subscriber-side, but for current testing/debugging requirements, one GUC is sufficient. ~~ 2. Reviewed-by: Pater Smith, Kuroda Hayato, Amit Kapila "Pater" --> "Peter" -- Kind Regards, Peter Smith. Fujitsu Australia
Re: Perform streaming logical transactions by background workers and parallel apply
On Wed, Jan 25, 2023 at 3:27 PM Amit Kapila wrote:
>
> On Wed, Jan 25, 2023 at 10:05 AM Amit Kapila wrote:
> >
> > On Wed, Jan 25, 2023 at 3:15 AM Peter Smith wrote:
> > >
> > > 1.
> > > @@ -210,7 +210,7 @@ int logical_decoding_work_mem;
> > > static const Size max_changes_in_memory = 4096; /* XXX for restore only
> > > */
> > >
> > > /* GUC variable */
> > > -int logical_decoding_mode = LOGICAL_DECODING_MODE_BUFFERED;
> > > +int logical_replication_mode = LOGICAL_REP_MODE_BUFFERED;
> > >
> > >
> > > I noticed that the comment /* GUC variable */ is currently only above
> > > the logical_replication_mode, but actually logical_decoding_work_mem
> > > is a GUC variable too. Maybe this should be rearranged somehow then
> > > change the comment "GUC variable" -> "GUC variables"?
> > >
> >
> > I think moving these variables together doesn't sound like a good idea
> > because logical_decoding_work_mem variable is defined with other
> > related variable. Also, if we are doing the last comment, I think that
> > will obviate the need for this.
> >
> > > ==
> > > src/backend/utils/misc/guc_tables.c
> > >
> > > @@ -4908,13 +4908,13 @@ struct config_enum ConfigureNamesEnum[] =
> > > },
> > >
> > > {
> > > - {"logical_decoding_mode", PGC_USERSET, DEVELOPER_OPTIONS,
> > > + {"logical_replication_mode", PGC_USERSET, DEVELOPER_OPTIONS,
> > > gettext_noop("Allows streaming or serializing each change in logical
> > > decoding."),
> > > NULL,
> > > GUC_NOT_IN_SAMPLE
> > > },
> > > - _decoding_mode,
> > > - LOGICAL_DECODING_MODE_BUFFERED, logical_decoding_mode_options,
> > > + _replication_mode,
> > > + LOGICAL_REP_MODE_BUFFERED, logical_replication_mode_options,
> > > NULL, NULL, NULL
> > > },
> > >
> > > That gettext_noop string seems incorrect. I think Kuroda-san
> > > previously reported the same, but then you replied it has been fixed
> > > already [1]
> > >
> > > > I felt the description seems not to be suitable for current behavior.
> > > > A short description should be like "Sets a behavior of logical
> > > > replication", and
> > > > further descriptions can be added in lond description.
> > > I adjusted the description here.
> > >
> > > But this doesn't look fixed to me. (??)
> > >
> >
> > Okay, so, how about the following for the 0001 patch:
> > short desc: Controls when to replicate each change.
> > long desc: On the publisher, it allows streaming or serializing each
> > change in logical decoding.
> >
>
> I have updated the patch accordingly and it looks good to me. I'll
> push this first patch early next week (Monday) unless there are more
> comments.
The patch looks good to me too. Thank you for the patch.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
RE: Perform streaming logical transactions by background workers and parallel apply
Dear Hou, Thank you for updating the patch! Followings are comments. 1. config.sgml ``` +the changes till logical_decoding_work_mem is reached. It can also be ``` I think it should be sandwiched by . 2. config.sgml ``` +On the publisher side, logical_replication_mode allows +allows streaming or serializing changes immediately in logical decoding. ``` Typo "allows allows" -> "allows" 3. test general You confirmed that the leader started to serialize changes, but did not ensure the endpoint. IIUC the parallel apply worker exits after applying serialized changes, and it is not tested yet. Can we add polling the log somewhere? 4. 015_stream.pl ``` +is($result, qq(15000), 'all changes are replayed from file') ``` The statement may be unclear because changes can be also replicated when streaming = on. How about: "parallel apply worker replayed all changes from file"? Best Regards, Hayato Kuroda FUJITSU LIMITED
RE: Perform streaming logical transactions by background workers and parallel apply
On Wednesday, January 25, 2023 7:30 AM Peter Smith
wrote:
>
> Here are my review comments for patch v87-0002.
Thanks for your comments.
> ==
> doc/src/sgml/config.sgml
>
> 1.
>
> -Allows streaming or serializing changes immediately in
> logical decoding.
> The allowed values of
> logical_replication_mode are
> -buffered and immediate. When
> set
> -to immediate, stream each change if
> +buffered and immediate.
> The default
> +is buffered.
> +
>
> I didn't think it was necessary to say “of logical_replication_mode”.
> IMO that much is already obvious because this is the first sentence of the
> description for logical_replication_mode.
>
Changed.
> ~~~
>
> 2.
> +
> +On the publisher side, it allows streaming or serializing changes
> +immediately in logical decoding. When set to
> +immediate, stream each change if
> streaming option (see optional parameters set by
> CREATE
> SUBSCRIPTION)
> is enabled, otherwise, serialize each change. When set to
> -buffered, which is the default, decoding will
> stream
> -or serialize changes when
> logical_decoding_work_mem
> -is reached.
> +buffered, decoding will stream or serialize
> changes
> +when logical_decoding_work_mem is
> reached.
>
>
> 2a.
> "it allows" --> "logical_replication_mode allows"
>
> 2b.
> "decoding" --> "the decoding"
Changed.
> ~~~
>
> 3.
> +
> +On the subscriber side, if streaming option is set
> +to parallel, this parameter also allows the leader
> +apply worker to send changes to the shared memory queue or to
> serialize
> +changes. When set to buffered, the leader sends
> +changes to parallel apply workers via shared memory queue. When
> set to
> +immediate, the leader serializes all changes to
> +files and notifies the parallel apply workers to read and apply them
> at
> +the end of the transaction.
> +
>
> "this parameter also allows" --> "logical_replication_mode also allows"
Changed.
> ~~~
>
> 4.
>
> This parameter is intended to be used to test logical decoding and
> replication of large transactions for which otherwise we need to
> generate the changes till
> logical_decoding_work_mem
> -is reached.
> +is reached. Moreover, this can also be used to test the transmission
> of
> +changes between the leader and parallel apply workers.
>
>
> "Moreover, this can also" --> "It can also"
>
> I am wondering would this sentence be better put at the top of the GUC
> description. So then the first paragraph becomes like this:
>
>
> SUGGESTION (I've also added another sentence "The effect of...")
>
> The allowed values are buffered and immediate. The default is buffered. This
> parameter is intended to be used to test logical decoding and replication of
> large
> transactions for which otherwise we need to generate the changes till
> logical_decoding_work_mem is reached. It can also be used to test the
> transmission of changes between the leader and parallel apply workers. The
> effect of logical_replication_mode is different for the publisher and
> subscriber:
>
> On the publisher side...
>
> On the subscriber side...
I think your suggestion makes sense, so changed as suggested.
> ==
> .../replication/logical/applyparallelworker.c
>
> 5.
> + /*
> + * In immeidate mode, directly return false so that we can switch to
> + * PARTIAL_SERIALIZE mode and serialize remaining changes to files.
> + */
> + if (logical_replication_mode == LOGICAL_REP_MODE_IMMEDIATE) return
> + false;
>
> Typo "immediate"
>
> Also, I felt "directly" is not needed. "return false" and "directly return
> false" is the
> same.
>
> SUGGESTION
> Using ‘immediate’ mode returns false to cause a switch to PARTIAL_SERIALIZE
> mode so that the remaining changes will be serialized.
Changed.
> ==
> src/backend/utils/misc/guc_tables.c
>
> 6.
> {
> {"logical_replication_mode", PGC_USERSET, DEVELOPER_OPTIONS,
> - gettext_noop("Allows streaming or serializing each change in logical
> decoding."),
> - NULL,
> + gettext_noop("Controls the behavior of logical replication publisher
> and subscriber"),
> + gettext_noop("If set to immediate, on the publisher side, it "
> + "allows streaming or serializing each change in "
> + "logical decoding. On the subscriber side, in "
> + "parallel streaming mode, it allows the leader apply "
> + "worker to serialize changes to files and notifies "
> + "the parallel apply workers to read and apply them at "
> + "the end of the transaction."),
> GUC_NOT_IN_SAMPLE
> },
>
> 6a. short description
>
> User PoV behaviour should be the same. Instead, maybe say "controls the
> internal behavior" or something like that?
Changed to "internal behavior xxx"
> ~
>
> 6b. long description
>
RE: Perform streaming logical transactions by background workers and parallel apply
Dear Amit, > > I have updated the patch accordingly and it looks good to me. I'll > push this first patch early next week (Monday) unless there are more > comments. Thanks for updating. I checked v88-0001 and I have no objection. LGTM. Best Regards, Hayato Kuroda FUJITSU LIMITED
Re: Perform streaming logical transactions by background workers and parallel apply
On Wed, Jan 25, 2023 at 10:05 AM Amit Kapila wrote:
>
> On Wed, Jan 25, 2023 at 3:15 AM Peter Smith wrote:
> >
> > 1.
> > @@ -210,7 +210,7 @@ int logical_decoding_work_mem;
> > static const Size max_changes_in_memory = 4096; /* XXX for restore only */
> >
> > /* GUC variable */
> > -int logical_decoding_mode = LOGICAL_DECODING_MODE_BUFFERED;
> > +int logical_replication_mode = LOGICAL_REP_MODE_BUFFERED;
> >
> >
> > I noticed that the comment /* GUC variable */ is currently only above
> > the logical_replication_mode, but actually logical_decoding_work_mem
> > is a GUC variable too. Maybe this should be rearranged somehow then
> > change the comment "GUC variable" -> "GUC variables"?
> >
>
> I think moving these variables together doesn't sound like a good idea
> because logical_decoding_work_mem variable is defined with other
> related variable. Also, if we are doing the last comment, I think that
> will obviate the need for this.
>
> > ==
> > src/backend/utils/misc/guc_tables.c
> >
> > @@ -4908,13 +4908,13 @@ struct config_enum ConfigureNamesEnum[] =
> > },
> >
> > {
> > - {"logical_decoding_mode", PGC_USERSET, DEVELOPER_OPTIONS,
> > + {"logical_replication_mode", PGC_USERSET, DEVELOPER_OPTIONS,
> > gettext_noop("Allows streaming or serializing each change in logical
> > decoding."),
> > NULL,
> > GUC_NOT_IN_SAMPLE
> > },
> > - _decoding_mode,
> > - LOGICAL_DECODING_MODE_BUFFERED, logical_decoding_mode_options,
> > + _replication_mode,
> > + LOGICAL_REP_MODE_BUFFERED, logical_replication_mode_options,
> > NULL, NULL, NULL
> > },
> >
> > That gettext_noop string seems incorrect. I think Kuroda-san
> > previously reported the same, but then you replied it has been fixed
> > already [1]
> >
> > > I felt the description seems not to be suitable for current behavior.
> > > A short description should be like "Sets a behavior of logical
> > > replication", and
> > > further descriptions can be added in lond description.
> > I adjusted the description here.
> >
> > But this doesn't look fixed to me. (??)
> >
>
> Okay, so, how about the following for the 0001 patch:
> short desc: Controls when to replicate each change.
> long desc: On the publisher, it allows streaming or serializing each
> change in logical decoding.
>
I have updated the patch accordingly and it looks good to me. I'll
push this first patch early next week (Monday) unless there are more
comments.
--
With Regards,
Amit Kapila.
v88-0001-Rename-GUC-logical_decoding_mode-to-logical_repl.patch
Description: Binary data
Re: Perform streaming logical transactions by background workers and parallel apply
On Wed, Jan 25, 2023 at 3:15 AM Peter Smith wrote:
>
> 1.
> @@ -210,7 +210,7 @@ int logical_decoding_work_mem;
> static const Size max_changes_in_memory = 4096; /* XXX for restore only */
>
> /* GUC variable */
> -int logical_decoding_mode = LOGICAL_DECODING_MODE_BUFFERED;
> +int logical_replication_mode = LOGICAL_REP_MODE_BUFFERED;
>
>
> I noticed that the comment /* GUC variable */ is currently only above
> the logical_replication_mode, but actually logical_decoding_work_mem
> is a GUC variable too. Maybe this should be rearranged somehow then
> change the comment "GUC variable" -> "GUC variables"?
>
I think moving these variables together doesn't sound like a good idea
because logical_decoding_work_mem variable is defined with other
related variable. Also, if we are doing the last comment, I think that
will obviate the need for this.
> ==
> src/backend/utils/misc/guc_tables.c
>
> @@ -4908,13 +4908,13 @@ struct config_enum ConfigureNamesEnum[] =
> },
>
> {
> - {"logical_decoding_mode", PGC_USERSET, DEVELOPER_OPTIONS,
> + {"logical_replication_mode", PGC_USERSET, DEVELOPER_OPTIONS,
> gettext_noop("Allows streaming or serializing each change in logical
> decoding."),
> NULL,
> GUC_NOT_IN_SAMPLE
> },
> - _decoding_mode,
> - LOGICAL_DECODING_MODE_BUFFERED, logical_decoding_mode_options,
> + _replication_mode,
> + LOGICAL_REP_MODE_BUFFERED, logical_replication_mode_options,
> NULL, NULL, NULL
> },
>
> That gettext_noop string seems incorrect. I think Kuroda-san
> previously reported the same, but then you replied it has been fixed
> already [1]
>
> > I felt the description seems not to be suitable for current behavior.
> > A short description should be like "Sets a behavior of logical
> > replication", and
> > further descriptions can be added in lond description.
> I adjusted the description here.
>
> But this doesn't look fixed to me. (??)
>
Okay, so, how about the following for the 0001 patch:
short desc: Controls when to replicate each change.
long desc: On the publisher, it allows streaming or serializing each
change in logical decoding.
Then we can extend it as follows for the 0002 patch:
Controls when to replicate or apply each change
On the publisher, it allows streaming or serializing each change in
logical decoding. On the subscriber, it allows serialization of all
changes to files and notifies the parallel apply workers to read and
apply them at the end of the transaction.
> ==
> src/include/replication/reorderbuffer.h
>
> 3.
> @@ -18,14 +18,14 @@
> #include "utils/timestamp.h"
>
> extern PGDLLIMPORT int logical_decoding_work_mem;
> -extern PGDLLIMPORT int logical_decoding_mode;
> +extern PGDLLIMPORT int logical_replication_mode;
>
> Probably here should also be a comment to say "/* GUC variables */"
>
Okay, we can do this.
--
With Regards,
Amit Kapila.
Re: Perform streaming logical transactions by background workers and parallel apply
Here are my review comments for patch v87-0002.
==
doc/src/sgml/config.sgml
1.
-Allows streaming or serializing changes immediately in
logical decoding.
The allowed values of logical_replication_mode are
-buffered and immediate. When set
-to immediate, stream each change if
+buffered and immediate.
The default
+is buffered.
+
I didn't think it was necessary to say “of logical_replication_mode”.
IMO that much is already obvious because this is the first sentence of
the description for logical_replication_mode.
(see also review comment #4)
~~~
2.
+
+On the publisher side, it allows streaming or serializing changes
+immediately in logical decoding. When set to
+immediate, stream each change if
streaming option (see optional parameters set by
CREATE
SUBSCRIPTION)
is enabled, otherwise, serialize each change. When set to
-buffered, which is the default, decoding will stream
-or serialize changes when logical_decoding_work_mem
-is reached.
+buffered, decoding will stream or serialize changes
+when logical_decoding_work_mem is reached.
2a.
"it allows" --> "logical_replication_mode allows"
2b.
"decoding" --> "the decoding"
~~~
3.
+
+On the subscriber side, if streaming option is set
+to parallel, this parameter also allows the leader
+apply worker to send changes to the shared memory queue or to serialize
+changes. When set to buffered, the leader sends
+changes to parallel apply workers via shared memory queue. When set to
+immediate, the leader serializes all changes to
+files and notifies the parallel apply workers to read and apply them at
+the end of the transaction.
+
"this parameter also allows" --> "logical_replication_mode also allows"
~~~
4.
This parameter is intended to be used to test logical decoding and
replication of large transactions for which otherwise we need to
generate the changes till logical_decoding_work_mem
-is reached.
+is reached. Moreover, this can also be used to test the transmission of
+changes between the leader and parallel apply workers.
"Moreover, this can also" --> "It can also"
I am wondering would this sentence be better put at the top of the GUC
description. So then the first paragraph becomes like this:
SUGGESTION (I've also added another sentence "The effect of...")
The allowed values are buffered and immediate. The default is
buffered. This parameter is intended to be used to test logical
decoding and replication of large transactions for which otherwise we
need to generate the changes till logical_decoding_work_mem is
reached. It can also be used to test the transmission of changes
between the leader and parallel apply workers. The effect of
logical_replication_mode is different for the publisher and
subscriber:
On the publisher side...
On the subscriber side...
==
.../replication/logical/applyparallelworker.c
5.
+ /*
+ * In immeidate mode, directly return false so that we can switch to
+ * PARTIAL_SERIALIZE mode and serialize remaining changes to files.
+ */
+ if (logical_replication_mode == LOGICAL_REP_MODE_IMMEDIATE)
+ return false;
Typo "immediate"
Also, I felt "directly" is not needed. "return false" and "directly
return false" is the same.
SUGGESTION
Using ‘immediate’ mode returns false to cause a switch to
PARTIAL_SERIALIZE mode so that the remaining changes will be
serialized.
==
src/backend/utils/misc/guc_tables.c
6.
{
{"logical_replication_mode", PGC_USERSET, DEVELOPER_OPTIONS,
- gettext_noop("Allows streaming or serializing each change in logical
decoding."),
- NULL,
+ gettext_noop("Controls the behavior of logical replication publisher
and subscriber"),
+ gettext_noop("If set to immediate, on the publisher side, it "
+ "allows streaming or serializing each change in "
+ "logical decoding. On the subscriber side, in "
+ "parallel streaming mode, it allows the leader apply "
+ "worker to serialize changes to files and notifies "
+ "the parallel apply workers to read and apply them at "
+ "the end of the transaction."),
GUC_NOT_IN_SAMPLE
},
6a. short description
User PoV behaviour should be the same. Instead, maybe say "controls
the internal behavior" or something like that?
~
6b. long description
IMO the long description shouldn’t mention ‘immediate’ mode first as it does.
BEFORE
If set to immediate, on the publisher side, ...
AFTER
On the publisher side, ...
--
Kind Regards,
Peter Smith.
Fujitsu Australia
Re: Perform streaming logical transactions by background workers and parallel apply
On Tue, Jan 24, 2023 at 11:49 PM houzj.f...@fujitsu.com
wrote:
>
...
>
> Sorry, the patch set was somehow attached twice. Here is the correct new
> version
> patch set which addressed all comments so far.
>
Here are my review comments for patch v87-0001.
==
src/backend/replication/logical/reorderbuffer.c
1.
@@ -210,7 +210,7 @@ int logical_decoding_work_mem;
static const Size max_changes_in_memory = 4096; /* XXX for restore only */
/* GUC variable */
-int logical_decoding_mode = LOGICAL_DECODING_MODE_BUFFERED;
+int logical_replication_mode = LOGICAL_REP_MODE_BUFFERED;
I noticed that the comment /* GUC variable */ is currently only above
the logical_replication_mode, but actually logical_decoding_work_mem
is a GUC variable too. Maybe this should be rearranged somehow then
change the comment "GUC variable" -> "GUC variables"?
==
src/backend/utils/misc/guc_tables.c
@@ -4908,13 +4908,13 @@ struct config_enum ConfigureNamesEnum[] =
},
{
- {"logical_decoding_mode", PGC_USERSET, DEVELOPER_OPTIONS,
+ {"logical_replication_mode", PGC_USERSET, DEVELOPER_OPTIONS,
gettext_noop("Allows streaming or serializing each change in logical
decoding."),
NULL,
GUC_NOT_IN_SAMPLE
},
- _decoding_mode,
- LOGICAL_DECODING_MODE_BUFFERED, logical_decoding_mode_options,
+ _replication_mode,
+ LOGICAL_REP_MODE_BUFFERED, logical_replication_mode_options,
NULL, NULL, NULL
},
That gettext_noop string seems incorrect. I think Kuroda-san
previously reported the same, but then you replied it has been fixed
already [1]
> I felt the description seems not to be suitable for current behavior.
> A short description should be like "Sets a behavior of logical replication",
> and
> further descriptions can be added in lond description.
I adjusted the description here.
But this doesn't look fixed to me. (??)
==
src/include/replication/reorderbuffer.h
3.
@@ -18,14 +18,14 @@
#include "utils/timestamp.h"
extern PGDLLIMPORT int logical_decoding_work_mem;
-extern PGDLLIMPORT int logical_decoding_mode;
+extern PGDLLIMPORT int logical_replication_mode;
Probably here should also be a comment to say "/* GUC variables */"
--
[1]
https://www.postgresql.org/message-id/OS0PR01MB5716AE9F095F9E7888987BC794C99%40OS0PR01MB5716.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
RE: Perform streaming logical transactions by background workers and parallel apply
Dear Hou, > Sorry, the patch set was somehow attached twice. Here is the correct new > version > patch set which addressed all comments so far. Thank you for updating the patch! I confirmed that All of my comments are addressed. One comment: In this test the rollback-prepared seems not to be executed. This is because serializations are finished while handling PREPARE message and the final state of transaction does not affect that, right? I think it may be helpful to add a one line comment. Best Regards, Hayato Kuroda FUJITSU LIMITED
RE: Perform streaming logical transactions by background workers and parallel apply
On Tuesday, January 24, 2023 8:47 PM Hou, Zhijie wrote: > > On Tuesday, January 24, 2023 3:19 PM Peter Smith > wrote: > > > > Here are some review comments for v86-0002 > > Sorry, the patch set was somehow attached twice. Here is the correct new version patch set which addressed all comments so far. Best Regards, Hou zj v87-0001-Rename-logical_decoding_mode-to-logical_replicat.patch Description: v87-0001-Rename-logical_decoding_mode-to-logical_replicat.patch v87-0002-Extend-the-logical_replication_mode-to-test-the-.patch Description: v87-0002-Extend-the-logical_replication_mode-to-test-the-.patch
RE: Perform streaming logical transactions by background workers and parallel apply
On Monday, January 23, 2023 8:34 PM Kuroda, Hayato wrote:
>
> Followings are my comments.
Thanks for your comments.
>
> 1. guc_tables.c
>
> ```
> static const struct config_enum_entry logical_decoding_mode_options[] = {
> - {"buffered", LOGICAL_DECODING_MODE_BUFFERED, false},
> - {"immediate", LOGICAL_DECODING_MODE_IMMEDIATE, false},
> + {"buffered", LOGICAL_REP_MODE_BUFFERED, false},
> + {"immediate", LOGICAL_REP_MODE_IMMEDIATE, false},
> {NULL, 0, false}
> };
> ```
>
> This struct should be also modified.
Modified.
>
> 2. guc_tables.c
>
>
> ```
> - {"logical_decoding_mode", PGC_USERSET,
> DEVELOPER_OPTIONS,
> + {"logical_replication_mode", PGC_USERSET,
> + DEVELOPER_OPTIONS,
> gettext_noop("Allows streaming or serializing each
> change in logical decoding."),
> NULL,
> ```
>
> I felt the description seems not to be suitable for current behavior.
> A short description should be like "Sets a behavior of logical replication",
> and
> further descriptions can be added in lond description.
I adjusted the description here.
> 3. config.sgml
>
> ```
>
> This parameter is intended to be used to test logical decoding and
> replication of large transactions for which otherwise we need to
> generate the changes till
> logical_decoding_work_mem
> is reached.
>
> ```
>
> I understood that this part described the usage of the parameter. How about
> adding a statement like:
>
> " Moreover, this can be also used to test the message passing between the
> leader and parallel apply workers."
Added.
> 4. 015_stream.pl
>
> ```
> +# Ensure that the messages are serialized.
> ```
>
> In other parts "changes" are used instead of "messages". Can you change the
> word?
Changed.
Best Regards,
Hou zj
RE: Perform streaming logical transactions by background workers and parallel apply
On Tuesday, January 24, 2023 11:43 AM Peter Smith wrote:
>
> Here are my review comments for patch v86-0001.
Thanks for your comments.
>
>
> ==
> Commit message
>
> 2.
> Since we may extend the developer option logical_decoding_mode to to test the
> parallel apply of large transaction on subscriber, rename this option to
> logical_replication_mode to make it easier to understand.
>
> ~
>
> 2a
> typo "to to"
>
> typo "large transaction on subscriber" --> "large transactions on the
> subscriber"
>
> ~
>
> 2b.
> IMO better to rephrase the whole paragraph like shown below.
>
> SUGGESTION
>
> Rename the developer option 'logical_decoding_mode' to the more flexible
> name 'logical_replication_mode' because doing so will make it easier to extend
> this option in future to help test other areas of logical replication.
Changed.
> ==
> doc/src/sgml/config.sgml
>
> 3.
> Allows streaming or serializing changes immediately in logical decoding. The
> allowed values of logical_replication_mode are buffered and immediate. When
> set to immediate, stream each change if streaming option (see optional
> parameters set by CREATE SUBSCRIPTION) is enabled, otherwise, serialize each
> change. When set to buffered, which is the default, decoding will stream or
> serialize changes when logical_decoding_work_mem is reached.
>
> ~
>
> IMO it's more clear to say the default when the options are first mentioned.
> So I
> suggested removing the "which is the default" part, and instead saying:
>
> BEFORE
> The allowed values of logical_replication_mode are buffered and immediate.
>
> AFTER
> The allowed values of logical_replication_mode are buffered and immediate. The
> default is buffered.
I included this change in the 0002 patch.
> ==
> src/backend/utils/misc/guc_tables.c
>
> 4.
> @@ -396,8 +396,8 @@ static const struct config_enum_entry
> ssl_protocol_versions_info[] = { };
>
> static const struct config_enum_entry logical_decoding_mode_options[] = {
> - {"buffered", LOGICAL_DECODING_MODE_BUFFERED, false},
> - {"immediate", LOGICAL_DECODING_MODE_IMMEDIATE, false},
> + {"buffered", LOGICAL_REP_MODE_BUFFERED, false}, {"immediate",
> + LOGICAL_REP_MODE_IMMEDIATE, false},
> {NULL, 0, false}
> };
>
> I noticed this array is still called "logical_decoding_mode_options".
> Was that deliberate?
No, I didn't notice this one. Changed.
Best Regards,
Hou zj
RE: Perform streaming logical transactions by background workers and parallel apply
On Tuesday, January 24, 2023 3:19 PM Peter Smith wrote:
>
> Here are some review comments for v86-0002
>
> ==
> Commit message
>
> 1.
> Use the use the existing developer option logical_replication_mode to test the
> parallel apply of large transaction on subscriber.
>
> ~
>
> Typo “Use the use the”
>
> SUGGESTION (rewritten)
> Give additional functionality to the existing developer option
> 'logical_replication_mode' to help test parallel apply of large transactions
> on the
> subscriber.
Changed.
> ~~~
>
> 2.
> Maybe that commit message should also say extra TAP tests that have been
> added to exercise the serialization part of the parallel apply?
Added.
> BTW – I can see the TAP tests are testing full serialization (when the GUC is
> 'immediate') but I not sure how is "partial" serialization (when it has to
> switch
> halfway from shmem to files) being tested.
The new tests are intended to test most of new code patch for partial
serialization by doing it from the beginning. Later, if required, we can add
different tests for it.
>
> ==
> doc/src/sgml/config.sgml
>
> 3.
> Allows streaming or serializing changes immediately in logical decoding. The
> allowed values of logical_replication_mode are buffered and immediate. When
> set to immediate, stream each change if streaming option (see optional
> parameters set by CREATE SUBSCRIPTION) is enabled, otherwise, serialize each
> change. When set to buffered, which is the default, decoding will stream or
> serialize changes when logical_decoding_work_mem is reached.
> On the subscriber side, if streaming option is set to parallel, this
> parameter also
> allows the leader apply worker to send changes to the shared memory queue or
> to serialize changes. When set to buffered, the leader sends changes to
> parallel
> apply workers via shared memory queue. When set to immediate, the leader
> serializes all changes to files and notifies the parallel apply workers to
> read and
> apply them at the end of the transaction.
>
> ~
>
> Because now this same developer GUC affects both the publisher side and the
> subscriber side differently IMO this whole description should be re-structured
> accordingly.
>
> SUGGESTION (something like)
>
> The allowed values of logical_replication_mode are buffered and immediate. The
> default is buffered.
>
> On the publisher side, ...
>
> On the subscriber side, ...
Changed.
>
> ~~~
>
> 4.
> This parameter is intended to be used to test logical decoding and
> replication of
> large transactions for which otherwise we need to generate the changes till
> logical_decoding_work_mem is reached.
>
> ~
>
> Maybe this paragraph needs rewording or moving. e.g. Isn't that misleading
> now? Although this might be an explanation for the publisher side, it does not
> seem relevant to the subscriber side's behaviour.
Adjusted the description here.
>
> ==
> .../replication/logical/applyparallelworker.c
>
> 5.
> @ -1149,6 +1149,9 @@ pa_send_data(ParallelApplyWorkerInfo *winfo, Size
> nbytes, const void *data)
> Assert(!IsTransactionState());
> Assert(!winfo->serialize_changes);
>
> + if (logical_replication_mode == LOGICAL_REP_MODE_IMMEDIATE) return
> + false;
> +
>
> I felt that code should have some comment, even if it is just something quite
> basic like "/* For developer testing */"
Added.
>
> ==
> .../t/018_stream_subxact_abort.pl
>
> 6.
> +# Clean up test data from the environment.
> +$node_publisher->safe_psql('postgres', "TRUNCATE TABLE test_tab_2");
> +$node_publisher->wait_for_catchup($appname);
>
> Is it necessary to TRUNCATE the table here? If everything is working shouldn't
> the data be rolled back anyway?
I think it's unnecessary, so removed.
>
> ~~~
>
> 7.
> +$node_publisher->safe_psql(
> + 'postgres', q{
> + BEGIN;
> + INSERT INTO test_tab_2 values(1);
> + SAVEPOINT sp;
> + INSERT INTO test_tab_2 values(1);
> + ROLLBACK TO sp;
> + COMMIT;
> + });
>
> Perhaps this should insert 2 different values so then the verification code
> can
> check the correct value remains instead of just checking COUNT(*)?
I think testing the count should be ok as the nearby testcases are
also checking the count.
Best regards,
Hou zj
v87-0001-Rename-logical_decoding_mode-to-logical_replicat.patch
Description: v87-0001-Rename-logical_decoding_mode-to-logical_replicat.patch
v87-0002-Extend-the-logical_replication_mode-to-test-the-.patch
Description: v87-0002-Extend-the-logical_replication_mode-to-test-the-.patch
v87-0002-Extend-the-logical_replication_mode-to-test-the-.patch
Description: v87-0002-Extend-the-logical_replication_mode-to-test-the-.patch
v87-0001-Rename-logical_decoding_mode-to-logical_replicat.patch
Description: v87-0001-Rename-logical_decoding_mode-to-logical_replicat.patch
Re: Perform streaming logical transactions by background workers and parallel apply
Here are some review comments for v86-0002
==
Commit message
1.
Use the use the existing developer option logical_replication_mode to test the
parallel apply of large transaction on subscriber.
~
Typo “Use the use the”
SUGGESTION (rewritten)
Give additional functionality to the existing developer option
'logical_replication_mode' to help test parallel apply of large
transactions on the subscriber.
~~~
2.
Maybe that commit message should also say extra TAP tests that have
been added to exercise the serialization part of the parallel apply?
BTW – I can see the TAP tests are testing full serialization (when the
GUC is 'immediate') but I not sure how is "partial" serialization
(when it has to switch halfway from shmem to files) being tested.
==
doc/src/sgml/config.sgml
3.
Allows streaming or serializing changes immediately in logical
decoding. The allowed values of logical_replication_mode are buffered
and immediate. When set to immediate, stream each change if streaming
option (see optional parameters set by CREATE SUBSCRIPTION) is
enabled, otherwise, serialize each change. When set to buffered, which
is the default, decoding will stream or serialize changes when
logical_decoding_work_mem is reached.
On the subscriber side, if streaming option is set to parallel, this
parameter also allows the leader apply worker to send changes to the
shared memory queue or to serialize changes. When set to buffered, the
leader sends changes to parallel apply workers via shared memory
queue. When set to immediate, the leader serializes all changes to
files and notifies the parallel apply workers to read and apply them
at the end of the transaction.
~
Because now this same developer GUC affects both the publisher side
and the subscriber side differently IMO this whole description should
be re-structured accordingly.
SUGGESTION (something like)
The allowed values of logical_replication_mode are buffered and
immediate. The default is buffered.
On the publisher side, ...
On the subscriber side, ...
~~~
4.
This parameter is intended to be used to test logical decoding and
replication of large transactions for which otherwise we need to
generate the changes till logical_decoding_work_mem is reached.
~
Maybe this paragraph needs rewording or moving. e.g. Isn't that
misleading now? Although this might be an explanation for the
publisher side, it does not seem relevant to the subscriber side's
behaviour.
==
.../replication/logical/applyparallelworker.c
5.
@ -1149,6 +1149,9 @@ pa_send_data(ParallelApplyWorkerInfo *winfo, Size
nbytes, const void *data)
Assert(!IsTransactionState());
Assert(!winfo->serialize_changes);
+ if (logical_replication_mode == LOGICAL_REP_MODE_IMMEDIATE)
+ return false;
+
I felt that code should have some comment, even if it is just
something quite basic like "/* For developer testing */"
==
.../t/018_stream_subxact_abort.pl
6.
+# Clean up test data from the environment.
+$node_publisher->safe_psql('postgres', "TRUNCATE TABLE test_tab_2");
+$node_publisher->wait_for_catchup($appname);
Is it necessary to TRUNCATE the table here? If everything is working
shouldn't the data be rolled back anyway?
~~~
7.
+$node_publisher->safe_psql(
+ 'postgres', q{
+ BEGIN;
+ INSERT INTO test_tab_2 values(1);
+ SAVEPOINT sp;
+ INSERT INTO test_tab_2 values(1);
+ ROLLBACK TO sp;
+ COMMIT;
+ });
Perhaps this should insert 2 different values so then the verification
code can check the correct value remains instead of just checking
COUNT(*)?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
Re: Perform streaming logical transactions by background workers and parallel apply
On Tue, Jan 24, 2023 at 9:13 AM Peter Smith wrote: > > 1. > > IIUC the GUC name was made generic 'logical_replication_mode' so that > multiple developer GUCs are not needed later. > > But IMO those current option values (buffered/immediate) for that GUC > are maybe a bit too generic. Perhaps in future, we might want more > granular control than that allows. e.g. I can imagine there might be > multiple different meanings for what "buffered" means. If there is any > chance of the generic values being problematic later then maybe they > should be made more specific up-front. > > e.g. maybe like: > logical_replication_mode = buffered_decoding > logical_replication_mode = immediate_decoding > For now, it seems the meaning of buffered/immediate suits our debugging and test needs for publisher/subscriber. This is somewhat explained in Shveta's email [1]. I also think in the future this parameter could be extended for a different purpose but maybe it would be better to invent some new values at that time as things would be more clear. We could do what you are suggesting or in fact even use different values for publisher and subscriber but not really sure whether that will simplify the usage. [1] - https://www.postgresql.org/message-id/CAJpy0uDzddK_ZUsB2qBJUbW_ZODYGoUHTaS5pVcYE2tzATCVXQ%40mail.gmail.com -- With Regards, Amit Kapila.
Re: Perform streaming logical transactions by background workers and parallel apply
Here are my review comments for patch v86-0001.
==
General
1.
IIUC the GUC name was made generic 'logical_replication_mode' so that
multiple developer GUCs are not needed later.
But IMO those current option values (buffered/immediate) for that GUC
are maybe a bit too generic. Perhaps in future, we might want more
granular control than that allows. e.g. I can imagine there might be
multiple different meanings for what "buffered" means. If there is any
chance of the generic values being problematic later then maybe they
should be made more specific up-front.
e.g. maybe like:
logical_replication_mode = buffered_decoding
logical_replication_mode = immediate_decoding
Thoughts?
==
Commit message
2.
Since we may extend the developer option logical_decoding_mode to to test the
parallel apply of large transaction on subscriber, rename this option to
logical_replication_mode to make it easier to understand.
~
2a
typo "to to"
typo "large transaction on subscriber" --> "large transactions on the
subscriber"
~
2b.
IMO better to rephrase the whole paragraph like shown below.
SUGGESTION
Rename the developer option 'logical_decoding_mode' to the more
flexible name 'logical_replication_mode' because doing so will make it
easier to extend this option in future to help test other areas of
logical replication.
==
doc/src/sgml/config.sgml
3.
Allows streaming or serializing changes immediately in logical
decoding. The allowed values of logical_replication_mode are buffered
and immediate. When set to immediate, stream each change if streaming
option (see optional parameters set by CREATE SUBSCRIPTION) is
enabled, otherwise, serialize each change. When set to buffered, which
is the default, decoding will stream or serialize changes when
logical_decoding_work_mem is reached.
~
IMO it's more clear to say the default when the options are first
mentioned. So I suggested removing the "which is the default" part,
and instead saying:
BEFORE
The allowed values of logical_replication_mode are buffered and immediate.
AFTER
The allowed values of logical_replication_mode are buffered and
immediate. The default is buffered.
==
src/backend/utils/misc/guc_tables.c
4.
@@ -396,8 +396,8 @@ static const struct config_enum_entry
ssl_protocol_versions_info[] = {
};
static const struct config_enum_entry logical_decoding_mode_options[] = {
- {"buffered", LOGICAL_DECODING_MODE_BUFFERED, false},
- {"immediate", LOGICAL_DECODING_MODE_IMMEDIATE, false},
+ {"buffered", LOGICAL_REP_MODE_BUFFERED, false},
+ {"immediate", LOGICAL_REP_MODE_IMMEDIATE, false},
{NULL, 0, false}
};
I noticed this array is still called "logical_decoding_mode_options".
Was that deliberate?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
RE: Perform streaming logical transactions by background workers and parallel apply
Dear Hou,
Thank you for updating the patch! Followings are my comments.
1. guc_tables.c
```
static const struct config_enum_entry logical_decoding_mode_options[] = {
- {"buffered", LOGICAL_DECODING_MODE_BUFFERED, false},
- {"immediate", LOGICAL_DECODING_MODE_IMMEDIATE, false},
+ {"buffered", LOGICAL_REP_MODE_BUFFERED, false},
+ {"immediate", LOGICAL_REP_MODE_IMMEDIATE, false},
{NULL, 0, false}
};
```
This struct should be also modified.
2. guc_tables.c
```
- {"logical_decoding_mode", PGC_USERSET, DEVELOPER_OPTIONS,
+ {"logical_replication_mode", PGC_USERSET, DEVELOPER_OPTIONS,
gettext_noop("Allows streaming or serializing each
change in logical decoding."),
NULL,
```
I felt the description seems not to be suitable for current behavior.
A short description should be like "Sets a behavior of logical replication", and
further descriptions can be added in lond description.
3. config.sgml
```
This parameter is intended to be used to test logical decoding and
replication of large transactions for which otherwise we need to
generate the changes till logical_decoding_work_mem
is reached.
```
I understood that this part described the usage of the parameter. How about
adding
a statement like:
" Moreover, this can be also used to test the message passing between the leader
and parallel apply workers."
4. 015_stream.pl
```
+# Ensure that the messages are serialized.
```
In other parts "changes" are used instead of "messages". Can you change the
word?
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
RE: Perform streaming logical transactions by background workers and parallel apply
On Monday, January 23, 2023 11:17 AM Amit Kapila wrote: > > On Fri, Jan 20, 2023 at 11:48 AM Masahiko Sawada > wrote: > > > > > > > > Yet another way is to use the existing parameter logical_decode_mode > > > [1]. If the value of logical_decoding_mode is 'immediate', then we > > > can immediately switch to partial serialize mode. This will > > > eliminate the dependency on timing. The one argument against using > > > this is that it won't be as clear as a separate parameter like > > > 'stream_serialize_threshold' proposed by the patch but OTOH we > > > already have a few parameters that serve a different purpose when > > > used on the subscriber. For example, 'max_replication_slots' is used > > > to define the maximum number of replication slots on the publisher > > > and the maximum number of origins on subscribers. Similarly, > > > wal_retrieve_retry_interval' is used for different purposes on > > > subscriber and standby nodes. > > > > Using the existing parameter makes sense to me. But if we use > > logical_decoding_mode also on the subscriber, as Shveta Malik also > > suggested, probably it's better to rename it so as not to confuse. For > > example, logical_replication_mode or something. > > > > +1. Among the options discussed, this sounds better. OK, here is patch set which does the same. The first patch set only renames the GUC name, and the second patch uses the GUC to test the partial serialization. Best Regards, Hou zj v86-0002-Extend-the-logical_replication_mode-to-test-the-stre.patch Description: v86-0002-Extend-the-logical_replication_mode-to-test-the-stre.patch v86-0001-Rename-logical_decoding_mode-to-logical_replication_.patch Description: v86-0001-Rename-logical_decoding_mode-to-logical_replication_.patch
Re: Perform streaming logical transactions by background workers and parallel apply
On Mon, Jan 23, 2023 at 8:47 AM Amit Kapila wrote: > > On Fri, Jan 20, 2023 at 11:48 AM Masahiko Sawada > wrote: > > > > > > > > Yet another way is to use the existing parameter logical_decode_mode > > > [1]. If the value of logical_decoding_mode is 'immediate', then we can > > > immediately switch to partial serialize mode. This will eliminate the > > > dependency on timing. The one argument against using this is that it > > > won't be as clear as a separate parameter like > > > 'stream_serialize_threshold' proposed by the patch but OTOH we already > > > have a few parameters that serve a different purpose when used on the > > > subscriber. For example, 'max_replication_slots' is used to define the > > > maximum number of replication slots on the publisher and the maximum > > > number of origins on subscribers. Similarly, > > > wal_retrieve_retry_interval' is used for different purposes on > > > subscriber and standby nodes. > > > > Using the existing parameter makes sense to me. But if we use > > logical_decoding_mode also on the subscriber, as Shveta Malik also > > suggested, probably it's better to rename it so as not to confuse. For > > example, logical_replication_mode or something. > > > > +1. Among the options discussed, this sounds better. Yeah, this looks better option with the parameter name 'logical_replication_mode'. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: Perform streaming logical transactions by background workers and parallel apply
On Fri, Jan 20, 2023 at 11:48 AM Masahiko Sawada wrote: > > > > > Yet another way is to use the existing parameter logical_decode_mode > > [1]. If the value of logical_decoding_mode is 'immediate', then we can > > immediately switch to partial serialize mode. This will eliminate the > > dependency on timing. The one argument against using this is that it > > won't be as clear as a separate parameter like > > 'stream_serialize_threshold' proposed by the patch but OTOH we already > > have a few parameters that serve a different purpose when used on the > > subscriber. For example, 'max_replication_slots' is used to define the > > maximum number of replication slots on the publisher and the maximum > > number of origins on subscribers. Similarly, > > wal_retrieve_retry_interval' is used for different purposes on > > subscriber and standby nodes. > > Using the existing parameter makes sense to me. But if we use > logical_decoding_mode also on the subscriber, as Shveta Malik also > suggested, probably it's better to rename it so as not to confuse. For > example, logical_replication_mode or something. > +1. Among the options discussed, this sounds better. -- With Regards, Amit Kapila.
Re: Perform streaming logical transactions by background workers and parallel apply
On Thu, Jan 19, 2023 at 2:41 PM Amit Kapila wrote: > > On Wed, Jan 18, 2023 at 12:09 PM Amit Kapila wrote: > > > > On Fri, Jan 13, 2023 at 11:50 AM Peter Smith wrote: > > > > > > Here are some review comments for patch v79-0002. > > > > > > > So, this is about the latest v84-0001-Stop-extra-worker-if-GUC-was-changed. > > > > > > > > I feel this patch just adds more complexity for almost no gain: > > > - reducing the 'max_apply_workers_per_suibscription' seems not very > > > common in the first place. > > > - even when the GUC is reduced, at that point in time all the workers > > > might be in use so there may be nothing that can be immediately done. > > > - IIUC the excess workers (for a reduced GUC) are going to get freed > > > naturally anyway over time as more transactions are completed so the > > > pool size will reduce accordingly. > > > > > > > I am still not sure if it is worth pursuing this patch because of the > > above reasons. I don't think it would be difficult to add this even at > > a later point in time if we really see a use case for this. > > Sawada-San, IIRC, you raised this point. What do you think? > > > > The other point I am wondering is whether we can have a different way > > to test partial serialization apart from introducing another developer > > GUC (stream_serialize_threshold). One possibility could be that we can > > have a subscription option (parallel_send_timeout or something like > > that) with some default value (current_timeout used in the patch) > > which will be used only when streaming = parallel. Users may want to > > wait for more time before serialization starts depending on the > > workload (say when resource usage is high on a subscriber-side > > machine, or there are concurrent long-running transactions that can > > block parallel apply for a bit longer time). I know with this as well > > it may not be straightforward to test the functionality because we > > can't be sure how many changes would be required for a timeout to > > occur. This is just for brainstorming other options to test the > > partial serialization functionality. I can see parallel_send_timeout idea could be useful somewhat but I'm concerned users can tune this value properly. It's likely to indicate something abnormal or locking issues if LA waits to write data to the queue for more than 10s. Also, I think it doesn't make sense to allow users to set this timeout to a very low value. If switching to partial serialization mode early is useful in some cases, I think it's better to provide it as a new mode, such as streaming = 'parallel-file' etc. > > Apart from the above, we can also have a subscription option to > specify parallel_shm_queue_size (queue_size used to determine the > queue between the leader and parallel worker) and that can be used for > this purpose. Basically, configuring it to a smaller value can help in > reducing the test time but still, it will not eliminate the need for > dependency on timing we have to wait before switching to partial > serialize mode. I think this can be used in production as well to tune > the performance depending on workload. A parameter for the queue size is interesting but I agree it will not eliminate the need for dependency on timing. > > Yet another way is to use the existing parameter logical_decode_mode > [1]. If the value of logical_decoding_mode is 'immediate', then we can > immediately switch to partial serialize mode. This will eliminate the > dependency on timing. The one argument against using this is that it > won't be as clear as a separate parameter like > 'stream_serialize_threshold' proposed by the patch but OTOH we already > have a few parameters that serve a different purpose when used on the > subscriber. For example, 'max_replication_slots' is used to define the > maximum number of replication slots on the publisher and the maximum > number of origins on subscribers. Similarly, > wal_retrieve_retry_interval' is used for different purposes on > subscriber and standby nodes. Using the existing parameter makes sense to me. But if we use logical_decoding_mode also on the subscriber, as Shveta Malik also suggested, probably it's better to rename it so as not to confuse. For example, logical_replication_mode or something. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Re: Perform streaming logical transactions by background workers and parallel apply
On Thu, Jan 19, 2023 at 3:44 PM shveta malik wrote: > > On Thu, Jan 19, 2023 at 11:11 AM Amit Kapila wrote: > > > > On Wed, Jan 18, 2023 at 12:09 PM Amit Kapila > > wrote: > > > > > > On Fri, Jan 13, 2023 at 11:50 AM Peter Smith > > > wrote: > > > > > > > > Here are some review comments for patch v79-0002. > > > > > > > > > > So, this is about the latest > > > v84-0001-Stop-extra-worker-if-GUC-was-changed. > > > > > > > > > > > I feel this patch just adds more complexity for almost no gain: > > > > - reducing the 'max_apply_workers_per_suibscription' seems not very > > > > common in the first place. > > > > - even when the GUC is reduced, at that point in time all the workers > > > > might be in use so there may be nothing that can be immediately done. > > > > - IIUC the excess workers (for a reduced GUC) are going to get freed > > > > naturally anyway over time as more transactions are completed so the > > > > pool size will reduce accordingly. > > > > > > > > > > I am still not sure if it is worth pursuing this patch because of the > > > above reasons. I don't think it would be difficult to add this even at > > > a later point in time if we really see a use case for this. > > > Sawada-San, IIRC, you raised this point. What do you think? > > > > > > The other point I am wondering is whether we can have a different way > > > to test partial serialization apart from introducing another developer > > > GUC (stream_serialize_threshold). One possibility could be that we can > > > have a subscription option (parallel_send_timeout or something like > > > that) with some default value (current_timeout used in the patch) > > > which will be used only when streaming = parallel. Users may want to > > > wait for more time before serialization starts depending on the > > > workload (say when resource usage is high on a subscriber-side > > > machine, or there are concurrent long-running transactions that can > > > block parallel apply for a bit longer time). I know with this as well > > > it may not be straightforward to test the functionality because we > > > can't be sure how many changes would be required for a timeout to > > > occur. This is just for brainstorming other options to test the > > > partial serialization functionality. > > > > > > > Apart from the above, we can also have a subscription option to > > specify parallel_shm_queue_size (queue_size used to determine the > > queue between the leader and parallel worker) and that can be used for > > this purpose. Basically, configuring it to a smaller value can help in > > reducing the test time but still, it will not eliminate the need for > > dependency on timing we have to wait before switching to partial > > serialize mode. I think this can be used in production as well to tune > > the performance depending on workload. > > > > Yet another way is to use the existing parameter logical_decode_mode > > [1]. If the value of logical_decoding_mode is 'immediate', then we can > > immediately switch to partial serialize mode. This will eliminate the > > dependency on timing. The one argument against using this is that it > > won't be as clear as a separate parameter like > > 'stream_serialize_threshold' proposed by the patch but OTOH we already > > have a few parameters that serve a different purpose when used on the > > subscriber. For example, 'max_replication_slots' is used to define the > > maximum number of replication slots on the publisher and the maximum > > number of origins on subscribers. Similarly, > > wal_retrieve_retry_interval' is used for different purposes on > > subscriber and standby nodes. > > > > [1] - https://www.postgresql.org/docs/devel/runtime-config-developer.html > > > > -- > > With Regards, > > Amit Kapila. > > Hi Amit, > > On rethinking the complete model, what I feel is that the name > logical_decoding_mode is not something which defines modes of logical > decoding. We, I think, picked it based on logical_decoding_work_mem. > As per current implementation, the parameter 'logical_decoding_mode' > tells what happens when work-memory used by logical decoding reaches > its limit. So it is in-fact 'logicalrep_workmem_vacate_mode' or Minor correction in what I said earlier: As per current implementation, the parameter 'logical_decoding_mode' more or less tells how to deal with workmem i.e. to keep it buffering with txns until it reaches its limit or immediately vacate it. > 'logicalrep_trans_eviction_mode'. So if it is set to immediate, > meaning vacate the work-memory immediately or evict transactions > immediately. Add buffered means the reverse (i.e. keep on buffering > transactions until we reach a limit). Now coming to subscribers, we > can reuse the same parameter. On subscriber as well, shared-memory > queue could be considered as its workmem and thus the name > 'logicalrep_workmem_vacate_mode' might look more relevant. > > On publisher: > logicalrep_workmem_vacate_mode=immediate, buffered. > > On subscriber: >
Re: Perform streaming logical transactions by background workers and parallel apply
On Thu, Jan 19, 2023 at 11:11 AM Amit Kapila wrote: > > On Wed, Jan 18, 2023 at 12:09 PM Amit Kapila wrote: > > > > On Fri, Jan 13, 2023 at 11:50 AM Peter Smith wrote: > > > > > > Here are some review comments for patch v79-0002. > > > > > > > So, this is about the latest v84-0001-Stop-extra-worker-if-GUC-was-changed. > > > > > > > > I feel this patch just adds more complexity for almost no gain: > > > - reducing the 'max_apply_workers_per_suibscription' seems not very > > > common in the first place. > > > - even when the GUC is reduced, at that point in time all the workers > > > might be in use so there may be nothing that can be immediately done. > > > - IIUC the excess workers (for a reduced GUC) are going to get freed > > > naturally anyway over time as more transactions are completed so the > > > pool size will reduce accordingly. > > > > > > > I am still not sure if it is worth pursuing this patch because of the > > above reasons. I don't think it would be difficult to add this even at > > a later point in time if we really see a use case for this. > > Sawada-San, IIRC, you raised this point. What do you think? > > > > The other point I am wondering is whether we can have a different way > > to test partial serialization apart from introducing another developer > > GUC (stream_serialize_threshold). One possibility could be that we can > > have a subscription option (parallel_send_timeout or something like > > that) with some default value (current_timeout used in the patch) > > which will be used only when streaming = parallel. Users may want to > > wait for more time before serialization starts depending on the > > workload (say when resource usage is high on a subscriber-side > > machine, or there are concurrent long-running transactions that can > > block parallel apply for a bit longer time). I know with this as well > > it may not be straightforward to test the functionality because we > > can't be sure how many changes would be required for a timeout to > > occur. This is just for brainstorming other options to test the > > partial serialization functionality. > > > > Apart from the above, we can also have a subscription option to > specify parallel_shm_queue_size (queue_size used to determine the > queue between the leader and parallel worker) and that can be used for > this purpose. Basically, configuring it to a smaller value can help in > reducing the test time but still, it will not eliminate the need for > dependency on timing we have to wait before switching to partial > serialize mode. I think this can be used in production as well to tune > the performance depending on workload. > > Yet another way is to use the existing parameter logical_decode_mode > [1]. If the value of logical_decoding_mode is 'immediate', then we can > immediately switch to partial serialize mode. This will eliminate the > dependency on timing. The one argument against using this is that it > won't be as clear as a separate parameter like > 'stream_serialize_threshold' proposed by the patch but OTOH we already > have a few parameters that serve a different purpose when used on the > subscriber. For example, 'max_replication_slots' is used to define the > maximum number of replication slots on the publisher and the maximum > number of origins on subscribers. Similarly, > wal_retrieve_retry_interval' is used for different purposes on > subscriber and standby nodes. > > [1] - https://www.postgresql.org/docs/devel/runtime-config-developer.html > > -- > With Regards, > Amit Kapila. Hi Amit, On rethinking the complete model, what I feel is that the name logical_decoding_mode is not something which defines modes of logical decoding. We, I think, picked it based on logical_decoding_work_mem. As per current implementation, the parameter 'logical_decoding_mode' tells what happens when work-memory used by logical decoding reaches its limit. So it is in-fact 'logicalrep_workmem_vacate_mode' or 'logicalrep_trans_eviction_mode'. So if it is set to immediate, meaning vacate the work-memory immediately or evict transactions immediately. Add buffered means the reverse (i.e. keep on buffering transactions until we reach a limit). Now coming to subscribers, we can reuse the same parameter. On subscriber as well, shared-memory queue could be considered as its workmem and thus the name 'logicalrep_workmem_vacate_mode' might look more relevant. On publisher: logicalrep_workmem_vacate_mode=immediate, buffered. On subscriber: logicalrep_workmem_vacate_mode=stream_serialize (or if we want to keep immediate here too, that will also be fine). Thoughts? And I am assuming it is possible to change the GUC name before the coming release. If not, please let me know, we can brainstorm other ideas. thanks Shveta
Re: Perform streaming logical transactions by background workers and parallel apply
On Wed, Jan 18, 2023 at 12:09 PM Amit Kapila wrote: > > On Fri, Jan 13, 2023 at 11:50 AM Peter Smith wrote: > > > > Here are some review comments for patch v79-0002. > > > > So, this is about the latest v84-0001-Stop-extra-worker-if-GUC-was-changed. > > > > > I feel this patch just adds more complexity for almost no gain: > > - reducing the 'max_apply_workers_per_suibscription' seems not very > > common in the first place. > > - even when the GUC is reduced, at that point in time all the workers > > might be in use so there may be nothing that can be immediately done. > > - IIUC the excess workers (for a reduced GUC) are going to get freed > > naturally anyway over time as more transactions are completed so the > > pool size will reduce accordingly. > > > > I am still not sure if it is worth pursuing this patch because of the > above reasons. I don't think it would be difficult to add this even at > a later point in time if we really see a use case for this. > Sawada-San, IIRC, you raised this point. What do you think? > > The other point I am wondering is whether we can have a different way > to test partial serialization apart from introducing another developer > GUC (stream_serialize_threshold). One possibility could be that we can > have a subscription option (parallel_send_timeout or something like > that) with some default value (current_timeout used in the patch) > which will be used only when streaming = parallel. Users may want to > wait for more time before serialization starts depending on the > workload (say when resource usage is high on a subscriber-side > machine, or there are concurrent long-running transactions that can > block parallel apply for a bit longer time). I know with this as well > it may not be straightforward to test the functionality because we > can't be sure how many changes would be required for a timeout to > occur. This is just for brainstorming other options to test the > partial serialization functionality. > Apart from the above, we can also have a subscription option to specify parallel_shm_queue_size (queue_size used to determine the queue between the leader and parallel worker) and that can be used for this purpose. Basically, configuring it to a smaller value can help in reducing the test time but still, it will not eliminate the need for dependency on timing we have to wait before switching to partial serialize mode. I think this can be used in production as well to tune the performance depending on workload. Yet another way is to use the existing parameter logical_decode_mode [1]. If the value of logical_decoding_mode is 'immediate', then we can immediately switch to partial serialize mode. This will eliminate the dependency on timing. The one argument against using this is that it won't be as clear as a separate parameter like 'stream_serialize_threshold' proposed by the patch but OTOH we already have a few parameters that serve a different purpose when used on the subscriber. For example, 'max_replication_slots' is used to define the maximum number of replication slots on the publisher and the maximum number of origins on subscribers. Similarly, wal_retrieve_retry_interval' is used for different purposes on subscriber and standby nodes. [1] - https://www.postgresql.org/docs/devel/runtime-config-developer.html -- With Regards, Amit Kapila.
Re: Perform streaming logical transactions by background workers and parallel apply
On Fri, Jan 13, 2023 at 11:50 AM Peter Smith wrote: > > Here are some review comments for patch v79-0002. > So, this is about the latest v84-0001-Stop-extra-worker-if-GUC-was-changed. > == > > General > > 1. > > I saw that earlier in this thread Hou-san [1] and Amit [2] also seemed > to say there is not much point for this patch. > > So I wanted to +1 that same opinion. > > I feel this patch just adds more complexity for almost no gain: > - reducing the 'max_apply_workers_per_suibscription' seems not very > common in the first place. > - even when the GUC is reduced, at that point in time all the workers > might be in use so there may be nothing that can be immediately done. > - IIUC the excess workers (for a reduced GUC) are going to get freed > naturally anyway over time as more transactions are completed so the > pool size will reduce accordingly. > I am still not sure if it is worth pursuing this patch because of the above reasons. I don't think it would be difficult to add this even at a later point in time if we really see a use case for this. Sawada-San, IIRC, you raised this point. What do you think? The other point I am wondering is whether we can have a different way to test partial serialization apart from introducing another developer GUC (stream_serialize_threshold). One possibility could be that we can have a subscription option (parallel_send_timeout or something like that) with some default value (current_timeout used in the patch) which will be used only when streaming = parallel. Users may want to wait for more time before serialization starts depending on the workload (say when resource usage is high on a subscriber-side machine, or there are concurrent long-running transactions that can block parallel apply for a bit longer time). I know with this as well it may not be straightforward to test the functionality because we can't be sure how many changes would be required for a timeout to occur. This is just for brainstorming other options to test the partial serialization functionality. Thoughts? -- With Regards, Amit Kapila.
RE: Perform streaming logical transactions by background workers and parallel apply
On Wed, Jan 18, 2023 12:36 PM Amit Kapila wrote: > On Tue, Jan 17, 2023 at 8:07 PM Masahiko Sawada > wrote: > > > > On Tue, Jan 17, 2023 at 6:14 PM houzj.f...@fujitsu.com > > wrote: > > > > > > On Tuesday, January 17, 2023 2:46 PM Masahiko Sawada > wrote: > > > > > > > > On Tue, Jan 17, 2023 at 12:37 PM houzj.f...@fujitsu.com > > > > wrote: > > > > I'm slightly concerned that there could be overhead of executing > > > > GetLeaderApplyWorkerPid () for every backend process except for parallel > > > > query workers. The number of such backends could be large and > > > > GetLeaderApplyWorkerPid() acquires the lwlock. For example, does it > make > > > > sense to check (st_backendType == B_BG_WORKER) before calling > > > > GetLeaderApplyWorkerPid()? Or it might not be a problem since it's > > > > LogicalRepWorkerLock which is not likely to be contended. > > > > > > Thanks for the comment and I think your suggestion makes sense. > > > I have added the check before getting the leader pid. Here is the new > version patch. > > > > Thank you for updating the patch. Looks good to me. > > > > Pushed. Rebased and attach remaining patches for reviewing. Regards, Wang Wei v84-0001-Stop-extra-worker-if-GUC-was-changed.patch Description: v84-0001-Stop-extra-worker-if-GUC-was-changed.patch v84-0002-Add-GUC-stream_serialize_threshold-and-test-seri.patch Description: v84-0002-Add-GUC-stream_serialize_threshold-and-test-seri.patch v84-0003-Retry-to-apply-streaming-xact-only-in-apply-work.patch Description: v84-0003-Retry-to-apply-streaming-xact-only-in-apply-work.patch
Re: Perform streaming logical transactions by background workers and parallel apply
On Tue, Jan 17, 2023 at 8:07 PM Masahiko Sawada wrote: > > On Tue, Jan 17, 2023 at 6:14 PM houzj.f...@fujitsu.com > wrote: > > > > On Tuesday, January 17, 2023 2:46 PM Masahiko Sawada > > wrote: > > > > > > On Tue, Jan 17, 2023 at 12:37 PM houzj.f...@fujitsu.com > > > wrote: > > > I'm slightly concerned that there could be overhead of executing > > > GetLeaderApplyWorkerPid () for every backend process except for parallel > > > query workers. The number of such backends could be large and > > > GetLeaderApplyWorkerPid() acquires the lwlock. For example, does it make > > > sense to check (st_backendType == B_BG_WORKER) before calling > > > GetLeaderApplyWorkerPid()? Or it might not be a problem since it's > > > LogicalRepWorkerLock which is not likely to be contended. > > > > Thanks for the comment and I think your suggestion makes sense. > > I have added the check before getting the leader pid. Here is the new > > version patch. > > Thank you for updating the patch. Looks good to me. > Pushed. -- With Regards, Amit Kapila.
Re: Perform streaming logical transactions by background workers and parallel apply
On Tue, Jan 17, 2023 at 6:14 PM houzj.f...@fujitsu.com
wrote:
>
> On Tuesday, January 17, 2023 2:46 PM Masahiko Sawada
> wrote:
> >
> > On Tue, Jan 17, 2023 at 12:37 PM houzj.f...@fujitsu.com
> > wrote:
> > > Attach the new version 0001 patch which addressed all other comments.
> > >
> >
> > Thank you for updating the patch. Here is one comment:
> >
> > @@ -426,14 +427,24 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
> >
> > /*
> > * Show the leader only for active parallel
> > workers. This
> > -* leaves the field as NULL for the
> > leader of a parallel
> > -* group.
> > +* leaves the field as NULL for the
> > leader of a parallel group
> > +* or the leader of parallel apply workers.
> > */
> > if (leader && leader->pid !=
> > beentry->st_procpid)
> > {
> > values[28] =
> > Int32GetDatum(leader->pid);
> > nulls[28] = false;
> > }
> > + else
> > + {
> > + int
> > leader_pid = GetLeaderApplyWorkerPid(beentry->st_procpid);
> > +
> > + if (leader_pid != InvalidPid)
> > + {
> > + values[28] =
> > Int32GetDatum(leader_pid);
> > + nulls[28] = false;
> > + }
> > + }
> > }
> >
> > I'm slightly concerned that there could be overhead of executing
> > GetLeaderApplyWorkerPid () for every backend process except for parallel
> > query workers. The number of such backends could be large and
> > GetLeaderApplyWorkerPid() acquires the lwlock. For example, does it make
> > sense to check (st_backendType == B_BG_WORKER) before calling
> > GetLeaderApplyWorkerPid()? Or it might not be a problem since it's
> > LogicalRepWorkerLock which is not likely to be contended.
>
> Thanks for the comment and I think your suggestion makes sense.
> I have added the check before getting the leader pid. Here is the new version
> patch.
Thank you for updating the patch. Looks good to me.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
RE: Perform streaming logical transactions by background workers and parallel apply
On Tuesday, January 17, 2023 12:34 PM shveta malik wrote: > > On Tue, Jan 17, 2023 at 9:07 AM houzj.f...@fujitsu.com > wrote: > > > > On Tuesday, January 17, 2023 11:32 AM Peter Smith > wrote: > > > OK. I didn't know there was another header convention that you were > > > following. > > > In that case, it is fine to leave the name as-is. > > > > Thanks for confirming! > > > > Attach the new version 0001 patch which addressed all other comments. > > > > Best regards, > > Hou zj > > Hello Hou-san, > > 1. Do we need to extend test-cases to review the leader_pid column in pg_stats > tables? Thanks for the comments. We currently don't have any tests for the view, so I feel we can extend them later as a separate patch. > 2. Do we need to follow the naming convention for > 'GetLeaderApplyWorkerPid' like other functions in the same file which starts > with 'logicalrep_' We have agreed [1] to follow the naming convention for functions in logicallauncher.h which are mainly used for other modules. [1] https://www.postgresql.org/message-id/CAHut%2BPtgj%3DDY8F1cMBRUxsZtq2-faW%3D%3D5-dSuHSPJGx1a_vBFQ%40mail.gmail.com Best regards, Hou zj
RE: Perform streaming logical transactions by background workers and parallel apply
On Tuesday, January 17, 2023 2:46 PM Masahiko Sawada
wrote:
>
> On Tue, Jan 17, 2023 at 12:37 PM houzj.f...@fujitsu.com
> wrote:
> > Attach the new version 0001 patch which addressed all other comments.
> >
>
> Thank you for updating the patch. Here is one comment:
>
> @@ -426,14 +427,24 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
>
> /*
> * Show the leader only for active parallel
> workers. This
> -* leaves the field as NULL for the
> leader of a parallel
> -* group.
> +* leaves the field as NULL for the
> leader of a parallel group
> +* or the leader of parallel apply workers.
> */
> if (leader && leader->pid !=
> beentry->st_procpid)
> {
> values[28] =
> Int32GetDatum(leader->pid);
> nulls[28] = false;
> }
> + else
> + {
> + int
> leader_pid = GetLeaderApplyWorkerPid(beentry->st_procpid);
> +
> + if (leader_pid != InvalidPid)
> + {
> + values[28] =
> Int32GetDatum(leader_pid);
> + nulls[28] = false;
> + }
> + }
> }
>
> I'm slightly concerned that there could be overhead of executing
> GetLeaderApplyWorkerPid () for every backend process except for parallel
> query workers. The number of such backends could be large and
> GetLeaderApplyWorkerPid() acquires the lwlock. For example, does it make
> sense to check (st_backendType == B_BG_WORKER) before calling
> GetLeaderApplyWorkerPid()? Or it might not be a problem since it's
> LogicalRepWorkerLock which is not likely to be contended.
Thanks for the comment and I think your suggestion makes sense.
I have added the check before getting the leader pid. Here is the new version
patch.
Best regards,
Hou zj
v83-0001-Display-the-leader-apply-worker-s-PID-for-parall.patch
Description: v83-0001-Display-the-leader-apply-worker-s-PID-for-parall.patch
Re: Perform streaming logical transactions by background workers and parallel apply
On Tue, Jan 17, 2023 at 12:37 PM houzj.f...@fujitsu.com
wrote:
>
> On Tuesday, January 17, 2023 11:32 AM Peter Smith
> wrote:
> >
> > On Tue, Jan 17, 2023 at 1:21 PM houzj.f...@fujitsu.com
> > wrote:
> > >
> > > On Tuesday, January 17, 2023 5:43 AM Peter Smith
> > wrote:
> > > >
> > > > On Mon, Jan 16, 2023 at 5:41 PM Amit Kapila
> > > >
> > > > wrote:
> > > > >
> > > > > On Mon, Jan 16, 2023 at 10:24 AM Peter Smith
> > > > >
> > > > wrote:
> > > > > >
> > > > > > 2.
> > > > > >
> > > > > > /*
> > > > > > + * Return the pid of the leader apply worker if the given pid
> > > > > > +is the pid of a
> > > > > > + * parallel apply worker, otherwise return InvalidPid.
> > > > > > + */
> > > > > > +pid_t
> > > > > > +GetLeaderApplyWorkerPid(pid_t pid) { int leader_pid =
> > > > > > +InvalidPid; int i;
> > > > > > +
> > > > > > + LWLockAcquire(LogicalRepWorkerLock, LW_SHARED);
> > > > > > +
> > > > > > + for (i = 0; i < max_logical_replication_workers; i++) {
> > > > > > + LogicalRepWorker *w = >workers[i];
> > > > > > +
> > > > > > + if (isParallelApplyWorker(w) && w->proc && pid ==
> > > > > > + w->proc->pid) { leader_pid = w->leader_pid; break; } }
> > > > > > +
> > > > > > + LWLockRelease(LogicalRepWorkerLock);
> > > > > > +
> > > > > > + return leader_pid;
> > > > > > +}
> > > > > >
> > > > > > 2a.
> > > > > > IIUC the IsParallelApplyWorker macro does nothing except check
> > > > > > that the leader_pid is not InvalidPid anyway, so AFAIK this
> > > > > > algorithm does not benefit from using this macro because we will
> > > > > > want to return InvalidPid anyway if the given pid matches.
> > > > > >
> > > > > > So the inner condition can just say:
> > > > > >
> > > > > > if (w->proc && w->proc->pid == pid) { leader_pid =
> > > > > > w->leader_pid; break; }
> > > > > >
> > > > >
> > > > > Yeah, this should also work but I feel the current one is explicit
> > > > > and more clear.
> > > >
> > > > OK.
> > > >
> > > > But, I have one last comment about this function -- I saw there are
> > > > already other functions that iterate max_logical_replication_workers
> > > > like this looking for things:
> > > > - logicalrep_worker_find
> > > > - logicalrep_workers_find
> > > > - logicalrep_worker_launch
> > > > - logicalrep_sync_worker_count
> > > >
> > > > So I felt this new function (currently called
> > > > GetLeaderApplyWorkerPid) ought to be named similarly to those ones.
> > > > e.g. call it something like "logicalrep_worker_find_pa_leader_pid".
> > > >
> > >
> > > I am not sure we can use the name, because currently all the API name
> > > in launcher that used by other module(not related to subscription) are
> > > like AxxBxx style(see the functions in logicallauncher.h).
> > > logicalrep_worker_xxx style functions are currently only declared in
> > > worker_internal.h.
> > >
> >
> > OK. I didn't know there was another header convention that you were
> > following.
> > In that case, it is fine to leave the name as-is.
>
> Thanks for confirming!
>
> Attach the new version 0001 patch which addressed all other comments.
>
Thank you for updating the patch. Here is one comment:
@@ -426,14 +427,24 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
/*
* Show the leader only for active
parallel workers. This
-* leaves the field as NULL for the
leader of a parallel
-* group.
+* leaves the field as NULL for the
leader of a parallel group
+* or the leader of parallel apply workers.
*/
if (leader && leader->pid !=
beentry->st_procpid)
{
values[28] = Int32GetDatum(leader->pid);
nulls[28] = false;
}
+ else
+ {
+ int
leader_pid = GetLeaderApplyWorkerPid(beentry->st_procpid);
+
+ if (leader_pid != InvalidPid)
+ {
+ values[28] =
Int32GetDatum(leader_pid);
+ nulls[28] = false;
+ }
+ }
}
I'm slightly concerned that there could be overhead of executing
GetLeaderApplyWorkerPid () for every backend process except for
parallel query workers. The number of such backends could be large and
GetLeaderApplyWorkerPid() acquires the lwlock. For example, does it
make sense to check (st_backendType == B_BG_WORKER) before calling
GetLeaderApplyWorkerPid()? Or it might not be a problem since it's
LogicalRepWorkerLock which is not likely to be contended.
Regards,
--
Masahiko Sawada
Amazon
RE: Perform streaming logical transactions by background workers and parallel apply
On Tuesday, January 17, 2023 12:55 PM Amit Kapila wrote: > > On Tue, Jan 17, 2023 at 8:59 AM Amit Kapila wrote: > > > > On Tue, Jan 17, 2023 at 8:35 AM Masahiko Sawada > wrote: > > > > > > On Mon, Jan 16, 2023 at 3:19 PM Amit Kapila > wrote: > > > > > > > > Okay, I have added the comments in get_transaction_apply_action() > > > > and updated the comments to refer to the enum TransApplyAction > > > > where all the actions are explained. > > > > > > Thank you for the patch. > > > > > > @@ -1710,6 +1712,7 @@ apply_handle_stream_stop(StringInfo s) > > > } > > > > > > in_streamed_transaction = false; > > > + stream_xid = InvalidTransactionId; > > > > > > We reset stream_xid also in stream_close_file() but probably it's no > > > longer necessary? > > > > > > > I think so. > > > > > How about adding an assertion in apply_handle_stream_start() to make > > > sure the stream_xid is invalid? > > > > > > > I think it would be better to add such an assert in > > apply_handle_begin/apply_handle_begin_prepare because there won't be a > > problem if we start_stream message even when stream_xid is valid. > > However, maybe it is better to add in all three functions > > > (apply_handle_begin/apply_handle_begin_prepare/apply_handle_stream_star > t). > > What do you think? > > > > > --- > > > It's not related to this issue but I realized that if the action > > > returned by get_transaction_apply_action() is not handled in the > > > switch statement, we do only Assert(false). Is it better to raise an > > > error like "unexpected apply action %d" just in case in order to > > > detect failure cases also in the production environment? > > > > > > > Yeah, that may be better. Shall we do that as part of this patch only > > or as a separate patch? > > > > Please find attached the updated patches to address the above comments. I > think we can combine and commit them as one patch as both are related. Thanks for fixing these. I have confirmed that all regression tests passed after applying the patches. And the patches look good to me. Best regards, Hou zj
Re: Perform streaming logical transactions by background workers and parallel apply
On Tue, Jan 17, 2023 at 1:55 PM Amit Kapila wrote: > > On Tue, Jan 17, 2023 at 8:59 AM Amit Kapila wrote: > > > > On Tue, Jan 17, 2023 at 8:35 AM Masahiko Sawada > > wrote: > > > > > > On Mon, Jan 16, 2023 at 3:19 PM Amit Kapila > > > wrote: > > > > > > > > Okay, I have added the comments in get_transaction_apply_action() and > > > > updated the comments to refer to the enum TransApplyAction where all > > > > the actions are explained. > > > > > > Thank you for the patch. > > > > > > @@ -1710,6 +1712,7 @@ apply_handle_stream_stop(StringInfo s) > > > } > > > > > > in_streamed_transaction = false; > > > + stream_xid = InvalidTransactionId; > > > > > > We reset stream_xid also in stream_close_file() but probably it's no > > > longer necessary? > > > > > > > I think so. > > > > > How about adding an assertion in apply_handle_stream_start() to make > > > sure the stream_xid is invalid? > > > > > > > I think it would be better to add such an assert in > > apply_handle_begin/apply_handle_begin_prepare because there won't be a > > problem if we start_stream message even when stream_xid is valid. > > However, maybe it is better to add in all three functions > > (apply_handle_begin/apply_handle_begin_prepare/apply_handle_stream_start). > > What do you think? > > > > > --- > > > It's not related to this issue but I realized that if the action > > > returned by get_transaction_apply_action() is not handled in the > > > switch statement, we do only Assert(false). Is it better to raise an > > > error like "unexpected apply action %d" just in case in order to > > > detect failure cases also in the production environment? > > > > > > > Yeah, that may be better. Shall we do that as part of this patch only > > or as a separate patch? > > > > Please find attached the updated patches to address the above > comments. I think we can combine and commit them as one patch as both > are related. Thank you for the patches! Looks good to me. And +1 to merge them. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Re: Perform streaming logical transactions by background workers and parallel apply
On Tue, Jan 17, 2023 at 8:59 AM Amit Kapila wrote: > > On Tue, Jan 17, 2023 at 8:35 AM Masahiko Sawada wrote: > > > > On Mon, Jan 16, 2023 at 3:19 PM Amit Kapila wrote: > > > > > > Okay, I have added the comments in get_transaction_apply_action() and > > > updated the comments to refer to the enum TransApplyAction where all > > > the actions are explained. > > > > Thank you for the patch. > > > > @@ -1710,6 +1712,7 @@ apply_handle_stream_stop(StringInfo s) > > } > > > > in_streamed_transaction = false; > > + stream_xid = InvalidTransactionId; > > > > We reset stream_xid also in stream_close_file() but probably it's no > > longer necessary? > > > > I think so. > > > How about adding an assertion in apply_handle_stream_start() to make > > sure the stream_xid is invalid? > > > > I think it would be better to add such an assert in > apply_handle_begin/apply_handle_begin_prepare because there won't be a > problem if we start_stream message even when stream_xid is valid. > However, maybe it is better to add in all three functions > (apply_handle_begin/apply_handle_begin_prepare/apply_handle_stream_start). > What do you think? > > > --- > > It's not related to this issue but I realized that if the action > > returned by get_transaction_apply_action() is not handled in the > > switch statement, we do only Assert(false). Is it better to raise an > > error like "unexpected apply action %d" just in case in order to > > detect failure cases also in the production environment? > > > > Yeah, that may be better. Shall we do that as part of this patch only > or as a separate patch? > Please find attached the updated patches to address the above comments. I think we can combine and commit them as one patch as both are related. -- With Regards, Amit Kapila. v2-0001-Improve-the-code-to-decide-the-apply-action.patch Description: Binary data v2-0002-Change-assert-to-elog-for-unexpected-apply-action.patch Description: Binary data
Re: Perform streaming logical transactions by background workers and parallel apply
On Tue, Jan 17, 2023 at 9:07 AM houzj.f...@fujitsu.com
wrote:
>
> On Tuesday, January 17, 2023 11:32 AM Peter Smith
> wrote:
> >
> > On Tue, Jan 17, 2023 at 1:21 PM houzj.f...@fujitsu.com
> > wrote:
> > >
> > > On Tuesday, January 17, 2023 5:43 AM Peter Smith
> > wrote:
> > > >
> > > > On Mon, Jan 16, 2023 at 5:41 PM Amit Kapila
> > > >
> > > > wrote:
> > > > >
> > > > > On Mon, Jan 16, 2023 at 10:24 AM Peter Smith
> > > > >
> > > > wrote:
> > > > > >
> > > > > > 2.
> > > > > >
> > > > > > /*
> > > > > > + * Return the pid of the leader apply worker if the given pid
> > > > > > +is the pid of a
> > > > > > + * parallel apply worker, otherwise return InvalidPid.
> > > > > > + */
> > > > > > +pid_t
> > > > > > +GetLeaderApplyWorkerPid(pid_t pid) { int leader_pid =
> > > > > > +InvalidPid; int i;
> > > > > > +
> > > > > > + LWLockAcquire(LogicalRepWorkerLock, LW_SHARED);
> > > > > > +
> > > > > > + for (i = 0; i < max_logical_replication_workers; i++) {
> > > > > > + LogicalRepWorker *w = >workers[i];
> > > > > > +
> > > > > > + if (isParallelApplyWorker(w) && w->proc && pid ==
> > > > > > + w->proc->pid) { leader_pid = w->leader_pid; break; } }
> > > > > > +
> > > > > > + LWLockRelease(LogicalRepWorkerLock);
> > > > > > +
> > > > > > + return leader_pid;
> > > > > > +}
> > > > > >
> > > > > > 2a.
> > > > > > IIUC the IsParallelApplyWorker macro does nothing except check
> > > > > > that the leader_pid is not InvalidPid anyway, so AFAIK this
> > > > > > algorithm does not benefit from using this macro because we will
> > > > > > want to return InvalidPid anyway if the given pid matches.
> > > > > >
> > > > > > So the inner condition can just say:
> > > > > >
> > > > > > if (w->proc && w->proc->pid == pid) { leader_pid =
> > > > > > w->leader_pid; break; }
> > > > > >
> > > > >
> > > > > Yeah, this should also work but I feel the current one is explicit
> > > > > and more clear.
> > > >
> > > > OK.
> > > >
> > > > But, I have one last comment about this function -- I saw there are
> > > > already other functions that iterate max_logical_replication_workers
> > > > like this looking for things:
> > > > - logicalrep_worker_find
> > > > - logicalrep_workers_find
> > > > - logicalrep_worker_launch
> > > > - logicalrep_sync_worker_count
> > > >
> > > > So I felt this new function (currently called
> > > > GetLeaderApplyWorkerPid) ought to be named similarly to those ones.
> > > > e.g. call it something like "logicalrep_worker_find_pa_leader_pid".
> > > >
> > >
> > > I am not sure we can use the name, because currently all the API name
> > > in launcher that used by other module(not related to subscription) are
> > > like AxxBxx style(see the functions in logicallauncher.h).
> > > logicalrep_worker_xxx style functions are currently only declared in
> > > worker_internal.h.
> > >
> >
> > OK. I didn't know there was another header convention that you were
> > following.
> > In that case, it is fine to leave the name as-is.
>
> Thanks for confirming!
>
> Attach the new version 0001 patch which addressed all other comments.
>
> Best regards,
> Hou zj
Hello Hou-san,
1. Do we need to extend test-cases to review the leader_pid column in
pg_stats tables?
2. Do we need to follow the naming convention for
'GetLeaderApplyWorkerPid' like other functions in the same file which
starts with 'logicalrep_'
thanks
Shveta
Re: Perform streaming logical transactions by background workers and parallel apply
On Tue, Jan 17, 2023 at 2:37 PM houzj.f...@fujitsu.com
wrote:
>
> On Tuesday, January 17, 2023 11:32 AM Peter Smith
> wrote:
> >
> > On Tue, Jan 17, 2023 at 1:21 PM houzj.f...@fujitsu.com
> > wrote:
> > >
> > > On Tuesday, January 17, 2023 5:43 AM Peter Smith
> > wrote:
> > > >
> > > > On Mon, Jan 16, 2023 at 5:41 PM Amit Kapila
> > > >
> > > > wrote:
> > > > >
> > > > > On Mon, Jan 16, 2023 at 10:24 AM Peter Smith
> > > > >
> > > > wrote:
> > > > > >
> > > > > > 2.
> > > > > >
> > > > > > /*
> > > > > > + * Return the pid of the leader apply worker if the given pid
> > > > > > +is the pid of a
> > > > > > + * parallel apply worker, otherwise return InvalidPid.
> > > > > > + */
> > > > > > +pid_t
> > > > > > +GetLeaderApplyWorkerPid(pid_t pid) { int leader_pid =
> > > > > > +InvalidPid; int i;
> > > > > > +
> > > > > > + LWLockAcquire(LogicalRepWorkerLock, LW_SHARED);
> > > > > > +
> > > > > > + for (i = 0; i < max_logical_replication_workers; i++) {
> > > > > > + LogicalRepWorker *w = >workers[i];
> > > > > > +
> > > > > > + if (isParallelApplyWorker(w) && w->proc && pid ==
> > > > > > + w->proc->pid) { leader_pid = w->leader_pid; break; } }
> > > > > > +
> > > > > > + LWLockRelease(LogicalRepWorkerLock);
> > > > > > +
> > > > > > + return leader_pid;
> > > > > > +}
> > > > > >
> > > > > > 2a.
> > > > > > IIUC the IsParallelApplyWorker macro does nothing except check
> > > > > > that the leader_pid is not InvalidPid anyway, so AFAIK this
> > > > > > algorithm does not benefit from using this macro because we will
> > > > > > want to return InvalidPid anyway if the given pid matches.
> > > > > >
> > > > > > So the inner condition can just say:
> > > > > >
> > > > > > if (w->proc && w->proc->pid == pid) { leader_pid =
> > > > > > w->leader_pid; break; }
> > > > > >
> > > > >
> > > > > Yeah, this should also work but I feel the current one is explicit
> > > > > and more clear.
> > > >
> > > > OK.
> > > >
> > > > But, I have one last comment about this function -- I saw there are
> > > > already other functions that iterate max_logical_replication_workers
> > > > like this looking for things:
> > > > - logicalrep_worker_find
> > > > - logicalrep_workers_find
> > > > - logicalrep_worker_launch
> > > > - logicalrep_sync_worker_count
> > > >
> > > > So I felt this new function (currently called
> > > > GetLeaderApplyWorkerPid) ought to be named similarly to those ones.
> > > > e.g. call it something like "logicalrep_worker_find_pa_leader_pid".
> > > >
> > >
> > > I am not sure we can use the name, because currently all the API name
> > > in launcher that used by other module(not related to subscription) are
> > > like AxxBxx style(see the functions in logicallauncher.h).
> > > logicalrep_worker_xxx style functions are currently only declared in
> > > worker_internal.h.
> > >
> >
> > OK. I didn't know there was another header convention that you were
> > following.
> > In that case, it is fine to leave the name as-is.
>
> Thanks for confirming!
>
> Attach the new version 0001 patch which addressed all other comments.
>
OK. I checked the differences between patches v81-0001/v82-0001 and
found everything I was expecting to see.
I have no more review comments for v82-0001.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
RE: Perform streaming logical transactions by background workers and parallel apply
On Tuesday, January 17, 2023 11:32 AM Peter Smith wrote:
>
> On Tue, Jan 17, 2023 at 1:21 PM houzj.f...@fujitsu.com
> wrote:
> >
> > On Tuesday, January 17, 2023 5:43 AM Peter Smith
> wrote:
> > >
> > > On Mon, Jan 16, 2023 at 5:41 PM Amit Kapila
> > >
> > > wrote:
> > > >
> > > > On Mon, Jan 16, 2023 at 10:24 AM Peter Smith
> > > >
> > > wrote:
> > > > >
> > > > > 2.
> > > > >
> > > > > /*
> > > > > + * Return the pid of the leader apply worker if the given pid
> > > > > +is the pid of a
> > > > > + * parallel apply worker, otherwise return InvalidPid.
> > > > > + */
> > > > > +pid_t
> > > > > +GetLeaderApplyWorkerPid(pid_t pid) { int leader_pid =
> > > > > +InvalidPid; int i;
> > > > > +
> > > > > + LWLockAcquire(LogicalRepWorkerLock, LW_SHARED);
> > > > > +
> > > > > + for (i = 0; i < max_logical_replication_workers; i++) {
> > > > > + LogicalRepWorker *w = >workers[i];
> > > > > +
> > > > > + if (isParallelApplyWorker(w) && w->proc && pid ==
> > > > > + w->proc->pid) { leader_pid = w->leader_pid; break; } }
> > > > > +
> > > > > + LWLockRelease(LogicalRepWorkerLock);
> > > > > +
> > > > > + return leader_pid;
> > > > > +}
> > > > >
> > > > > 2a.
> > > > > IIUC the IsParallelApplyWorker macro does nothing except check
> > > > > that the leader_pid is not InvalidPid anyway, so AFAIK this
> > > > > algorithm does not benefit from using this macro because we will
> > > > > want to return InvalidPid anyway if the given pid matches.
> > > > >
> > > > > So the inner condition can just say:
> > > > >
> > > > > if (w->proc && w->proc->pid == pid) { leader_pid =
> > > > > w->leader_pid; break; }
> > > > >
> > > >
> > > > Yeah, this should also work but I feel the current one is explicit
> > > > and more clear.
> > >
> > > OK.
> > >
> > > But, I have one last comment about this function -- I saw there are
> > > already other functions that iterate max_logical_replication_workers
> > > like this looking for things:
> > > - logicalrep_worker_find
> > > - logicalrep_workers_find
> > > - logicalrep_worker_launch
> > > - logicalrep_sync_worker_count
> > >
> > > So I felt this new function (currently called
> > > GetLeaderApplyWorkerPid) ought to be named similarly to those ones.
> > > e.g. call it something like "logicalrep_worker_find_pa_leader_pid".
> > >
> >
> > I am not sure we can use the name, because currently all the API name
> > in launcher that used by other module(not related to subscription) are
> > like AxxBxx style(see the functions in logicallauncher.h).
> > logicalrep_worker_xxx style functions are currently only declared in
> > worker_internal.h.
> >
>
> OK. I didn't know there was another header convention that you were following.
> In that case, it is fine to leave the name as-is.
Thanks for confirming!
Attach the new version 0001 patch which addressed all other comments.
Best regards,
Hou zj
v82-0001-Display-the-leader-apply-worker-s-PID-for-parall.patch
Description: v82-0001-Display-the-leader-apply-worker-s-PID-for-parall.patch
