On 8/9/18, 5:29 AM, "Michael Paquier" <[email protected]> wrote:
>> -truncate_check_rel(Relation rel)
>> +truncate_check_rel(Oid relid, Form_pg_class reltuple)
>>
>> Could we use HeapTupleGetOid(reltuple) instead of passing the OID
>> separately?
>
> If that was a HeapTuple. And it seems to me that we had better make
> truncate_check_rel() depend directly on a Form_pg_class instead of
> allowing the caller pass anything and deform the tuple within the
> routine.
Got it. I agree, it makes sense to force the caller to provide a
Form_pg_class.
>> +# Test for lock lookup conflicts with TRUNCATE when working on unowned
>> +# relations, particularly catalogs should trigger an ERROR for all the
>> +# scenarios present here.
>>
>> If possible, it might be worth it to check that TRUNCATE still blocks
>> when a role has privileges, too.
>
> Good idea. I have added more tests for that. Doing a truncate on
> pg_authid for installcheck was a very bad idea anyway (even if it failed
> all the time), so I have switched to a custom table, with a GRANT
> command to control the access with a custom role.
Good call. Accidentally truncating pg_authid might have led to some
interesting test results...
>> Since the only behavior this patch changes is the order of locking and
>> permission checks, my vote would be to back-patch. However, since the
>> REINDEX piece won't be back-patched, I can see why we might not here,
>> either.
>
> This patch is a bit more invasive than the REINDEX one to be honest, and
> as this is getting qualified as an improvement, at least on this thread,
> I would be inclined to be more restrictive and just patch HEAD, but not
> v11.
Alright.
> Attached is an updated patch with all your suggestions added.
Thanks! This patch builds cleanly, the new tests pass, and my manual
testing hasn't uncovered any issues. Notably, I cannot reproduce the
originally reported authentication issue by using TRUNCATE after this
change. Beyond a few small comment wording suggestions below, it
looks good to me.
+ /*
+ * RangeVarGetRelidExtended has done some basic checks with its
+ * callback, and only remain session-level checks.
+ */
This is definitely a nitpick, but I might rephrase this to "...so only
session-level checks remain" or to "...but we still need to do
session-level checks."
+/*
+ * Set of extra sanity checks to check if a given relation is safe to
+ * truncate. This is split with truncate_check_rel as
+ * RangeVarCallbackForTruncate can only call the former.
+ */
+static void
+truncate_check_activity(Relation rel)
It might be worth mentioning why RangeVarCallbackForTruncate can only
call truncate_check_rel() (we haven't opened the relation yet).
+# Test for lock lookup conflicts with TRUNCATE when working on unowned
+# relations, particularly catalogs should trigger an ERROR for all the
+# scenarios present here.
Since we are testing a few more things now, perhaps this could just be
"Test for locking conflicts with TRUNCATE commands."
+# TRUNCATE will directly fail
Maybe we could say something more like this:
If the role doesn't have privileges to TRUNCATE the table,
TRUNCATE should immediately fail without waiting for a lock.
+# TRUNCATE will block
+permutation "s1_begin" "s1_tab_lookup" "s2_grant" "s2_auth" "s2_truncate"
"s1_commit" "s2_reset"
+permutation "s1_begin" "s2_grant" "s2_auth" "s2_truncate" "s1_tab_lookup"
"s1_commit" "s2_reset"
+permutation "s1_begin" "s2_grant" "s2_auth" "s1_tab_lookup" "s2_truncate"
"s1_commit" "s2_reset"
+permutation "s2_grant" "s2_auth" "s2_truncate" "s1_begin" "s1_tab_lookup"
"s1_commit" "s2_reset"
The second and fourth tests don't seem to actually block. Perhaps we
could reword the comment to say something like this:
If the role has privileges to TRUNCATE the table, TRUNCATE
will block if another session holds a lock on the table.
Nathan
