Re: Stack overflow issue

From: Alexander Korotkov <aekorotkov(at)gmail(dot)com>
To: Andres Freund <andres(at)anarazel(dot)de>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Egor Chindyaskin <kyzevan23(at)mail(dot)ru>, Sascha Kuhl <yogidabanli(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Stack overflow issue
Date: 2024-04-17 11:37:24
Message-ID: CAPpHfdusLFo=tZ=yLOdCuguesqn-a59+JkT3yEavqFvMWrgGUA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Tue, Apr 16, 2024 at 7:42 PM Alexander Korotkov <aekorotkov(at)gmail(dot)com> wrote:
>
> On Tue, Apr 16, 2024 at 6:35 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2024-04-16 15:45:42 +0300, Alexander Korotkov wrote:
> > > On Tue, Apr 16, 2024 at 1:48 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > > On 2024-03-06 14:17:23 +0200, Alexander Korotkov wrote:
> > > > > 0001 Turn tail recursion into iteration in CommitTransactionCommand()
> > > > > I did minor revision of comments and code blocks order to improve the
> > > > > readability.
> > > >
> > > > After sending
> > > > https://www.postgresql.org/message-id/20240414223305.m3i5eju6zylabvln%40awork3.anarazel.de
> > > > I looked some more at important areas where changes didn't have code
> > > > coverage. One thing I noticed was that the "non-internal" part of
> > > > AbortCurrentTransaction() is uncovered:
> > > > https://anarazel.de/postgres/cov/16-vs-HEAD-2024-04-14/src/backend/access/transam/xact.c.gcov.html#L3403
> > > >
> > > > Which made me try to understand fefd9a3fed2. I'm a bit confused about why
> > > > some parts are handled in CommitCurrentTransaction()/AbortCurrentTransaction()
> > > > and others are in the *Internal functions.
> > > >
> > > > I understand that fefd9a3fed2 needed to remove the recursion in
> > > > CommitTransactionCommand()/AbortCurrentTransaction(). But I don't understand
> > > > why that means having some code in in the non-internal and some in the
> > > > internal functions? Wouldn't it be easier to just have all the state handling
> > > > code in the Internal() function and just break after the
> > > > CleanupSubTransaction() calls?
> > >
> > > I'm not sure I correctly get what you mean. Do you think the attached
> > > patch matches the direction you're pointing? The patch itself is not
> > > final, it requires cleanup and comments revision, just to check the
> > > direction.
> >
> > Something like that, yea. The split does seem less confusing that way to me,
> > but also not 100% certain.
>
> Thank you for your feedback. I'm going to go ahead and polish this patch.

I've invested more time into polishing this. I'm intended to push
this. Could you, please, take a look before?

------
Regards,
Alexander Korotkov

Attachment Content-Type Size
v2-0001-Refactoring-for-CommitTransactionCommand-AbortCur.patch application/octet-stream 7.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alexander Korotkov 2024-04-17 11:39:14 Re: Stack overflow issue
Previous Message Dagfinn Ilmari Mannsåker 2024-04-17 11:07:24 Re: documentation structure