Unexpected changes of CurrentResourceOwner and CurrentMemoryContext

Hello, Antonin!

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

IIUC memory context is already switched above:

MemoryContext ecxt = MemoryContextSwitchTo(ccxt);

Best regards,
Mikhail.

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)

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/

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

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

Á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

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 :)