|From:||Michael Paquier <michael(at)paquier(dot)xyz>|
|To:||"Bossart, Nathan" <bossartn(at)amazon(dot)com>|
|Cc:||Postgres hackers <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: Improve behavior of concurrent TRUNCATE|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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
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
> - 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,
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
Attached is an updated patch with all your suggestions added.
|Next Message||Dean Rasheed||2018-08-09 11:02:06||Re: BUG #15307: Low numerical precision of (Co-) Variance|
|Previous Message||KES||2018-08-09 10:11:05||Re: Typo in doc or wrong EXCLUDE implementation|