>>> Yes of course, I’d be happy to take a look. >> Attached is the patch. It's pretty small and passes regress tests. >> > Nice one. > Few comments on it: > #1: > + oldcontext = MemoryContextSwitchTo(VfdCxt); > + newVfdCache = repalloc_array(VfdCache, Vfd, newCacheSize); > + MemoryContextSwitchTo(oldcontext); > > Can't we just do this? > + newVfdCache = repalloc_array(VfdCache, Vfd, newCacheSize); > > because repalloc doesn't need CurrentMemoryContext.
Yes. You can then make it even simpler and get rid of newVfdCache via
> #2: > + newDescs = MemoryContextAllocExtended(VfdCxt, > + newMax * sizeof(AllocateDesc), MCXT_ALLOC_NO_OOM); > if (newDescs == NULL) > return false; > + memcpy(newDescs, allocatedDescs, maxAllocatedDescs * > sizeof(AllocateDesc)); > + pfree(allocatedDescs); > > We can also just replace it with: > + newDescs = repalloc_array_extended(allocatedDescs, AllocateDesc, > + newMax, MCXT_ALLOC_NO_OOM); > Correct?
Yes.
v7-0001: looks good to me.
v7-0003: I'm wondering if we still want cache_bytes in pg_stat_vfdcache, now where it's exposed via pg_backend_memory_contexts. It seems to me that other stats functionality also doesn't expose memory info that is accessible via pg_backend_memory_contexts. But I'm not completely sure what's best here.
pg_backend_memory_contexts is local to current backend session, so AFAIK there's no other way of getting cluster-wide vfd cache memory usage. Therefore we need cache_bytes for this?
