Feedback-ID: ie0a040ee:Fastmail
Message-ID: <a54c746c-d956-e712-b4e7-4df2bfd61821@eisentraut.org>
Date: Mon, 10 Jul 2023 14:37:23 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.12.0
Subject: Re: ResourceOwner refactoring
Content-Language: en-US
To: Heikki Linnakangas <hlinnaka@iki.fi>,
 Aleksander Alekseev <aleksander@timescale.com>,
 "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Cc: Andres Freund <andres@anarazel.de>, Julien Rouhaud <rjuju123@gmail.com>,
 Robert Haas <robertmhaas@gmail.com>
References: <908491f9-57ff-5061-30c6-d15d3dbf0e67@iki.fi>
 <CALNJ-vTh=O0O0ENOMC4uGd1gbzcnHaXTi8W4W=qri5Na3-LMKw@mail.gmail.com>
 <CAJ7c6TP8v=2Ym5S1C4YenaCMgwu5FrFTGi40Nn5TyDS4ig-gRA@mail.gmail.com>
 <1377f062-13ea-ebfb-ef8a-97739c156a05@iki.fi>
 <CAJ7c6TOj8RsUkfnVZnAPZ6jgWxqTLuv-o5mHYTc5goEhfBXaTA@mail.gmail.com>
 <CAJ7c6TNqFTFBw5KF55TPQAMHYuct5NZP7afohVxReBLXfUOyOQ@mail.gmail.com>
 <CAOBaU_Z-NaqzAv=9YntFjbJhgWkJDAwCVyzvKUQTJ2UKgK4AOw@mail.gmail.com>
 <2e10b71b-352e-b97b-1e47-658e2669cecb@iki.fi>
 <20221101001523.lsgmohl6fjwppbgx@awork3.anarazel.de>
 <9a3245db-ca03-4a47-5706-73404cfe5108@iki.fi>
 <20221101154251.64vyq7wy6w7aykrn@awork3.anarazel.de>
 <bf88d2a9-a7bc-3d68-57e3-abfba87832e9@iki.fi>
 <CAJ7c6TNijrJ+0hnTiAPDBn-tCiJsk-yvpF3Y8rXG4gTw09JYbg@mail.gmail.com>
 <d17d4d6b-c39c-7e40-9f3d-b09f19e5ccec@iki.fi>
 <CAJ7c6TM_9XtQX6xAGkH2u2-b20z-1rii9szMdW-aMHJZC_NOsA@mail.gmail.com>
 <527af829-4a03-03b0-121c-46ff79c61887@iki.fi>
From: Peter Eisentraut <peter@eisentraut.org>
In-Reply-To: <527af829-4a03-03b0-121c-46ff79c61887@iki.fi>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/a54c746c-d956-e712-b4e7-4df2bfd61821%40eisentraut.org>
Precedence: bulk

A few suggestions on the API:

 > +static ResourceOwnerFuncs tupdesc_resowner_funcs =

These aren't all "functions", so maybe another word like "info" or 
"description" would be more appropriate?


 > +   .release_phase = RESOURCE_RELEASE_AFTER_LOCKS,
 > +   .release_priority = RELEASE_PRIO_TUPDESC_REFS,

I suggest assigning explicit non-zero numbers to the RESOURCE_RELEASE_* 
constants, as well as changing RELEASE_PRIO_FIRST to 1 and adding an 
assertion that the priority is not zero.  Otherwise, someone might 
forget to assign these fields and would implicitly get phase 0 and 
priority 0, which would probably work in most cases, but wouldn't be the 
explicit intention.

Then again, you are using RELEASE_PRIO_FIRST for the pgcrypto patch.  Is 
it the recommendation that if there are no other requirements, external 
users should use that?  If we are going to open this up to external 
users, we should probably have some documented guidance around this.


 > +   .PrintLeakWarning = ResOwnerPrintTupleDescLeakWarning

I think this can be refactored further.  We really just need a function 
to describe a resource, like maybe

static char *
ResOwnerDescribeTupleDesc(Datum res)
{
     TupleDesc   tupdesc = (TupleDesc) DatumGetPointer(res);

     return psprintf("TupleDesc %p (%u,%d)",
                     tupdesc, tupdesc->tdtypeid, tupdesc->tdtypmod);
}

and then the common code can generate the warnings from that like

     elog(WARNING,
          "%s leak: %s still referenced",
          kind->name, kind->describe(value));

That way, we could more easily develop further options, such as turning 
this warning into an error (useful for TAP tests).

Also, maybe, you could supply a default description that just prints 
"%p", which appears to be applicable in about half the places.

Possible bonus project: I'm not sure these descriptions are so useful 
anyway.  It doesn't help me much in debugging if "File 55 was not 
released" or some such.  If would be cool if ResourceOwnerRemember() in 
some debug mode could remember the source code location, and then print 
that together with the leak warning.

 > +#define ResourceOwnerRememberTupleDesc(owner, tupdesc) \
 > +   ResourceOwnerRemember(owner, PointerGetDatum(tupdesc), 
&tupdesc_resowner_funcs)
 > +#define ResourceOwnerForgetTupleDesc(owner, tupdesc) \
 > +   ResourceOwnerForget(owner, PointerGetDatum(tupdesc), 
&tupdesc_resowner_funcs)

I would prefer that these kinds of things be made inline functions, so 
that we maintain the type safety of the previous interface.  And it's 
also easier when reading code to see type decorations.

(But I suspect the above bonus project would require these to be macros?)


 > +   if (context->resowner)
 > +       ResourceOwnerForgetJIT(context->resowner, context);

It seems most ResourceOwnerForget*() calls have this kind of check 
before them.  Could this be moved inside the function?