On Sat, Mar 08, 2025 at 08:34:40PM +0530, Ayush Vatsa wrote: >> I'm wondering whether setting missing_ok to true is correct here. IIUC we >> should have an AccessShareLock on the index, but I don't know if that's >> enough protection. > > Since we are already opening the relation with rel = relation_open(relOid, > AccessShareLock);, > if relOid does not exist, it will throw an error. If it does exist, we > acquire an AccessShareLock, > preventing it from being dropped. > > By the time we reach IndexGetRelation(), we can be confident that relOid > exists and is > protected by the lock. Given this, it makes sense to keep missing_ok = false > here. > > Let me know if you agree or if you see any scenario where > missing_ok = true would be preferable-I can update the condition > accordingly.
Right, we will have a lock on the index, but my concern is that we won't have a lock on its table. I was specifically concerned that a concurrent DROP TABLE could cause IndexGetRelation() to fail, i.e., emit a gross "cache lookup failed" error. From a quick test and skim of the relevant code, I think your patch is fine, though. IndexGetRelation() retrieves the table OID from pg_index, so the OID should definitely be valid. And IIUC DROP TABLE first acquires a lock on the table and its dependent objects (e.g., indexes) before any actual deletions, so AFAICT there's no problem with using it in pg_class_aclcheck() and get_rel_name(), either. -- nathan
