RE: Perform streaming logical transactions by background workers and parallel apply

On Friday, January 6, 2023 3:29 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

Hi,

Thanks for your comments.

> On Fri, Jan 6, 2023 at 12:05 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> 
> >
> > Yeah, I also don't think sending extra eight bytes with stream_start
> > message is worth it. But it is fine to mention the same in the
> > comments.
> 
> Right.

Added some comment.

> 
> > > 2.
> > >
> > > +     * XXX Additionally, we also stop the worker if the leader apply
> worker
> > > +     * serialize part of the transaction data due to a send timeout. This is
> > > +     * because the message could be partially written to the queue and
> there
> > > +     * is no way to clean the queue other than resending the message
> until it
> > > +     * succeeds. Instead of trying to send the data which anyway would
> have
> > > +     * been serialized and then letting the parallel apply worker deal with
> > > +     * the spurious message, we stop the worker.
> > > +     */
> > > +    if (winfo->serialize_changes ||
> > > +        list_length(ParallelApplyWorkerPool) >
> > > +        (max_parallel_apply_workers_per_subscription / 2))
> > >
> > > IMHO this reason (XXX Additionally, we also stop the worker if the
> > > leader apply worker serialize part of the transaction data due to a
> > > send timeout) for stopping the worker looks a bit hackish to me.  It
> > > may be a rare case so I am not talking about the performance but the
> > > reasoning behind stopping is not good. Ideally we should be able to
> > > clean up the message queue and reuse the worker.
> > >
> >
> > TBH, I don't know what is the better way to deal with this with the
> > current infrastructure. I thought we can do this as a separate
> > enhancement in the future.
> 
> Okay.
> 
> > > 3.
> > > +        else if (shmq_res == SHM_MQ_WOULD_BLOCK)
> > > +        {
> > > +            /* Replay the changes from the file, if any. */
> > > +            if (pa_has_spooled_message_pending())
> > > +            {
> > > +                pa_spooled_messages();
> > > +            }
> > >
> > > I think we do not need this pa_has_spooled_message_pending() function.
> > > Because this function is just calling pa_get_fileset_state() which
> > > is acquiring mutex and getting filestate then if the filestate is
> > > not FS_EMPTY then we call pa_spooled_messages() that will again call
> > > pa_get_fileset_state() which will again acquire mutex.  I think when
> > > the state is FS_SERIALIZE_IN_PROGRESS it will frequently call
> > > pa_get_fileset_state() consecutively 2 times, and I think we can
> > > easily achieve the same behavior with just one call.
> > >
> >
> > This is just to keep the code easy to follow. As this would be a rare
> > case, so thought of giving preference to code clarity.
> 
> I think the code will be simpler with just one function no? I mean instead of
> calling pa_has_spooled_message_pending() in if condition what if we directly
> call pa_spooled_messages();, this is anyway fetching the file_state and if the
> filestate is EMPTY then it can return false, and if it returns false we can execute
> the code which is there in else condition.  We might need to change the name
> of the function though.

Changed as suggested.

I have addressed all the comments and here is the new version patch set.
I also added some documents about the new lock and fixed some typos.

Attach the new version patch set.

Best regards,
Hou zj