Subject: Inconsistency in vacuum behavior

From: Alexander Pyhalov <[email-masked]>
Date: Mon, 16 Jan 2023 11:18:08 +0300

Hi.

We've run regress isolation tests on partitioned tables and found 
interesting VACUUM behavior. I'm not sure, if it's intended.

In the following example, partitioned tables and regular tables behave 
differently:

CREATE TABLE vacuum_tab (a int) PARTITION BY HASH (a);
CREATE TABLE vacuum_tab_1 PARTITION OF vacuum_tab FOR VALUES WITH 
(MODULUS 2, REMAINDER 0);
CREATE TABLE vacuum_tab_2 PARTITION OF vacuum_tab FOR VALUES WITH 
(MODULUS 2, REMAINDER 1);
CREATE ROLE regress_vacuum_conflict;

In the first session:

begin;
  LOCK vacuum_tab IN SHARE UPDATE EXCLUSIVE MODE;

In the second:
SET ROLE regress_vacuum_conflict;
  VACUUM vacuum_tab;
  WARNING:  permission denied to vacuum "vacuum_tab", skipping it <---- 
hangs here, trying to lock vacuum_tab_1

In non-partitioned case second session exits after emitting warning. In 
partitioned case, it hangs, trying to get locks.
This is due to the fact that in expand_vacuum_rel() we skip parent table 
if vacuum_is_permitted_for_relation(), but don't perform such check for 
its child.
The check will be performed later in vacuum_rel(), but after 
vacuum_open_relation(), which leads to hang in the second session.

Is it intended? Why don't we perform vacuum_is_permitted_for_relation() 
check for inheritors in expand_vacuum_rel()?

-- 
Best regards,
Alexander Pyhalov,
Postgres Professional

---

From: Nikita Malakhov <[email-masked]>
Date: Mon, 16 Jan 2023 16:48:12 +0300

Hi!

I've checked this expand_vacuum_rel() and made a quick fix for this.Here's
the result of the test:

postgres@postgres=# set role regress_vacuum_conflict;
SET
Time: 0.369 ms
postgres@postgres=> vacuum vacuum_tab;
WARNING:  permission denied to vacuum "vacuum_tab", skipping it
WARNING:  permission denied to vacuum "vacuum_tab_1", skipping it
WARNING:  permission denied to vacuum "vacuum_tab_2", skipping it
VACUUM
Time: 0.936 ms
postgres@postgres=>

Looks like it's a subject for a patch.

-- 
Regards,
Nikita Malakhov
Postgres Professional
https://postgrespro.ru/

---

From: Nikita Malakhov <[email-masked]>
Date: Mon, 16 Jan 2023 17:26:20 +0300

Hi!

Here's the patch that fixes this case, please check it out.
The patch adds vacuum_is_permitted_for_relation() check before adding
partition relation to the vacuum list, and if permission is denied the
relation
is not added, so it is not passed to vacuum_rel() and there are no try to
acquire the lock.

Cheers!

On Mon, Jan 16, 2023 at 4:48 PM Nikita Malakhov <[email-masked]> wrote:

> On Mon, Jan 16, 2023 at 11:18 AM Alexander Pyhalov <
> [email-masked]> wrote:
>
>> Hi.
>>

-- 
Regards,
Nikita Malakhov
Postgres Professional
https://postgrespro.ru/

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 20 Jan 2023 17:12:35 -0800

> check for inheritors in expand_vacuum_rel()?

Since no lock is held on the partition, the calls to functions like
object_ownercheck() and pg_class_aclcheck() in
vacuum_is_permitted_for_relation() will produce cache lookup ERRORs if the
relation is concurrently dropped.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com