On Sat, Dec 10, 2022 2:03 PM Dilip Kumar <[email protected]> wrote: > > On Tue, Dec 6, 2022 at 11:53 AM [email protected] > <[email protected]> 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.
[1]
https://www.postgresql.org/message-id/CAHut%2BPtOjZ_e-KLf26i1XLH2ffPEZGOmGSKy0wDjwyB_uvzxBQ%40mail.gmail.com
Regards,
Shi yu
v2-0001-Allow-streaming-every-change-without-waiting-till.patch
Description: v2-0001-Allow-streaming-every-change-without-waiting-till.patch
