Re: ResourceOwner refactoring

Michael Paquier <michael@paquier.xyz>

From: Michael Paquier <michael@paquier.xyz>
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, Andres Freund <andres@anarazel.de>
Date: 2020-11-18T08:06:48Z
Lists: pgsql-hackers
On Tue, Nov 17, 2020 at 04:21:29PM +0200, Heikki Linnakangas wrote:
> 2. It's difficult for extensions to use. There is a callback mechanism for
> extensions, but it's much less convenient to use than the built-in
> functions. The pgcrypto code uses the callbacks currently, and Michael's
> patch [2] would move that support for tracking OpenSSL contexts to the core,
> which makes it a lot more convenient for pgcrypto. Wouldn't it be nice if
> extensions could have the same ergonomics as built-in code, if they need to
> track external resources?

+1.  True that the current interface is a bit hard to grasp, one can
easily miss that showing reference leaks is very important if an
allocation happens out of the in-core palloc machinery at commit time.
(The patch you are referring to is not really popular, but that does
not prevent this thread to move on on its own.)

> Attached patch refactors the ResourceOwner internals to do that.

+ * Size of the small fixed-size array to hold most-recently remembered resources.
  */
-#define RESARRAY_INIT_SIZE 16
+#define RESOWNER_ARRAY_SIZE 8
Why did you choose this size for the initial array?

+extern const char *ResourceOwnerGetName(ResourceOwner owner);
This is only used in resowner.c, at one place.

@@ -140,7 +139,6 @@ jit_release_context(JitContext *context)
    if (provider_successfully_loaded)
            provider.release_context(context);

-   ResourceOwnerForgetJIT(context->resowner, PointerGetDatum(context));
[...]
+   ResourceOwnerForget(context->resowner, PointerGetDatum(context), &jit_funcs);
This moves the JIT context release from jit.c to llvm.c.  I think
that's indeed more consistent, and correct.  It would be better to
check this one with Andres, though.

> I haven't done thorough performance testing of this, but with some quick
> testing with Kyotaro's "catcachebench" to stress-test syscache lookups, this
> performs a little better. In that test, all the activity happens in the
> small array portion, but I believe that's true for most use cases.



> Thoughts? Can anyone suggest test scenarios to verify the performance of
> this?

Playing with catcache.c is the first thing that came to my mind.
After that some micro-benchmarking with DSM or snapshots?  I am not
sure if we would notice a difference in a real-life scenario, say even
a pgbench -S with prepared queries.
--
Michael

Commits

  1. Make RelationFlushRelation() work without ResourceOwner during abort

  2. Fix bug in bulk extending temp relation after failure

  3. Add missing PGDLLIMPORT markings

  4. Add test_dsa module.

  5. Clear CurrentResourceOwner earlier in CommitTransaction.

  6. Fix dsa.c with different resource owners.

  7. Fix bug in the new ResourceOwner implementation.

  8. Change pgcrypto to use the new ResourceOwner mechanism.

  9. Use a faster hash function in resource owners.

  10. Make ResourceOwners more easily extensible.

  11. Move a few ResourceOwnerEnlarge() calls for safety and clarity.