Re: Unexpected changes of CurrentResourceOwner and CurrentMemoryContext

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