MIME-Version: 1.0
References: <CAA4eK1LGmZsevrqJra0V4O8oBU_eKyzm2VMpSAYQaDgC6n4fkA@mail.gmail.com>
 <CAFiTN-uhnJkrUsQByBHMK268T-GEx_D8DQ6b2T+aW6RiU75pbQ@mail.gmail.com>
 <OS0PR01MB571621ED532C2D7C3E01625894FB9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAFiTN-t5+_zTf86EzgCwObs1ED-P_5Ab1KMw340d3oPY_+Cwpw@mail.gmail.com>
 <OS3PR01MB5718AE486227CE3ACB844C5E94F89@OS3PR01MB5718.jpnprd01.prod.outlook.com>
 <CAFiTN-sgQ-K9BnMWKLR7Hm36QNjS7ZOBF6hsXJmQz5ChC0Desw@mail.gmail.com>
 <CAA4eK1+P9WOFBupaTZCzYtranSH=8TSUYV=kk5xQJ-VNKnvgmQ@mail.gmail.com>
 <OS0PR01MB571685A645E509267AC5F30894F99@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <OS0PR01MB57168A5A1C31BC316CB63D3C94F99@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1KK7Xty7SaCReY_b5moPURowMGOsfvhOb6xQ1EAaJAh6A@mail.gmail.com>
 <OS0PR01MB5716863B23DBAD186F64ADF194FF9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAHut+PvA10Bp9Jaw9OS2+puKHr7ry_xB3Tf2-bbv5gyxD5E_gw@mail.gmail.com>
 <CAA4eK1KYUbnthSPyo4VjnhMygB0c1DZtp0XC-V2-GSETQ743ww@mail.gmail.com>
 <CAJpy0uC0JamKT6pTtux3NDhcqSMJnDoQXZxx32pq=2yyxQ1c5Q@mail.gmail.com>
 <CAA4eK1KueUT_vr1cg43MpvRcaVvW03i6zDECQNAwFBK5PhXOVA@mail.gmail.com>
 <OS0PR01MB5716C663C85687E76672327094FD9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAHut+PuKYdA4047wATm8WpkkqQX2CPNCa++dSQKr6dD1O5nz_Q@mail.gmail.com>
 <CAA4eK1LotEuPsteuJMNpixxTj6R4B8k93q-6ruRmDzCxKzMNpA@mail.gmail.com>
 <CAA4eK1JdDrFxcw20YHifFrfX1o_e=9sXpNMfNC5vMYvYcebwNg@mail.gmail.com> <CAJpy0uDzddK_ZUsB2qBJUbW_ZODYGoUHTaS5pVcYE2tzATCVXQ@mail.gmail.com>
In-Reply-To: <CAJpy0uDzddK_ZUsB2qBJUbW_ZODYGoUHTaS5pVcYE2tzATCVXQ@mail.gmail.com>
From: shveta malik <shveta.malik@gmail.com>
Date: Thu, 19 Jan 2023 15:52:36 +0530
Message-ID: <CAJpy0uBVjmzNn10VFsP6RpAKHHaGa0EL-AEdaF6o7ocaONthKA@mail.gmail.com>
Subject: Re: Perform streaming logical transactions by background workers and
 parallel apply
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Peter Smith <smithpb2250@gmail.com>, 
	"houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>, Masahiko Sawada <sawada.mshk@gmail.com>, 
	"wangw.fnst@fujitsu.com" <wangw.fnst@fujitsu.com>, "shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Dilip Kumar <dilipbalaut@gmail.com>, 
	shveta malik <shveta.malik@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CAJpy0uBVjmzNn10VFsP6RpAKHHaGa0EL-AEdaF6o7ocaONthKA%40mail.gmail.com>
Precedence: bulk

On Thu, Jan 19, 2023 at 3:44 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Thu, Jan 19, 2023 at 11:11 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Jan 18, 2023 at 12:09 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Fri, Jan 13, 2023 at 11:50 AM Peter Smith <smithpb2250@gmail.com> 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:
> 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

thanks
Shveta