database/postgres
database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2025-04-29 13:56:47 +03:00
Code

Tidy up GetMultiXactIdMembers()'s behavior on error

Browse Source 
One of the error paths left *members uninitialized. That's not a live
bug, because most callers don't look at *members when the function
returns -1, but let's be tidy. One caller, in heap_lock_tuple(), does
"if (members != NULL) pfree(members)", but AFAICS it never passes an
invalid 'multi' value so it should not reach that error case.

The callers are also a bit inconsistent in their expectations.
heap_lock_tuple() pfrees the 'members' array if it's not-NULL, others
pfree() it if "nmembers >= 0", and others if "nmembers > 0". That's
not a live bug either, because the function should never return 0, but
add an Assert for that to make it more clear. I left the callers alone
for now.

I also moved the line where we set *nmembers. It wasn't wrong before,
but I like to do that right next to the 'return' statement, to make it
clear that it's always set on return.

Also remove one unreachable return statement after ereport(ERROR), for
brevity and for consistency with the similar if-block right after it.

Author: Greg Nancarrow with the additional changes by me
Backpatch-through: 9.6, all supported versions
This commit is contained in:
Heikki Linnakangas 2021-06-17 14:50:42 +03:00
parent 9c31e41655
commit 009ee51af7
1 changed files with 8 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
src/backend/access/transam/multixact.c
View File

@ -1220,7 +1220,10 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
debug_elog3(DEBUG2, "GetMembers: asked for %u", multi); debug_elog3(DEBUG2, "GetMembers: asked for %u", multi);
if (!MultiXactIdIsValid(multi) || from_pgupgrade) if (!MultiXactIdIsValid(multi) || from_pgupgrade)
{
*members = NULL;
return -1; return -1;
}
/* See if the MultiXactId is in the local cache */ /* See if the MultiXactId is in the local cache */
length = mXactCacheGetById(multi, members); length = mXactCacheGetById(multi, members);
@ -1271,13 +1274,10 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
LWLockRelease(MultiXactGenLock); LWLockRelease(MultiXactGenLock);
if (MultiXactIdPrecedes(multi, oldestMXact)) if (MultiXactIdPrecedes(multi, oldestMXact))
{
ereport(ERROR, ereport(ERROR,
(errcode(ERRCODE_INTERNAL_ERROR), (errcode(ERRCODE_INTERNAL_ERROR),
errmsg("MultiXactId %u does no longer exist -- apparent wraparound", errmsg("MultiXactId %u does no longer exist -- apparent wraparound",
multi))); multi)));
return -1;
}
if (!MultiXactIdPrecedes(multi, nextMXact)) if (!MultiXactIdPrecedes(multi, nextMXact))
ereport(ERROR, ereport(ERROR,
@ -1377,7 +1377,6 @@ retry:
LWLockRelease(MultiXactOffsetControlLock); LWLockRelease(MultiXactOffsetControlLock);
ptr = (MultiXactMember *) palloc(length * sizeof(MultiXactMember)); ptr = (MultiXactMember *) palloc(length * sizeof(MultiXactMember));
*members = ptr;
/* Now get the members themselves. */ /* Now get the members themselves. */
LWLockAcquire(MultiXactMemberControlLock, LW_EXCLUSIVE); LWLockAcquire(MultiXactMemberControlLock, LW_EXCLUSIVE);
@ -1422,6 +1421,9 @@ retry:
LWLockRelease(MultiXactMemberControlLock); LWLockRelease(MultiXactMemberControlLock);
/* A multixid with zero members should not happen */
Assert(truelength > 0);
/* /*
* Copy the result into the local cache. * Copy the result into the local cache.
*/ */
@ -1429,6 +1431,7 @@ retry:
debug_elog3(DEBUG2, "GetMembers: no cache for %s", debug_elog3(DEBUG2, "GetMembers: no cache for %s",
mxid_to_string(multi, truelength, ptr)); mxid_to_string(multi, truelength, ptr));
*members = ptr;
return truelength; return truelength;
} }
@ -1529,7 +1532,6 @@ mXactCacheGetById(MultiXactId multi, MultiXactMember **members)
size = sizeof(MultiXactMember) * entry->nmembers; size = sizeof(MultiXactMember) * entry->nmembers;
ptr = (MultiXactMember *) palloc(size); ptr = (MultiXactMember *) palloc(size);
*members = ptr;
memcpy(ptr, entry->members, size); memcpy(ptr, entry->members, size);
@ -1545,6 +1547,7 @@ mXactCacheGetById(MultiXactId multi, MultiXactMember **members)
*/ */
dlist_move_head(&MXactCache, iter.cur); dlist_move_head(&MXactCache, iter.cur);
*members = ptr;
return entry->nmembers; return entry->nmembers;
} }
} }