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

Hi,

On Thu, Nov 14, 2024 at 12:16:52AM +0200, Heikki Linnakangas wrote:
> 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 think it all depends what the inserted code expects as parentOffset value.
I thought it could be better to just use a single line but this is probably just
a matter of taste afterall.

> 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().

Yeah, that's a good idea and that would also provide a more natural representation
of transactions relationships IMHO. I don't see cons doing so, that would not
add extra complexity, just the need to maintain both parent and child pointers
which should be simple enough. I think that would make the code simpler actually.

> > === 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.

Yeah, +1 for using "newsize * sizeof(TransactionId)": that was the main idea
behind the define proposal (change the constant value at only one place if
needed).

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

Yeah, OTOH GetCommittedChildren() is the main reason of 
"If there are no subxacts, *ptr is set to NULL": so maybe instead of moving the
comment, just duplicate it. That's probably a Nit though. 

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

Agree.

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

Yes, there is nothing wrong with the patch, it works.

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

Yeah, that's what I had in mind. It's probably not worth the extra code but
maybe it's worth a comment?

Regards,

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