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