From: Peter Eisentraut <peter@eisentraut.org>
To: Heikki Linnakangas <hlinnaka@iki.fi>
To: Aleksander Alekseev <aleksander@timescale.com>
To: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Cc: Andres Freund <andres@anarazel.de>
Cc: Julien Rouhaud <rjuju123@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Subject: Re: ResourceOwner refactoring
Date: Mon, 10 Jul 2023 14:37:23 +0200
Message-ID: <a54c746c-d956-e712-b4e7-4df2bfd61821@eisentraut.org> (raw)
In-Reply-To: <527af829-4a03-03b0-121c-46ff79c61887@iki.fi>
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>
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?
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: peter@eisentraut.org, hlinnaka@iki.fi, aleksander@timescale.com, andres@anarazel.de, rjuju123@gmail.com, robertmhaas@gmail.com
Subject: Re: ResourceOwner refactoring
In-Reply-To: <a54c746c-d956-e712-b4e7-4df2bfd61821@eisentraut.org>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox