Hi,
On 8/6/18, 11:59 AM, "Michael Paquier" <michael@paquier.xyz> wrote:
> Attached is a patch I have been working on which refactors the code of
> TRUNCATE in such a way that we check for privileges before trying to
> acquire a lock, without any user-facing impact (I have reworked a couple
> of comments compared to the last version). This includes a set of tests
> showing the new behavior.
Here are some comments for the latest version of the patch.
-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 (rel->rd_rel->relkind != RELKIND_RELATION &&
- rel->rd_rel->relkind != RELKIND_PARTITIONED_TABLE)
+ if (reltuple->relkind != RELKIND_RELATION &&
+
+ reltuple->relkind != RELKIND_PARTITIONED_TABLE)
There appears to be an extra empty line here.
+# 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.
> Like cbe24a6, perhaps we would not want to back-patch it? Based on the
> past history (and the consensus being reached for the REINDEX case would
> be to patch only HEAD), I would be actually incline to not back-patch
> this stuff and qualify that as an improvement. That's also less work
> for me at commit :)
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.
Nathan
