On 19/08/2025 21:21, Nathan Bossart wrote:
On Mon, Jun 23, 2025 at 04:16:09PM -0700, Noah Misch wrote:
On Wed, Jun 11, 2025 at 05:22:53PM +0200, Peter Eisentraut wrote:
There is an open item for PG18 for this. So here is a patch that adds a
comment back, mostly from your descriptions in this thread.
The change to AccessShareLock at least prevents confusing "cache lookup
failed" messages, and might alleviate some security concerns about swapping
in a completely different object concurrently (even if we currently think
this is not an actual problem).
Perhaps. To me, the v17 behavior smells mildly superior to the v18 behavior.
Hmm. I think there has been a general effort to get rid of internal errors
like "cache lookup failed ..." and replace them with proper user-facing
errors. This change seems in line with that.
I have seen some commits along those lines, e.g. d8a0993. They weren't adding
lock acquisitions, though. If we've deliberately incurred lock acquisitions
just to get rid of "cache lookup failed ...", I don't remember those commits.
An alternative, if we wanted to go back to the old behavior (other than
reverting altogether, since I think the refactoring is still valuable),
would be to allow get_object_address() to work with lockmode == NoLock. That
would require a bit of work, but nothing magical.
That seems a bit better to me than your comment-only proposal, but either
could be okay.
There is still an open item for this. After a read-through, I tend to
agree with Noah that it might not be worth incurring extra lock
acquisitions solely in the name of avoiding cache lookup errors. This
seems like something that could perhaps be revisited for v19, but it's
pretty late in the game for v18...
+1 for the comment patch (0001-Improve-objectNamesToOids-comment.patch).
+1 for also doing something like
(0002-WIP-Attempt-to-put-back-intra-grant-inplace.spec-cov.patch), to
keep the test coverage for the "cache lookup failed" error. Maybe add a
new test for that, given that it's pretty hacky.
Those are just comment and test updates, however, so they should not
block the release. I will remove this from the Open Items list.
- Heikki
