On Sat, 20 Jul 2024 at 20:48, vignesh C <vignesh21@gmail.com> wrote:
>
> On Fri, 12 Jul 2024 at 08:22, Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Hi Vignesh. Here are the rest of my comments for patch v20240705-0003.
> > ======
> >
> > 8. logicalrep_sequence_sync_worker_find
> >
> > +/*
> > + * Walks the workers array and searches for one that matches given
> > + * subscription id.
> > + *
> > + * We are only interested in the sequence sync worker.
> > + */
> > +LogicalRepWorker *
> > +logicalrep_sequence_sync_worker_find(Oid subid, bool only_running)
> >
> > There are other similar functions for walking the workers array to
> > search for a worker. Instead of having different functions for
> > different cases, wouldn't it be cleaner to combine these into a single
> > function, where you pass a parameter (e.g. a mask of worker types that
> > you are interested in finding)?
This is fixed in the v20240730_2 version attached at [1].
> > 17.
> > Also, where does the number 100 come from? Why not 1000? Why not 10?
> > Why have batching at all? Maybe there should be some comment to
> > describe the reason and the chosen value.
I had run some tests with 10/100 and 1000 sequences per batch for
10000 sequences. The results for it:
10 per batch - 4.94 seconds
100 per batch - 4.87 seconds
1000 per batch - 4.53 seconds
There is not much time difference between each of them. Currently, it
is set to 100, which seems fine since it will not generate a lot of
transactions. Additionally, the locks on the sequences will be
periodically released during the commit transaction.
I had used the test from the attached patch by changing
max_sequences_sync_per_batch to 10/100/100 in 035_sequences.pl to
verify this.
[1] - https://www.postgresql.org/message-id/CALDaNm3%2BXzHAbgyn8gmbBLK5goyv_uyGgHEsTQxRZ8bVk6nAEg%40mail.gmail.com
Regards,
Vignesh
