Re: Improve behavior of concurrent ANALYZE/VACUUM

2018-08-26 Thread Michael Paquier
On Fri, Aug 24, 2018 at 05:30:01PM +, Bossart, Nathan wrote: > On 8/23/18, 9:16 PM, "Michael Paquier" wrote: >> Thanks, I have pushed the new test series, and reused it to check the >> new version of the main patch, which is attached. I have added a commit >> message and I have indented the

Re: Improve behavior of concurrent ANALYZE/VACUUM

2018-08-24 Thread Bossart, Nathan
On 8/23/18, 9:16 PM, "Michael Paquier" wrote: > Thanks, I have pushed the new test series, and reused it to check the > new version of the main patch, which is attached. I have added a commit > message and I have indented the thing. Thanks for the new version! > After pondering about it, I

Re: Improve behavior of concurrent ANALYZE/VACUUM

2018-08-23 Thread Michael Paquier
On Thu, Aug 23, 2018 at 09:53:57PM +, Bossart, Nathan wrote: > This seems reasonable to me. I think establishing the expected > behavior here is a good idea. Thanks, I have pushed the new test series, and reused it to check the new version of the main patch, which is attached. I have added

Re: Improve behavior of concurrent ANALYZE/VACUUM

2018-08-23 Thread Bossart, Nathan
On 8/23/18, 12:08 AM, "Michael Paquier" wrote: > Normal regression tests are less costly than isolation tests, so let's > use them as possible. What you attached is covering only a portion of > all the scenarios though, as it is as well interesting to see what > happens if another user owns only

Re: Improve behavior of concurrent ANALYZE/VACUUM

2018-08-22 Thread Michael Paquier
On Wed, Aug 22, 2018 at 03:49:16PM +, Bossart, Nathan wrote: > 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

Re: Improve behavior of concurrent ANALYZE/VACUUM

2018-08-21 Thread Michael Paquier
On Wed, Aug 22, 2018 at 02:17:44AM +, 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

Re: Improve behavior of concurrent ANALYZE/VACUUM

2018-08-21 Thread Bossart, Nathan
On 8/21/18, 7:44 PM, "Michael Paquier" wrote: > On Tue, Aug 21, 2018 at 04:01:50PM +, 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

Re: Improve behavior of concurrent ANALYZE/VACUUM

2018-08-21 Thread Michael Paquier
On Tue, Aug 21, 2018 at 04:01:50PM +, 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

Re: Improve behavior of concurrent ANALYZE/VACUUM

2018-08-21 Thread Bossart, Nathan
On 8/20/18, 8:29 PM, "Michael Paquier" wrote: >> In short, my vote would be to maintain the current behavior for now >> and to bring up any logging improvements separately. > > On the other hand, it would be useful for the user to know exactly what > is getting skipped. For example if VACUUM

Re: Improve behavior of concurrent ANALYZE/VACUUM

2018-08-20 Thread Michael Paquier
On Mon, Aug 20, 2018 at 08:57:00PM +, Bossart, Nathan wrote: > Sorry for the delay! I looked through the latest patch. Thanks a lot for the review! > I like the idea of emitting a separate WARNING for each for clarity on > what operations are being skipped. However, I think it could be a

Re: Improve behavior of concurrent ANALYZE/VACUUM

2018-08-20 Thread Bossart, Nathan
Hi, Sorry for the delay! I looked through the latest patch. On 8/17/18, 1:43 AM, "Michael Paquier" wrote: > I have reworked the patch on this side, clarifying the use of the new > common API for the logs. One thing I am wondering about is what do we > want to do when VACUUM ANALYZE is used.

Re: Improve behavior of concurrent ANALYZE/VACUUM

2018-08-17 Thread Michael Paquier
On Mon, Aug 13, 2018 at 12:21:42AM +0200, Michael Paquier wrote: > The patch attached includes tests which I have used to also check that > correct error messages are produced for VACUUM, VACUUM ANALYZE and > ANALYZE. I have reworked the patch on this side, clarifying the use of the new common

Re: Improve behavior of concurrent ANALYZE/VACUUM

2018-08-16 Thread Michael Paquier
On Thu, Aug 16, 2018 at 03:07:18PM -0400, Robert Haas wrote: > We definitely don't want to use RangeVarGetRelidExtended more than > necessary. It is important that we use that function only when > necessary - that is, to look up names supplied by users - and it is > also important that we look up

Re: Improve behavior of concurrent ANALYZE/VACUUM

2018-08-16 Thread Robert Haas
On Tue, Aug 14, 2018 at 11:59 AM, Michael Paquier wrote: > On HEAD, we check the ownership of the relation vacuumed or analyzed > after taking a lock on it in respectively vacuum_rel() and > analyze_rel(), where we already know the OID of the relation and there > may be no RangeVar which we could

Re: Improve behavior of concurrent ANALYZE/VACUUM

2018-08-14 Thread Michael Paquier
On Tue, Aug 14, 2018 at 03:26:29PM +, Robert Haas wrote: > I feel like you're not being very clear about exactly what this new > approach is. Sorry if I'm being dense. On HEAD, we check the ownership of the relation vacuumed or analyzed after taking a lock on it in respectively vacuum_rel()

Re: Improve behavior of concurrent ANALYZE/VACUUM

2018-08-14 Thread Robert Haas
On Sun, Aug 12, 2018 at 10:21 PM, Michael Paquier wrote: > In the previous thread, we discussed a couple of approaches, but I was > not happy with any of those, hence I have been spending more time in > getting to a solution which has no user-facing changes, and still solves > the problems folks

Improve behavior of concurrent ANALYZE/VACUUM

2018-08-12 Thread Michael Paquier
Hi all, (In CC are the folks who have reviewed the first patch versions, Nathan and Horiguchi-san.) After TRUNCATE and REINDEX, here is the third and last thread I am spawning for the previous thread "Canceling authentication due to timeout aka Denial of Service Attack":