Drop extension failure. #96
Conversation
1. Saves from race condition while doing table scan in object acess hook, which arises when the current drop object is pg_ivm_immv's toast table, which looks up for primary key which already got dropped earlier. 2. Use RangeVarGetRelidExtended(... AccessShareLock ...) instead of get_relname_relid(). That way, DROP EXTENSION can't cause errors in concurrent sessions that are in the middle of running the hook.
|
@yugo-n can you review this? This is a critical bug which breaks basic functionality of extension (CREATE DROP) and users can get stuck with this extension installed with no way out. |
|
@yugo-n did you get a chance to take a look at it? |
|
@ibhaskar2 Thank you for your PR, and sorry for late response. |
The fix in PgIvmObjectAccessHook seems reasonable to me, but I've not understood why we should use RangeVarGetRelidExtended rather than get_relname_relid since it is not clear for mewhat errors occur in what situations.... I'll investigate it a bit more. |
OK, I've understood that if we don't acquire the lock before getting relid of pg_ivm_immv, "DROP TABLE" command could raise an error |
| return pg_ivm_immv_id; | ||
| return RangeVarGetRelidExtended( | ||
| makeRangeVar("pg_catalog", "pg_ivm_immv", -1), | ||
| AccessShareLock, RVR_MISSING_OK, NULL, NULL); | ||
| } |
There was a problem hiding this comment.
I've understood that RangeVarGetRelid is required to avoid the could not open relation ERROR, but PgIvmImmvRelationId is called from another places in pg_ivm, so I would not like to acquire a lock in this function for avoiding unnecessary behaviour changes. Instead, I would like to call RangeVarGetRelid() directly in PgIvmObjectAccessHook. (I think it is better to use RangeVarGetRelid rather than RangeVarGetRelidExtended for making the function call simpler.)
There was a problem hiding this comment.
I would not like to acquire a lock in this function for avoiding unnecessary behaviour changes.
After some considering, I came to think that this is not problem because the lock only conflicts with DROP EXTENSION. Moreover, where PgIvmImmvRelationId is called, AccessShareLock or more stronger lock is acquired right after it, so taking a lock in this function would not cause additional conflicts including dead locks.
| return pg_ivm_immv_pkey_id; | ||
| return RangeVarGetRelidExtended( | ||
| makeRangeVar("pg_catalog", "pg_ivm_immv_pkey", -1), | ||
| AccessShareLock, RVR_MISSING_OK, NULL, NULL); | ||
| } |
There was a problem hiding this comment.
The same discussion as PgIvmImmvRelationId is applied here, too.
Tested in GCE VM with postgres 15