Re: [PATCH] fix a performance issue with multiple logical-decoding walsenders

On Sunday, December 29, 2019 1:32:31 PM CET Julien Rouhaud wrote:
> On Sat, Dec 28, 2019 at 1:56 PM Pierre Ducroquet <p(dot)psql(at)pinaraf(dot)info>
wrote:
> > Thank you for your comments.
> > Attached to this email is a patch with better comments regarding the
> > XLogSendLogical change.
>
> Arguably the first test to compare to InvalidXLogRecPtr is unneeded,
> as any value of EndRecPtr is greater or equal than that value. It
> will only save at best 1 GetFlushRecPtr() per walsender process
> lifetime, so I'm not sure it's worth arguing about it. Other than
> that I still think that it's a straightforward optimization that
> brings nice speedup, and I don't see any problem with this patch. I
> think that given the time of the year you should create a commitfest
> entry for this patch to make sure it won't be forgotten (and obviously
> I'll mark it as RFC, unless someone objects by then).
>
> > We've spent quite some time yesterday benching it again, this time with
> > changes that must be fully processed by the decoder. The speed-up is
> > obviously much smaller, we are only ~5% faster than without the patch.
>
> I'm assuming that it's benchmarking done with multiple logical slots?
> Anyway, a 5% speedup in the case that this patch is not aimed to
> optimize is quite nice!

I've created a commitfest entry for this patch.
https://commitfest.postgresql.org/26/2403/
I would like to know if it would be acceptable to backport this to PostgreSQL
12. I have to write a clean benchmark for that (our previous benchs are either
PG10 or PG12 specific), but the change from Thomas Munro that removed the
calls to PostmasterIsAlive is very likely to have the same side-effect we
observed in PG10 when patching IsAlive, aka. moving the pressure from the pipe
reads to the PostgreSQL locks between processes, and this made the whole
process slower: the benchmark showed a serious regression, going from 3m45s to
5m15s to decode the test transactions.

Regards

Pierre