From: Michael Paquier <michael@paquier.xyz>
To: Bossart, Nathan <bossartn@amazon.com>
Cc: Postgres hackers <pgsql-hackers@postgresql.org>
Cc: horiguchi.kyotaro@lab.ntt.co.jp <horiguchi.kyotaro@lab.ntt.co.jp>
Subject: Re: Improve behavior of concurrent ANALYZE/VACUUM
Date: Wed, 22 Aug 2018 09:43:35 +0900
Message-ID: <20180822004335.GA4333@paquier.xyz> (raw)
In-Reply-To: <CAA861D8-68C1-4A2D-84E9-E2EE6B7B6A72@amazon.com>
References: <20180812222142.GA6097@paquier.xyz>
<20180817064223.GH1693@paquier.xyz>
<720DF5E4-053F-46C7-B11F-550EF2C39F6D@amazon.com>
<20180821012815.GB2897@paquier.xyz>
<CAA861D8-68C1-4A2D-84E9-E2EE6B7B6A72@amazon.com>
On Tue, Aug 21, 2018 at 04:01:50PM +0000, Bossart, Nathan wrote:
> I think my biggest concern with this approach is that we'd be
> introducing inconsistent behavior whenever there are concurrent
> changes. If a user never had permissions to VACUUM the partitioned
> table, the partitions are skipped outright. However, if the user
> loses permissions to VACUUM the partitioned table between
> expand_vacuum_rel() and vacuum_rel(), we'll still attempt to VACUUM
> each individual partition.
>
> I'll admit I don't have a great alternative proposal that doesn't
> involve adding deadlock risk or complexity, but it still seems worth
> mulling over.
That counts only for a manual vacuum/analyze listing directly the
relation in question. If running a system-wide VACUUM then all the
relations are still processed. This is a rather edge case in my opinion
but.. I don't mind mulling over it (as you say). So please let me
think over it for a couple of days. I don't see a smart solution which
does not create risks of lock upgrades and deadlocks now, there may be
one able to preserve the existing behavior.
>> I have split the patch into two parts:
>> - 0001 includes new tests which generate WARNING messages for VACUUM,
>> ANALYZE and VACUUM (ANALYZE). That's useful separately.
>
> 0001 looks good to me.
Thanks, I have pushed this one.
>> - 0002 is the original patch discussed here.
>
> I'd suggest even splitting 0002 into two patches: one for refactoring
> the existing permissions checks into vacuum_is_relation_owner() and
> another for the new checks.
Hmmm. The second patch changes also some comment blocks when calling
vacuum_is_relation_owner(), so we finish by changing the same code
areas, resulting in more code churn for no real gain.
> +# The role doesn't have privileges to vacuum the table, so VACUUM should
> +# immediately skip the table without waiting for a lock.
>
> Can we add tests for concurrent changes that cause the relation to be
> skipped in vacuum_rel() and analyze_rel() instead of
> expand_vacuum_rel()?
Doing that deterministically with concurrent tests look difficult to me
as doing ALTER TABLE OWNER TO to a relation in a first session causes a
second session running VACUUM to block in expand_vacuum_rel(), be it
with a plain table or a partitioned table (doing the ALTER TABLE on a
leaf will block scanning the parent as well).
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../20180822004335.GA4333@paquier.xyz/2-signature.asc)
download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: michael@paquier.xyz, bossartn@amazon.com, horiguchi.kyotaro@lab.ntt.co.jp
Subject: Re: Improve behavior of concurrent ANALYZE/VACUUM
In-Reply-To: <20180822004335.GA4333@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox