Re: Stale hash entries when reusing an auxiliary resource owner

Hi Andres,

> I don't think fixing this locally in ReleaseAuxProcessResources() can be the
> right fix. It's just continuing an existing pattern, admittedly. But:

My original patch aimed to keep the fix local, since the auxiliary
resource owner is currently the only owner reused after bulk release.
That was why I avoided changes to the generic ResourceOwner code.

> It seems quite wrong that ReleaseAuxProcessResources() does stuff that callers
> outside of resowner.c can't do. What if there's a need for a long-lived,
> reusable, resowner outside of a context where using AuxProcessResourceOwner is
> right? Seems like we should just have a helper that makes a resowner reusable
> after some assertions?

Since we have decided to change the API, I think we could add a public
ResourceOwnerReset() interface, by analogy with MemoryContext. After
completing all three release phases, callers could either delete the
owner or reset it for reuse:

Create -> Use -> Release (all three phases) -> Delete
^ |
| v
+----------- Reset

For the implementation, I see two options. One is to clear stale hash
entries during sorting, as you suggested, and clear each entry during
bulk release. Reset would then only restore the internal state. This
also adds some work for owners that are deleted without being reset.

The other is to clear the hash table in Reset, so only owners being
reset for reuse pay that cost.

> As for the concrete bug at hand - why don't we fix this up in
> ResourceOwnerSort()? ReleaseAuxProcessResources() shouldn't be that common,
> but always memset()ing the full size of the hash can't be great? And even just
> for debugging in non-auxprocess context, it doesn't seem awesome to have a
> hashtable with obsoleted entries still visible?
>
> The fast-path is the !hash case, I don't think any realistic workload would
> notice the overhead of zeroing out hash table entries during the compaction in
> ResourceOwnerSort()? I don't know if we'd need to do anything else?

For now, the latter seems more reasonable to me, since Reset is
specifically responsible for preparing a released owner for reuse.
Clearing the retained hash table fits that responsibility. Also, Reset
would currently only be used for the auxiliary owner, whose cleanup
seems less performance-critical than the general ResourceOwner release
path.

I'm not sure which is better overall, so I have prepared patches
for both approaches.

---
Regards,
Jingtang