Hi, On 8/6/18, 11:59 AM, "Michael Paquier" <mich...@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
