Re: ResourceOwner refactoring

On 10/07/2023 15:37, Peter Eisentraut wrote:
> 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.

Done.

> 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.

I added a brief comment about that in the ResourceReleasePhase typedef.

I also added a section to the README on how to define your own resource
kind. (The README doesn't go into any detail on how to choose the
release priority though).

> > + .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.

Refactored it that way.

> 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.

Yeah, that would be useful. I remember I've hacked something like that
as a one-off thing in the past, when debugging a leak.

> > +#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?

Many do, but I don't know if it's the majority. We could make
ResurceOwnerForget(NULL) do nothing, but I think it's better to have the
check in the callers. You wouldn't want to silently do nothing when the
resource owner is not expected to be NULL.

(I'm attaching new patch version in my reply to Andres shortly)

--
Heikki Linnakangas
Neon (https://neon.tech)