Horton

Thread

Commits

  1. Avoid unexpected changes of CurrentResourceOwner and CurrentMemoryContext

  1. Unexpected changes of CurrentResourceOwner and CurrentMemoryContext

    Antonin Houska <ah@cybertec.at> — 2025-09-03T09:49:32Z

    When working on the REPACK command, we see an ERROR caused by unexpected
change of CurrentResourceOwner [1]. I think the problem is that
reorderbuffer.c does not restore the original value after calling
RollbackAndReleaseCurrentSubTransaction(). The attached patch tries to handle
the call like other callers throughout the tree do.

[1]
https://www.postgresql.org/message-id/CADzfLwUgPMLiFkXRnk97ugPqkDfsNJ3TRdw9gjJM%3D8WB4_nXwQ%40mail.gmail.com

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

  2. Re: Unexpected changes of CurrentResourceOwner and CurrentMemoryContext

    Mihail Nikalayeu <mihailnikalayeu@gmail.com> — 2025-09-04T14:16:20Z

    Hello, Antonin!

    if (using_subtxn)
    {
       RollbackAndReleaseCurrentSubTransaction();
       MemoryContextSwitchTo(ccxt);
       CurrentResourceOwner = cowner;
    }

IIUC memory context is already switched above:

    MemoryContext ecxt = MemoryContextSwitchTo(ccxt);

Best regards,
Mikhail.

  3. Re: Unexpected changes of CurrentResourceOwner and CurrentMemoryContext

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-09-11T18:05:32Z

    On 2025-Sep-03, Antonin Houska wrote:

> When working on the REPACK command, we see an ERROR caused by unexpected
> change of CurrentResourceOwner [1]. I think the problem is that
> reorderbuffer.c does not restore the original value after calling
> RollbackAndReleaseCurrentSubTransaction(). The attached patch tries to handle
> the call like other callers throughout the tree do.

I have registered this as
https://commitfest.postgresql.org/patch/6051/

I've been wondering whether this should be backpatched.  In principle
this is a bugfix, so it should, but I don't offhand recall any cases
where failure to set the current context/resowner in the other
reorderbuffer.c users causes a live bug, so ... maybe master only?  I'm
wondering if it's possible where anybody _depends_ on the current
behavior, but I suppose that's quite unlikely.

It applies cleanly back to 14, so I would probably stop there in any
case.  (A good thing also, because if we mess up 13 with it now and
don't notice until after the next minor is out, there won't be a chance
to unbreak it later.)

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
Essentially, you're proposing Kevlar shoes as a solution for the problem
that you want to walk around carrying a loaded gun aimed at your foot.
(Tom Lane)

  4. Re: Unexpected changes of CurrentResourceOwner and CurrentMemoryContext

    Euler Taveira <euler@eulerto.com> — 2025-09-11T20:38:23Z

    On Thu, Sep 11, 2025, at 3:05 PM, Álvaro Herrera wrote:
> On 2025-Sep-03, Antonin Houska wrote:
>
>> When working on the REPACK command, we see an ERROR caused by unexpected
>> change of CurrentResourceOwner [1]. I think the problem is that
>> reorderbuffer.c does not restore the original value after calling
>> RollbackAndReleaseCurrentSubTransaction(). The attached patch tries to handle
>> the call like other callers throughout the tree do.
>

Interesting. I'm wondering that if this patch is applied we could remove the
following code

		/*
		 * Logical decoding could have clobbered CurrentResourceOwner during
		 * transaction management, so restore the executor's value.  (This is
		 * a kluge, but it's not worth cleaning up right now.)
		 */
		CurrentResourceOwner = old_resowner;

from pg_logical_slot_get_changes_guts and LogicalSlotAdvanceAndCheckSnapState
functions too. IIUC the referred code is a band-aid that will be improved
someday.

> I have registered this as
> https://commitfest.postgresql.org/patch/6051/
>
> I've been wondering whether this should be backpatched.  In principle
> this is a bugfix, so it should, but I don't offhand recall any cases
> where failure to set the current context/resowner in the other
> reorderbuffer.c users causes a live bug, so ... maybe master only?  I'm
> wondering if it's possible where anybody _depends_ on the current
> behavior, but I suppose that's quite unlikely.
>

I would say apply it to master only. If/when we have a bug report we can
backpatch it. Per the crash description, I'm not sure we can create a
reproducible test case with the current supported commands. Am I wrong?

> It applies cleanly back to 14, so I would probably stop there in any
> case.  (A good thing also, because if we mess up 13 with it now and
> don't notice until after the next minor is out, there won't be a chance
> to unbreak it later.)
>

I would also refrain to apply scary fixes into an EOL branch.


-- 
Euler Taveira
EDB   https://www.enterprisedb.com/

  5. Re: Unexpected changes of CurrentResourceOwner and CurrentMemoryContext

    Antonin Houska <ah@cybertec.at> — 2025-09-12T06:46:18Z

    Euler Taveira <euler@eulerto.com> wrote:

> On Thu, Sep 11, 2025, at 3:05 PM, Álvaro Herrera wrote:
> > On 2025-Sep-03, Antonin Houska wrote:
> >
> >> When working on the REPACK command, we see an ERROR caused by unexpected
> >> change of CurrentResourceOwner [1]. I think the problem is that
> >> reorderbuffer.c does not restore the original value after calling
> >> RollbackAndReleaseCurrentSubTransaction(). The attached patch tries to handle
> >> the call like other callers throughout the tree do.
> >
> 
> Interesting. I'm wondering that if this patch is applied we could remove the
> following code
> 
> 		/*
> 		 * Logical decoding could have clobbered CurrentResourceOwner during
> 		 * transaction management, so restore the executor's value.  (This is
> 		 * a kluge, but it's not worth cleaning up right now.)
> 		 */
> 		CurrentResourceOwner = old_resowner;
> 
> from pg_logical_slot_get_changes_guts and LogicalSlotAdvanceAndCheckSnapState
> functions too. IIUC the referred code is a band-aid that will be improved
> someday.

Even though we're fixing the likely reason of this problem, we cannot be 100%
sure that no other problem like this still exists. So I'd not remove this
assignment. Maybe add Assert(CurrentResourceOwner == old_resowner) in front of
that, and adjust the comment?

> > I have registered this as
> > https://commitfest.postgresql.org/patch/6051/
> >
> > I've been wondering whether this should be backpatched.  In principle
> > this is a bugfix, so it should, but I don't offhand recall any cases
> > where failure to set the current context/resowner in the other
> > reorderbuffer.c users causes a live bug, so ... maybe master only?  I'm
> > wondering if it's possible where anybody _depends_ on the current
> > behavior, but I suppose that's quite unlikely.
> >
> 
> I would say apply it to master only. If/when we have a bug report we can
> backpatch it.

+1

> Per the crash description, I'm not sure we can create a
> reproducible test case with the current supported commands. Am I wrong?

It seems so, at least with he "CurrentResourceOwner = old_resowner" assignment
in place. REPACK CONCURRENTLY exposes the problem a bit more because it has at
least one kind of resource open during logical decoding: relation.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

  6. Re: Unexpected changes of CurrentResourceOwner and CurrentMemoryContext

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-09-12T17:10:55Z

    On 2025-Sep-12, Antonin Houska wrote:

> Euler Taveira <euler@eulerto.com> wrote:
> 
> > Interesting. I'm wondering that if this patch is applied we could remove the
> > following code [...] from pg_logical_slot_get_changes_guts and
> > LogicalSlotAdvanceAndCheckSnapState functions too. IIUC the referred
> > code is a band-aid that will be improved someday.
> 
> Even though we're fixing the likely reason of this problem, we cannot be 100%
> sure that no other problem like this still exists. So I'd not remove this
> assignment. Maybe add Assert(CurrentResourceOwner == old_resowner) in front of
> that, and adjust the comment?

Yeah, I'm going to risk removing it, because if we don't do it now,
we're never going to do it.  We can mitigate the risk of missing
remaining bugs by having that assertion you suggest, so that if anyone
actually hits a problem here, we'll know soon enough.

I have pushed it with that change.  I'll also add an open item for pg19
so that we remember to come back to remove the assertions, if we feel we
no longer need them.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Update: super-fast reaction on the Postgres bugs mailing list. The report
was acknowledged [...], and a fix is under discussion.
The wonders of open-source !"
             https://twitter.com/gunnarmorling/status/1596080409259003906

  7. Re: Unexpected changes of CurrentResourceOwner and CurrentMemoryContext

    Antonin Houska <ah@cybertec.at> — 2025-09-12T18:03:36Z

    Álvaro Herrera <alvherre@kurilemu.de> wrote:

> On 2025-Sep-12, Antonin Houska wrote:
> 
> > Euler Taveira <euler@eulerto.com> wrote:
> > 
> > > Interesting. I'm wondering that if this patch is applied we could remove the
> > > following code [...] from pg_logical_slot_get_changes_guts and
> > > LogicalSlotAdvanceAndCheckSnapState functions too. IIUC the referred
> > > code is a band-aid that will be improved someday.
> > 
> > Even though we're fixing the likely reason of this problem, we cannot be 100%
> > sure that no other problem like this still exists. So I'd not remove this
> > assignment. Maybe add Assert(CurrentResourceOwner == old_resowner) in front of
> > that, and adjust the comment?
> 
> Yeah, I'm going to risk removing it, because if we don't do it now,
> we're never going to do it.  We can mitigate the risk of missing
> remaining bugs by having that assertion you suggest, so that if anyone
> actually hits a problem here, we'll know soon enough.
> 
> I have pushed it with that change.  I'll also add an open item for pg19
> so that we remember to come back to remove the assertions, if we feel we
> no longer need them.

I was worried about removing those workarounds because it was not trivial to
diagnose the issue. But it should be ok for the master branch. Thanks.

-- 
Antonin Houska
Web: https://www.cybertec-postgresql.com

  8. Re: Unexpected changes of CurrentResourceOwner and CurrentMemoryContext

    Mihail Nikalayeu <mihailnikalayeu@gmail.com> — 2025-09-12T22:57:52Z

    Hi!

> Hello, Antonin!
>
>     if (using_subtxn)
>     {
>        RollbackAndReleaseCurrentSubTransaction();
>        MemoryContextSwitchTo(ccxt);
>        CurrentResourceOwner = cowner;
>     }
>
> IIUC memory context is already switched above:
>
>     MemoryContext ecxt = MemoryContextSwitchTo(ccxt);
>

Finally realized RollbackAndReleaseCurrentSubTransaction leaves as
with TransactionAbortContext, so, nevermind :)