From: | "shiy(dot)fnst(at)fujitsu(dot)com" <shiy(dot)fnst(at)fujitsu(dot)com> |
---|---|
To: | Dilip Kumar <dilipbalaut(at)gmail(dot)com> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | RE: Force streaming every change in logical decoding |
Date: | 2022-12-14 08:44:56 |
Message-ID: | OSZPR01MB63102546180646AEE1C66C34FDE09@OSZPR01MB6310.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Sat, Dec 10, 2022 2:03 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Tue, Dec 6, 2022 at 11:53 AM shiy(dot)fnst(at)fujitsu(dot)com
> <shiy(dot)fnst(at)fujitsu(dot)com> wrote:
> >
> > Hi hackers,
> >
> > In logical decoding, when logical_decoding_work_mem is exceeded, the
> changes are
> > sent to output plugin in streaming mode. But there is a restriction that the
> > minimum value of logical_decoding_work_mem is 64kB. I tried to add a GUC
> to
> > allow sending every change to output plugin without waiting until
> > logical_decoding_work_mem is exceeded.
> >
> > This helps to test streaming mode. For example, to test "Avoid streaming the
> > transaction which are skipped" [1], it needs many
> XLOG_XACT_INVALIDATIONS
> > messages. With the new option, it can be tested with fewer changes and in
> less
> > time. Also, this new option helps to test more scenarios for "Perform
> streaming
> > logical transactions by background workers" [2].
>
> Some comments on the patch
>
Thanks for your comments.
> 1. Can you add one test case using this option
>
I added a simple test to confirm the new option works.
> 2. + <varlistentry id="guc-force-stream-mode"
> xreflabel="force_stream_mode">
> + <term><varname>force_stream_mode</varname>
> (<type>boolean</type>)
> + <indexterm>
> + <primary><varname>force_stream_mode</varname> configuration
> parameter</primary>
> + </indexterm>
> + </term>
>
> This GUC name "force_stream_mode" somehow appears like we are forcing
> this streaming mode irrespective of whether the
> subscriber has requested for this mode or not. But actually it is not
> that, it is just streaming each change if
> it is enabled. So we might need to think on the name (at least we
> should avoid using *mode* in the name IMHO).
>
I think a similar GUC is force_parallel_mode, and if the query is parallel
unsafe or max_worker_processes is exceeded, force_parallel_mode will not work.
This is similar to what we do in this patch. So, maybe it's ok to use "mode". I
didn't change it in the new version of patch. What do you think?
> 3.
> - while (rb->size >= logical_decoding_work_mem * 1024L)
> + while ((!force_stream && rb->size >= logical_decoding_work_mem *
> 1024L) ||
> + (force_stream && rb->size > 0))
> {
>
> It seems like if force_stream is on then indirectly it is enabling
> force serialization as well. Because once we enter into the loop
> based on "force_stream" then it will either stream or serialize but I
> guess we do not want to force serialize based on this parameter.
>
Agreed, I refactored the code and modified this point.
Please see the attached patch. I also fix Peter's comments[1]. The GUC name and
design are still under discussion, so I didn't modify them.
By the way, I noticed that the comment for ReorderBufferCheckMemoryLimit() on
HEAD missed something. I fix it in this patch, too.
Regards,
Shi yu
Attachment | Content-Type | Size |
---|---|---|
v2-0001-Allow-streaming-every-change-without-waiting-till.patch | application/octet-stream | 9.4 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Pavel Luzanov | 2022-12-14 09:07:13 | Re: allow granting CLUSTER, REFRESH MATERIALIZED VIEW, and REINDEX |
Previous Message | Amit Langote | 2022-12-14 08:35:47 | Re: generic plans and "initial" pruning |