I took a look at 0001.
On 7/26/18, 12:24 AM, "Michael Paquier" <michael@paquier.xyz> wrote:
> 1) 0001 does the work for TRUNCATE, but using RangeVarGetRelidExtended
> with a custom callback based on the existing truncate_check_rel(). I
> had to split the session-level checks into a separate routine as no
> Relation is available, but that finishes by being a neat result without
> changing any user-facing behavior.
Splitting the checks like this seems reasonable. As you pointed out,
it doesn't change the behavior of the session checks, which AFAICT
aren't necessary for the kind of permissions checks we want to add to
the RangeVarGetRelidExtended() call.
- myrelid = RelationGetRelid(rel);
+ myrelid = RangeVarGetRelidExtended(rv, AccessExclusiveLock,
+ false, RangeVarCallbackForTruncate,
+ NULL);
Should the flags argument be 0 instead of false?
+ /* Nothing to do if the relation was not found. */
+ if (!OidIsValid(relId))
+ return;
+
+ tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relId));
+ if (!HeapTupleIsValid(tuple)) /* should not happen */
+ elog(ERROR, "cache lookup failed for relation %u", relId);
The first time we use this callback, the relation won't be locked, so
isn't it possible that we won't get a valid tuple here? I did notice
that callbacks like RangeVarCallbackForRenameRule,
RangeVarCallbackForPolicy, and RangeVarCallbackForRenameTrigger assume
that the relation can be concurrently dropped, but
RangeVarCallbackOwnsRelation does not. Instead, we assume that the
syscache search will succeed if the given OID is valid. Is this a
bug, or am I missing something?
Nathan