Re: Speed up TransactionIdIsCurrentTransactionId() with lots of subxacts - Mailing list pgsql-hackers

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)