Here's another version of this patchset:
- I squashed the resource release priority changes with the main patch.
- In 0001-Move-a-few-ResourceOwnerEnlarge-calls-for-safety.patch, I
moved things around a little differently. In previous version, I changed
PinBuffer() so that it does ResourceOwnerEnlargeBuffers, so that the
callers don't need to do it. In this version, I went the other
direction, and made the caller responsible for calling both
ResourceOwnerEnlargeBuffers and ReservePrivateRefCountEntry. There were
some callers that had to call those functions before calling PinBuffer()
anyway, because they wanted to avoid the possibility of elog(ERROR), so
it seems better to always make it the caller's responsibility.
More comments below:
On 22/03/2023 16:23, Aleksander Alekseev wrote:
> Certain "should never happen in practice" scenarios seem not to be
> test-covered in resowner.c, particularly:
>
> ```
> elog(ERROR, "ResourceOwnerEnlarge called after release started");
> elog(ERROR, "ResourceOwnerRemember called but array was full");
> elog(ERROR, "ResourceOwnerForget called for %s after release started",
> kind->name);
> elog(ERROR, "%s %p is not owned by resource owner %s",
> elog(ERROR, "ResourceOwnerForget called for %s after release started",
> kind->name);
> elog(ERROR, "lock reference %p is not owned by resource owner %s"
> ```
>
> I didn't check whether these or similar code paths were tested before
> the patch and I don't have a strong opinion on whether we should test
> these scenarios. Personally I'm OK with the fact that these few lines
> are not covered with tests.
They were not covered before. Might make sense to add coverage, I'll
look into that.
> The following procedures are never executed:
>
> * RegisterResourceReleaseCallback()
> * UnregisterResourceReleaseCallback()
>
> And are never actually called anymore due to changes in 0005.
> Previously they were used by contrib/pgcrypto. I suggest dropping this
> part of the API since it seems to be redundant now. This will simplify
> the implementation even further.
There might be extensions out there that use those callbacks, that's why
I left them in place. I'll add a test for those functions in
test_resowner now, so that we still have some coverage for them in core.
> Hmm... it looks like a file is missing in the patchset. When building
> with Autotools:
>
> ```
> ============== running regression test queries ==============
> test test_resowner ... /bin/sh: 1: cannot open
> /home/eax/projects/postgresql/src/test/modules/test_resowner/sql/test_resowner.sql:
> No such file
> ```
Oops, added.
> I wonder why the tests pass when using Meson.
A-ha, it's because I didn't add the new test_resowner directory to the
src/test/modules/meson.build file. Fixed.
Thanks for the review!
--
Heikki Linnakangas
Neon (https://neon.tech)