On 8/22/18, 12:37 AM, "Michael Paquier" <michael@paquier.xyz> wrote:
> On Wed, Aug 22, 2018 at 02:17:44AM +0000, Bossart, Nathan wrote:
>> I think this is doable by locking the table in SHARE mode. That won't
>> conflict with the AccessShareLock that expand_vacuum_rel() obtains,
>> but it will conflict with the ShareUpdateExclusiveLock or
>> AccessExclusiveLock that vacuum_rel() takes.
>
> Good point. Still is that really worth adding? This implies a test
> which has at least two roles, one switching the ownership to the other
> and do so back-and-forth. At least that should be on a different
> isolation spec file to not complicate the first one.
I think so, since this is the only ownership checks we do on
individual partitions. Another simple way to test this would be to
create a partitioned table with a different owner than the partitions
and to run VACUUM as the partitioned table owner. In this case, we
would still rely on the checks in vacuum_rel() and analyze_rel(). IMO
this is a reason to avoid skipping gathering the individual partitions
based upon the ownership of the partitioned table. It's true that
this wouldn't fix the locking issue for partitions, but the
aforementioned edge case is still present with 0002 anyway. Plus, it
would add a bit more consistency to partition handling in VACUUM.
I've attached a patch that applies on top of 0002 that adds a simple
test to exercise the checks in vacuum_rel() and analyze_rel().
+ /*
+ * For VACUUM ANALYZE, both logs could show up, but just generate
+ * information for VACUUM as that would be the first one to
+ * process.
+ */
+ return;
We should probably return false here.
Nathan