Re: Proper object locking for GRANT/REVOKE

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