From: | "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com> |
---|---|
To: | Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, "wangw(dot)fnst(at)fujitsu(dot)com" <wangw(dot)fnst(at)fujitsu(dot)com>, Peter Smith <smithpb2250(at)gmail(dot)com>, "shiy(dot)fnst(at)fujitsu(dot)com" <shiy(dot)fnst(at)fujitsu(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | RE: Perform streaming logical transactions by background workers and parallel apply |
Date: | 2023-01-06 10:07:57 |
Message-ID: | OS0PR01MB571621ED532C2D7C3E01625894FB9@OS0PR01MB5716.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Friday, January 6, 2023 3:29 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
Hi,
Thanks for your comments.
> On Fri, Jan 6, 2023 at 12:05 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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
Attachment | Content-Type | Size |
---|---|---|
v75-0005-Add-a-main_worker_pid-to-pg_stat_subscription.patch | application/octet-stream | 9.4 KB |
v75-0001-Perform-apply-of-large-transactions-by-parallel-.patch | application/octet-stream | 265.6 KB |
v75-0002-Add-GUC-stream_serialize_threshold-and-test-seri.patch | application/octet-stream | 12.5 KB |
v75-0003-Stop-extra-worker-if-GUC-was-changed.patch | application/octet-stream | 4.2 KB |
v75-0004-Retry-to-apply-streaming-xact-only-in-apply-work.patch | application/octet-stream | 21.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Jelte Fennema | 2023-01-06 10:17:48 | Re: [PATCH] Add `verify-system` sslmode to use system CA pool for server cert |
Previous Message | Guillaume Lelarge | 2023-01-06 10:07:50 | Issue attaching a table to a partitioned table with an auto-referenced foreign key |