On Wed, Feb 14, 2024 at 2:00 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> On Fri, Jan 12, 2024 at 11:00 PM Robert Haas <robertmhaas@gmail.com> wrote:
> > On Fri, Jan 12, 2024 at 10:12 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> > > Here's one goto-free attempt. It adds a local loop to where the
> > > recursion was, so that if you have a chain of subtransactions that need
> > > to be aborted in CommitTransactionCommand, they are aborted iteratively.
> > > The TBLOCK_SUBCOMMIT case already had such a loop.
> > >
> > > I added a couple of comments in the patch marked with "REVIEWER NOTE",
> > > to explain why I changed some things. They are to be removed before
> > > committing.
> > >
> > > I'm not sure if this is better than a goto. In fact, even if we commit
> > > this, I think I'd still prefer to replace the remaining recursive calls
> > > with a goto. Recursion feels a weird to me, when we're unwinding the
> > > states from the stack as we go.
> >
> > I'm not able to quickly verify whether this version is correct, but I
> > do think the code looks nicer this way.
> >
> > I understand that's a question of opinion rather than fact, though.
>
> I'd like to revive this thread. The attached 0001 patch represents my
> attempt to remove recursion in
> CommitTransactionCommand()/AbortCurrentTransaction() by adding a
> wrapper function. This method doesn't use goto, doesn't require much
> code changes and subjectively provides good readability.
>
> Regarding ShowTransactionStateRec() and memory context function, as I
> get from this thread they are called in states where abortion can lead
> to a panic. So, it's preferable to change them into loops too rather
> than just adding check_stack_depth(). The 0002 and 0003 patches by
> Heikki posted in [1] look good to me. Can we accept them?
>
> Also there are a number of recursive functions, which seem to be not
> used in critical states where abortion can lead to a panic. I've
> extracted them from [2] into an attached 0002 patch. I'd like to push
> it if there is no objection.
The revised set of remaining patches is attached.
0001 Turn tail recursion into iteration in CommitTransactionCommand()
I did minor revision of comments and code blocks order to improve the
readability.
0002 Avoid stack overflow in ShowTransactionStateRec()
I didn't notice any issues, leave this piece as is.
0003 Avoid recursion in MemoryContext functions
I've renamed MemoryContextTraverse() => MemoryContextTraverseNext(),
which I think is a bit more intuitive. Also I fixed
MemoryContextMemConsumed(), which was still trying to use the removed
argument "print" of MemoryContextStatsInternal() function.
Generally, I think this patchset fixes important stack overflow holes.
It is quite straightforward, clear and the code has a good shape. I'm
going to push this if no objections.
------
Regards,
Alexander Korotkov