Re: Improve behavior of concurrent TRUNCATE

Hi,

On 8/6/18, 11:59 AM, "Michael Paquier" <michael(at)paquier(dot)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