On Mon, Dec 5, 2022 at 9:21 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Mon, Dec 5, 2022 at 8:59 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Sun, Dec 4, 2022 at 5:14 PM houzj.fnst@fujitsu.com
> > <houzj.fnst@fujitsu.com> wrote:
> > >
> > > On Saturday, December 3, 2022 7:37 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > Apart from the above, I have slightly adjusted the comments in the attached. Do
> > > > let me know what you think of the attached.
> > >
> > > Thanks for updating the patch. It looks good to me.
> > >
> >
> > I feel the function name ReorderBufferLargestTopTXN() is slightly
> > misleading because it also checks some of the streaming properties
> > (like whether the TXN has partial changes and whether it contains any
> > streamable change). Shall we rename it to
> > ReorderBufferLargestStreamableTopTXN() or something like that?
>
> Yes that makes sense
I have done this change in the attached patch.
> > The other point to consider is whether we need to have a test case for
> > this patch. I think before this patch if the size of DDL changes in a
> > transaction exceeds logical_decoding_work_mem, the empty streams will
> > be output in the plugin but after this patch, there won't be any such
> > stream.
I tried this test, but I think generating 64k data with just CID
messages will make the test case really big. I tried using multiple
sessions such that one session makes the reorder buffer full but
contains partial changes so that we try to stream another transaction
but that is not possible in an automated test to consistently generate
the partial change.
I think we need something like this[1] so that we can better control
the streaming.
[1]https://www.postgresql.org/message-id/OSZPR01MB631042582805A8E8615BC413FD329%40OSZPR01MB6310.jpnprd01.prod.outlook.com
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
