Re: [Proposal] Level4 Warnings show many shadow vars

> On 9 Dec 2019, at 12:02, Ranier Vilela <ranier_gyn(at)hotmail(dot)com> wrote:

> diff --git a / src / backend / access / transam / multixact.c b / src / backend / access / transam / multixact.c
> index 7b2448e05b..6364014fb3 100644
> --- a / src / backend / access / transam / multixact.c
> +++ b / src / backend / access / transam / multixact.c
> @@ -1589.10 +1589.10 @@ mXactCachePut (MultiXactId multi, int nmembers, MultiXactMember * members)
> qsort (entry-> members, nmembers, sizeof (MultiXactMember), mxactMemberComparator);
>
> dlist_push_head (& MXactCache, & entry-> node);
> + pfree (entry); // <- is it really necessary?

Pushing an object to a dlist doesn't copy the object, so freeing entry here
would cause a dangling pointer on the list unless I'm misreading. Note that
entry is allocated in a specific context to ensure it has the correct lifespan.
The README in backend/utils/mmgr is a good primer on how memory contexts work
in postgres.

As a matter of fact, the pfree call in the cache purge if block isn't really
required either since the entire cache will be freed at the end of the
transaction.

> if (MXactCacheMembers ++> = MAX_CACHE_ENTRIES)
> {
> dlist_node * node;
> - mXactCacheEnt * entry;

I can agree that reusing the name entry here isn't ideal, as it's so close, but
removing it is worse. I'd prefer to rename it purged, or purged_entry or
something along those lines.

cheers ./daniel