Horton

Re: Stack overflow issue

Alexander Korotkov <aekorotkov@gmail.com>

View thread View on postgr.es
From: Alexander Korotkov <aekorotkov@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>, Egor Chindyaskin <kyzevan23@mail.ru>, Sascha Kuhl <yogidabanli@gmail.com>, pgsql-hackers@lists.postgresql.org
Date: 2024-02-14T12:00:06Z
Lists: pgsql-hackers

Attachments

Hi!

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.

Links.
1. https://www.postgresql.org/message-id/6b48c746-9704-46dc-b9be-01fe4137c824%40iki.fi
2. https://www.postgresql.org/message-id/4530546a-3216-eaa9-4c92-92d33290a211%40mail.ru

------
Regards,
Alexander Korotkov

Commits

  1. Refactoring for CommitTransactionCommand()/AbortCurrentTransaction()

  2. Avoid recursion in MemoryContext functions

  3. Avoid stack overflow in ShowTransactionStateRec()

  4. Turn tail recursion into iteration in CommitTransactionCommand()

  5. Adjust memory allocation functions to allow sibling calls

  6. Add missing check_stack_depth() to some recursive functions

  7. In the Snowball dictionary, don't try to stem excessively-long words.

  8. Defend against stack overrun in a few more places.