Re: Speed up TransactionIdIsCurrentTransactionId() with lots of subxacts

Hi,

On Fri, Oct 11, 2024 at 12:21:12AM +0300, Heikki Linnakangas wrote:
> > +   98.52%    98.45%  postgres  postgres           [.]
> > TransactionIdIsCurrentTransactionId
> In this scenario with lots of active subtransactions,
> TransactionIdIsCurrentTranactionId effectively degenerates into a linear
> search over a linked list, as it traverses each level in the
> CurrentTransactionState stack.

Agree.

> The patch

Thanks for the patch!

> Instead of having a separate childXids array on each transaction level, we
> can maintain one large array of XIDs that includes the XIDs of all committed
> and in-progress subtransactions. On each nesting level, remember the offset
> within that array, so that all XIDs belonging to that nesting level or its
> parents are above that offset.
> When a subtransaction commits, you don't need
> to copy XIDs to the parent, you just adjust the parent's offset into the
> array, to include the child's XIDs.

Yeah, makes sense. And in case of subtransaction rollback, that's fine because
all the subtransactions down to this one can be/are "discarded".

> Patch attached,

A few random comments:

=== 1

+  parentOffset--;
+  parents[parentOffset]->totalXids = totalXids;

what about "parents[--parentOffset]->totalXids = totalXids;" instead?

That would remove the ability for someone to insert code between the decrement
and the array access.

=== 2

+  newsize = 32;
+  CurrentTransactionIds = MemoryContextAlloc(TopTransactionContext,
+                                             32 * sizeof(TransactionId));

what about defining a macro for "32" (as it is used in multiple places in
xact.c)?

=== 3

-       /* Copy data into output area. */
+               /*
+                * In CurrentTransactoinIds,

s/CurrentTransactoinIds/CurrentTransactionIds/

4 ===

 int
 xactGetCommittedChildren(TransactionId **ptr)
 {
-       TransactionState s = CurrentTransactionState;
+       return GetCommittedChildren(CurrentTransactionState, ptr);

Worth to move this part of the comment on top of xactGetCommittedChildren(),

"
If there are no subxacts, *ptr is set to NULL.
"

on top of GetCommittedChildren() instead?

5 ===

 static void
 AtSubAbort_childXids(void)
 {
-       TransactionState s = CurrentTransactionState;
-
-       /*
-        * We keep the child-XID arrays in TopTransactionContext (see
-        * AtSubCommit_childXids).  This means we'd better free the array
-        * explicitly at abort to avoid leakage.
-        */
-       if (s->childXids != NULL)
-               pfree(s->childXids);
-       s->childXids = NULL;
-       s->nChildXids = 0;
-       s->maxChildXids = 0;
+       /* nothing to do */

Yeah that works but then some CurrentTransactionIds[] elements do not reflect
the "reality" anymore (until the top level transaction finishes, or until a
new subtransaction is created). 

Could'nt that be an issue from a code maintability point of view?

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com