Обсуждение: Inconsistency in vacuum behavior

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

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.

On Mon, Jan 16, 2023 at 11:18 AM Alexander Pyhalov <a.pyhalov@postgrespro.ru> wrote:

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

--

Regards,

Nikita Malakhov

Postgres Professional 

https://postgrespro.ru/

Nikita Malakhov писал 2023-01-16 17:26:
> 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!

Hi.

The patch seems to solve the issue.
Two minor questions I have:
1) should we error out if HeapTupleIsValid(part_tuple) is false?
2) comment "Check partition relations for vacuum permit" seems to be
broken in some way.

--
Best regards,
Alexander Pyhalov,
Postgres Professional

Nikita Malakhov писал 2023-01-16 20:12:
> Hi,
>
> Currently there is no error in this case, so additional thrown error
> would require a new test.
> Besides, throwing an error here does not make sense - it is just a
> check for a vacuum
> permission, I think the right way is to just skip a relation that is
> not suitable for vacuum.
> Any thoughts or objections?
>

No objections for not throwing an error.

--
Best regards,
Alexander Pyhalov,
Postgres Professional

On Mon, Jan 16, 2023 at 08:12:18PM +0300, Nikita Malakhov wrote:
> Hi,
>
> Currently there is no error in this case, so additional thrown error would
> require a new test.
> Besides, throwing an error here does not make sense - it is just a check
> for a vacuum
> permission, I think the right way is to just skip a relation that is not
> suitable for vacuum.
> Any thoughts or objections?

Could you check if this is consistent between the behavior of VACUUM
FULL and CLUSTER ?  See also Nathan's patches.

--
Justin

Justin Pryzby писал 2023-01-19 04:49:
> On Mon, Jan 16, 2023 at 08:12:18PM +0300, Nikita Malakhov wrote:
>> Hi,
>>
>> Currently there is no error in this case, so additional thrown error
>> would
>> require a new test.
>> Besides, throwing an error here does not make sense - it is just a
>> check
>> for a vacuum
>> permission, I think the right way is to just skip a relation that is
>> not
>> suitable for vacuum.
>> Any thoughts or objections?
>
> Could you check if this is consistent between the behavior of VACUUM
> FULL and CLUSTER ?  See also Nathan's patches.

Hi.

Cluster behaves in a different way - it errors out immediately if
relation is not owned by user. For partitioned rel it would anyway raise
error later.
VACUUM and VACUUM FULL behave consistently after applying Nikita's patch
(for partitioned and regular tables) - issue warning "skipping
TABLE_NAME --- only table or database owner can vacuum it" and return
success status.

--
Best regards,
Alexander Pyhalov,
Postgres Professional

On Mon, Jan 16, 2023 at 11:18:08AM +0300, Alexander Pyhalov wrote:
> Is it intended? Why don't we perform vacuum_is_permitted_for_relation()
> 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