Re: Speed up TransactionIdIsCurrentTransactionId() with lots of subxacts

On 13/11/2024 18:08, Bertrand Drouvot wrote:
> === 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.

IMHO it's good as it is. There's no particular harm if someone inserts
code there, is there?

I wonder if we should add a "child" pointer to TransactionStateData
though. That would make it unnecessary to build the temporary array
here, and it could be used to avoid the recursion in
ShowTransactionStateRec().

> === 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)?

The only other place is in AtStart_Memory(), but there it's used for a
different thing. I think a constant is fine here. It'd probably be good
to change "32 * sizeof(TransactionId)" to "newsize *
sizeof(TransactionId)" though.

> === 3
>
> - /* Copy data into output area. */
> + /*
> + * In CurrentTransactoinIds,
>
> s/CurrentTransactoinIds/CurrentTransactionIds/

thanks!

> 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?

I don't see why. The interface is described in
xactGetCommittedChildren() now, and GetCommittedChildren() just notes
that it's an internal version of xactGetCommittedChildren(). It seems
clear to me that you should look at xactGetCommittedChildren() for more
information.

> 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?

Hmm, I don't know. The subtransaction is in the process of being
aborted. Whether you consider its XID to still belong to the
subtransaction until it's fully aborted is a matter of taste. If you
consider that they still belong to the subtransaction until the
subtransaction's TransactionState is free'd, it makes sense to leave
them alone here.

Before this patch, it made sense to reset all those fields when the
childXids array was free'd, but with this patch, the elements in
CurrentTransactionIds[] are still valid until the TransactionState is
free'd.

What would be the alternative? I don't think it makes sense to go out of
our way to clear the elements in CurrentTransactionIds[]. We could set
them to InvalidXid to make it clear they're not valid anymore, but no
one is supposed to look at elements beyond totalXids anyway. We don't do
such clearing at the end of top transaction either.

Thanks for the review!

--
Heikki Linnakangas
Neon (https://neon.tech)