Re: Optimize SnapBuildPurgeOlderTxn: use in-place compaction instead of temporary array

Hi,

On Fri, Jan 9, 2026 at 7:59 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Thu, Jan 8, 2026 at 8:05 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >
> > Xuneng Zhou <xunengzhou(at)gmail(dot)com> writes:
> > > On Thu, Jan 8, 2026 at 4:49 PM Neil Chen <carpenter(dot)nail(dot)cz(at)gmail(dot)com> wrote:
> > >> I’ve given this patch a cursory review, and it looks good overall.
> > >> Considering the impact of this change, should we add a regression test for it, or provide a unit test to ensure the correctness of the modification?
> > >> I think it would be more receptive to merging such a "subtle performance optimization" only when its correctness is fully guaranteed.
> >
> > > Thanks for your review. I think we can add a tap test for it.
> >
> > What makes you think this code isn't adequately tested already?
> > The coverage report at
> >
> > https://coverage.postgresql.org/src/backend/replication/logical/snapbuild.c.gcov.html
> >
> > shows SnapBuildPurgeOlderTxn as pretty fully exercised.

I was unaware of this site before. Thanks for informing me.

> > A more interesting question is whether it's worth bothering with
> > at all. This code only runs when we receive a running-xacts WAL
> > record, which is infrequent. So it seems pretty likely to me that
> > nobody could ever detect any performance difference from saving
> > a palloc/pfree here. Perhaps the patch is worth applying on the
> > grounds that it makes the function shorter and clearer, but that's
> > pretty much in the eye of the beholder.
>
> The 0001 patch optimizes the management of catalog-modifying XIDs
> during the decoding of COMMIT records. Instead of doing qsort() for
> every snapshot building, the patch maintains committed.xip in sorted
> order.
>
> While this accelerates logical decoding in scenarios with frequent DDL
> execution, I share Tom's concern: the added complexity seems
> disproportionate for such an infrequent use case. I am also not sure
> that the optimization regarding the CONSISTENT state is necessary.
>
> Furthermore, the patch introduces a palloc()/pfree() overhead for
> every COMMIT record to replace qsort(). As indicated by the
> results[1], this appears to cause a slight regression in common
> workloads dominated by DML.
>

Yes, the current patch feels more like an algorithmic optimization on
its own. If SnapBuildPurgeOlderTxn is not a known hot spot, the added
complexity may outweigh marginal performance benefit from using a more
sophisticated approach such as binary search.

With respect to measuring regression in a CONSISTENT state, my
understanding is that in the presence of a long-running transaction
and a high volume of concurrent DML activity, the time to create a
replication slot is already dominated by the long-running transaction.
In that situation, slot-creation latency is not a reliable metric for
evaluating this change. A sensible way to assess impact would be to
look at CPU-level metrics using tools like perf.

The palloc()/pfree() overhead could potentially be reduced by
techniques such as lazy allocation—only allocating once the first XID
is actually needed—or by using a small fixed-size stack array
initially and falling back to heap allocation only when the capacity
is exceeded. However, both approaches would introduce additional
complexity into the code.

I am more inclined to the simpler approach of in-place compaction
given it is an easy win. WDYT.

--
Best,
Xuneng