Re: ResourceOwner refactoring

From: Peter Eisentraut <peter(at)eisentraut(dot)org>
To: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Aleksander Alekseev <aleksander(at)timescale(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Cc: Andres Freund <andres(at)anarazel(dot)de>, Julien Rouhaud <rjuju123(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>
Subject: Re: ResourceOwner refactoring
Date: 2023-07-10 12:37:23
Message-ID: a54c746c-d956-e712-b4e7-4df2bfd61821@eisentraut.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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?

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Matthias van de Meent 2023-07-10 12:38:11 Re: PATCH: Using BRIN indexes for sorted output
Previous Message Aleksander Alekseev 2023-07-10 12:33:27 Re: HOT readme missing documentation on summarizing index handling