On Wed, Aug 08, 2018 at 03:38:57PM +0000, Bossart, Nathan wrote:
> Here are some comments for the latest version of the patch.
Thanks for the review, Nathan!
> -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.
> - 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.
Fixed. I don't know where this has come from.
> +# 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.
> 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.
Attached is an updated patch with all your suggestions added.
--
Michael