Date: Mon, 18 Jan 2021 11:34:43 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: "kuroda.hayato@fujitsu.com" <kuroda.hayato@fujitsu.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: ResourceOwner refactoring
Message-ID: <20210118143443.GA7276@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <7ba71639-dbb7-5eef-e3a2-5f7f0dd22078@iki.fi>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On 2021-Jan-18, Heikki Linnakangas wrote:

> +static ResourceOwnerFuncs jit_funcs =
> +{
> +	/* relcache references */
> +	.name = "LLVM JIT context",
> +	.phase = RESOURCE_RELEASE_BEFORE_LOCKS,
> +	.ReleaseResource = ResOwnerReleaseJitContext,
> +	.PrintLeakWarning = ResOwnerPrintJitContextLeakWarning
> +};

I think you mean jit_resowner_funcs here; "jit_funcs" is a bit
excessively vague.  Also, why did you choose not to define
ResourceOwnerRememberJIT?  You do that in other modules and it seems
better.

> +static ResourceOwnerFuncs catcache_funcs =
> +{
> +	/* catcache references */
> +	.name = "catcache reference",
> +	.phase = RESOURCE_RELEASE_AFTER_LOCKS,
> +	.ReleaseResource = ResOwnerReleaseCatCache,
> +	.PrintLeakWarning = ResOwnerPrintCatCacheLeakWarning
> +};
> +
> +static ResourceOwnerFuncs catlistref_funcs =
> +{
> +	/* catcache-list pins */
> +	.name = "catcache list reference",
> +	.phase = RESOURCE_RELEASE_AFTER_LOCKS,
> +	.ReleaseResource = ResOwnerReleaseCatCacheList,
> +	.PrintLeakWarning = ResOwnerPrintCatCacheListLeakWarning
> +};

Similar naming issue here, I say.


> diff --git a/src/common/cryptohash_openssl.c b/src/common/cryptohash_openssl.c
> index 551ec392b60..642e72d8c0f 100644
> --- a/src/common/cryptohash_openssl.c
> +++ b/src/common/cryptohash_openssl.c
> @@ -60,6 +59,21 @@ struct pg_cryptohash_ctx
>  #endif
>  };
>  
> +/* ResourceOwner callbacks to hold JitContexts  */
> +#ifndef FRONTEND
> +static void ResOwnerReleaseCryptoHash(Datum res);
> +static void ResOwnerPrintCryptoHashLeakWarning(Datum res);

The comment is wrong -- "Crypohashes", not "JitContexts".


So according to your performance benchmark, we're willing to accept a
30% performance loss on an allegedly common operation -- numkeep=0
numsnaps=10 becomes 49.8ns from 37.6ns.  That seems a bit shocking.
Maybe you can claim that these operations aren't exactly hot spots, and
so the fact that we remain in the same power-of-ten is sufficient.  Is
that the argument?

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
"The Postgresql hackers have what I call a "NASA space shot" mentality.
 Quite refreshing in a world of "weekend drag racer" developers."
(Scott Marlowe)