Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-10-03 Thread Michael Paquier
On Wed, Oct 4, 2017 at 9:52 AM, Bossart, Nathan wrote: > On 10/3/17, 5:59 PM, "Tom Lane" wrote: >> I thought it would be a good idea to get this done before walking away >> from the commitfest and letting all this info get swapped out of my >> head. So

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-10-03 Thread Bossart, Nathan
On 10/3/17, 5:59 PM, "Tom Lane" wrote: > I thought it would be a good idea to get this done before walking away > from the commitfest and letting all this info get swapped out of my > head. So I've reviewed and pushed this. Thanks! > I took out most of the infrastructure

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-10-03 Thread Tom Lane
"Bossart, Nathan" writes: > Since get_rel_oids() was altered in 19de0ab2, here is a new version of > the patch. I thought it would be a good idea to get this done before walking away from the commitfest and letting all this info get swapped out of my head. So I've reviewed

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-10-02 Thread Michael Paquier
On Mon, Oct 2, 2017 at 1:43 PM, Bossart, Nathan wrote: > On 9/29/17, 9:33 AM, "Bossart, Nathan" wrote: >> Here's a version without the logging changes in vacuum_rel() and >> analyze_rel(). I’ll look into submitting those in the next commitfest. > >

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-28 Thread Bossart, Nathan
On 9/28/17, 8:46 PM, "Michael Paquier" wrote: > On Fri, Sep 29, 2017 at 10:44 AM, Tom Lane wrote: >> Michael Paquier writes: >>> On Fri, Sep 29, 2017 at 2:44 AM, Bossart, Nathan >>> wrote:

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-28 Thread Michael Paquier
On Fri, Sep 29, 2017 at 10:44 AM, Tom Lane wrote: > Michael Paquier writes: >> On Fri, Sep 29, 2017 at 2:44 AM, Bossart, Nathan wrote: >>> Alright, I've added logging for autovacuum in v23. I ended up needing to >>> do a

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-28 Thread Tom Lane
Michael Paquier writes: > On Fri, Sep 29, 2017 at 2:44 AM, Bossart, Nathan wrote: >> Alright, I've added logging for autovacuum in v23. I ended up needing to >> do a little restructuring to handle the case when the relation was skipped >> because

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-28 Thread Michael Paquier
On Fri, Sep 29, 2017 at 2:44 AM, Bossart, Nathan wrote: > Alright, I've added logging for autovacuum in v23. I ended up needing to > do a little restructuring to handle the case when the relation was skipped > because the lock could not be obtained. While doing so, I became

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-27 Thread Michael Paquier
On Thu, Sep 28, 2017 at 1:20 AM, Bossart, Nathan wrote: > On 9/26/17, 1:38 PM, "Bossart, Nathan" wrote: >> On 9/25/17, 12:42 AM, "Michael Paquier" wrote: >>> + if (!IsAutoVacuumWorkerProcess()) >>> +

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-26 Thread Bossart, Nathan
On 9/25/17, 12:42 AM, "Michael Paquier" wrote: > + if (!IsAutoVacuumWorkerProcess()) > + ereport(WARNING, > + (errmsg("skipping \"%s\" --- relation no longer exists", > + relation->relname))); > I like the use of

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-24 Thread Michael Paquier
On Sat, Sep 23, 2017 at 12:56 AM, Bossart, Nathan wrote: > On 9/21/17, 9:55 PM, "Michael Paquier" wrote: >> I still think that ExecVacuum() should pass a list of VacuumRelation >> objects to vacuum(), and get_rel_oids() should take in input a list,

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-21 Thread Michael Paquier
On Fri, Sep 22, 2017 at 7:26 AM, Tom Lane wrote: > "Bossart, Nathan" writes: >> [ 0001-error_on_duplicate_columns_in_analyze_v6.patch ] > > I've pushed (and back-patched) the 0001 patch, with some significant > changes: Thanks. Arrived here too late to

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-21 Thread Tom Lane
"Bossart, Nathan" writes: > [ 0001-error_on_duplicate_columns_in_analyze_v6.patch ] I've pushed (and back-patched) the 0001 patch, with some significant changes: * The list_concat_unique implementation is O(N^2), and seems a bit obscure as well. Perhaps there will never be

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-20 Thread Michael Paquier
On Thu, Sep 21, 2017 at 9:51 AM, Tom Lane wrote: > Michael Paquier writes: >> On Thu, Sep 21, 2017 at 9:08 AM, Tom Lane wrote: >>> Um ... so? With Nathan's proposed behavior, there are two cases depending >>> on just when the

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-20 Thread Tom Lane
Michael Paquier writes: > On Thu, Sep 21, 2017 at 9:08 AM, Tom Lane wrote: >> Um ... so? With Nathan's proposed behavior, there are two cases depending >> on just when the unexpected schema change happens: >> 1. *None* of the work gets done. >> 2.

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-20 Thread Michael Paquier
On Thu, Sep 21, 2017 at 9:08 AM, Tom Lane wrote: > Michael Paquier writes: >> On Thu, Sep 21, 2017 at 8:18 AM, Tom Lane wrote: >>> "Bossart, Nathan" writes: I agree that the patch might be simpler

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-20 Thread Tom Lane
Michael Paquier writes: > On Thu, Sep 21, 2017 at 8:18 AM, Tom Lane wrote: >> "Bossart, Nathan" writes: >>> I agree that the patch might be simpler without this, but the user-visible >>> behavior is the reason I had included

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-20 Thread Michael Paquier
On Thu, Sep 21, 2017 at 8:18 AM, Tom Lane wrote: > "Bossart, Nathan" writes: >> On 9/20/17, 2:29 PM, "Tom Lane" wrote: >>> The idea of "fast failing" because some later VacuumRelation struct might >>> contain an error seems like

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-20 Thread Tom Lane
"Bossart, Nathan" writes: > On 9/20/17, 2:29 PM, "Tom Lane" wrote: >> I think that the way this ought to work is you process the VacuumRelation >> structs purely sequentially, each in its own transaction, so you don't >> need leaps of faith about what to

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-20 Thread Bossart, Nathan
On 9/20/17, 2:29 PM, "Tom Lane" wrote: > I started to look at this... Thanks for taking a look. > I think that the way this ought to work is you process the VacuumRelation > structs purely sequentially, each in its own transaction, so you don't > need leaps of faith about

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-20 Thread Tom Lane
"Bossart, Nathan" writes: > [ 0001-vacuum_multiple_tables_v18.patch ] I started to look at this, but soon became pretty desperately unhappy with the way that it makes a bunch of tests on the relations and then releases all locks on them. That either makes the tests

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-13 Thread Kyotaro HORIGUCHI
Hello, At Wed, 13 Sep 2017 17:28:20 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI wrote in <20170913.172820.141647434.horiguchi.kyot...@lab.ntt.co.jp> > The context exists there before the patch but anyway using the > context as per-portal context that doesn't

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-13 Thread Kyotaro HORIGUCHI
At Wed, 13 Sep 2017 13:16:52 +0900, Michael Paquier wrote in

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-13 Thread Michael Paquier
On Wed, Sep 13, 2017 at 1:12 PM, Bossart, Nathan wrote: > On 9/12/17, 9:47 PM, "Michael Paquier" wrote: >> Those are minor points. The patch seems to be in good shape, and >> passes all my tests, including some pgbench'ing to make sure that >>

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-12 Thread Michael Paquier
On Wed, Sep 13, 2017 at 1:13 PM, Kyotaro HORIGUCHI wrote: > This patch creates a new memory context "Vacuum" under > PortalContext in vacuum.c, but AFAICS the current context there > is PortalHeapMemory, which has the same expected lifetime with > the new context

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-12 Thread Kyotaro HORIGUCHI
Hello, I began to look on this. (But it seems almost ready for committer..) At Wed, 13 Sep 2017 11:47:11 +0900, Michael Paquier wrote in > On Wed, Sep 13, 2017 at 12:31 AM, Bossart, Nathan

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-12 Thread Michael Paquier
On Wed, Sep 13, 2017 at 12:31 AM, Bossart, Nathan wrote: > Sorry for the spam. I am re-sending these patches with modified names so that > the apply order is obvious to the new automated testing framework (and to > everybody else). - * relid, if not InvalidOid, indicate the

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-11 Thread Michael Paquier
On Mon, Sep 11, 2017 at 2:05 PM, Bossart, Nathan wrote: >> + if (i == InvalidAttrNumber) >> + ereport(ERROR, >> + (errcode(ERRCODE_UNDEFINED_COLUMN), >> +errmsg("column \"%s\" of relation \"%s\" does not >>

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-10 Thread Michael Paquier
On Mon, Sep 11, 2017 at 9:38 AM, Bossart, Nathan wrote: > I agree that it is nice to see when relations are skipped, but I do not > know if the WARNING messages would provide much value for this > particular use case (i.e. 'VACUUM;'). If a user does not provide a list > of

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-10 Thread Bossart, Nathan
On 9/9/17, 7:28 AM, "Michael Paquier" wrote: > In the duplicate patch, it seems to me that you can save one lookup at > the list of VacuumRelation items by checking for column duplicates > after checking that all the columns are defined. If you put the > duplicate check

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-09 Thread Michael Paquier
On Sat, Sep 9, 2017 at 2:05 AM, Bossart, Nathan wrote: > On 9/8/17, 1:27 AM, "Michael Paquier" wrote: >> Thanks. This looks now correct to me. Except that: >> + ereport(ERROR, >> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), >>

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-08 Thread Michael Paquier
On Fri, Sep 8, 2017 at 7:27 AM, Bossart, Nathan wrote: > On 9/7/17, 2:33 AM, "Michael Paquier" wrote: >> Using the patch checking for duplicate columns: >> =# create table aa (a int); >> CREATE TABLE >> =# vacuum ANALYZE aa(z, z); >> ERROR: 0A000:

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-07 Thread Michael Paquier
On Thu, Sep 7, 2017 at 9:46 AM, Bossart, Nathan wrote: > I've attached v1 of this patch. I think we might want to refactor the > code for retrieving the relation name from a RangeVar, but it would > probably be better to do that in a separate patch. Using the patch checking

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-06 Thread Alvaro Herrera
Tom Lane wrote: > "Bossart, Nathan" writes: > > On 9/4/17, 10:32 PM, "Simon Riggs" wrote: > >> If we want to keep the code simple we must surely consider whether the > >> patch has any utility. > > > ... I'd argue that this feels like a natural

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-05 Thread Michael Paquier
On Wed, Sep 6, 2017 at 2:36 AM, Bossart, Nathan wrote: > On 9/4/17, 8:16 PM, "Michael Paquier" wrote: >> So I would tend to think that the same column specified multiple times >> should cause an error, and that we could let VACUUM run work N times

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-05 Thread Bossart, Nathan
On 9/4/17, 8:16 PM, "Michael Paquier" wrote: > So vacuum_multiple_tables_v14.patch is good for a committer in my > opinion. With this patch, if the same relation is specified multiple > times, then it gets vacuum'ed that many times. Using the same column > multi-times

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-05 Thread Tom Lane
"Bossart, Nathan" writes: > On 9/4/17, 10:32 PM, "Simon Riggs" wrote: >> If we want to keep the code simple we must surely consider whether the >> patch has any utility. > ... I'd argue that this feels like a natural extension of the > VACUUM command,

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-05 Thread Bossart, Nathan
On 9/4/17, 10:32 PM, "Simon Riggs" wrote: > ISTM there is no difference between > VACUUM a, b > and > VACUUM a; VACUUM b; > > If we want to keep the code simple we must surely consider whether the > patch has any utility. Yes, this is true, but I think the convenience

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-04 Thread Masahiko Sawada
On Tue, Sep 5, 2017 at 12:24 PM, Michael Paquier wrote: > On Tue, Sep 5, 2017 at 12:05 PM, Masahiko Sawada > wrote: >> In get_rel_oids() we often switch the memory context to vac_context >> and switch back. As a result almost code in

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-04 Thread Simon Riggs
On 5 September 2017 at 02:16, Michael Paquier wrote: > On Mon, Sep 4, 2017 at 11:47 PM, Bossart, Nathan wrote: >> I've made this change in v14 of the main patch. >> >> In case others had opinions regarding the de-duplication patch, I've >> attached

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-04 Thread Michael Paquier
On Tue, Sep 5, 2017 at 12:05 PM, Masahiko Sawada wrote: > In get_rel_oids() we often switch the memory context to vac_context > and switch back. As a result almost code in get_rel_oids() is working > in vac_context. Maybe we can switch memory context before and after > the

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-04 Thread Masahiko Sawada
On Tue, Sep 5, 2017 at 10:16 AM, Michael Paquier wrote: > On Mon, Sep 4, 2017 at 11:47 PM, Bossart, Nathan wrote: >> I've made this change in v14 of the main patch. >> >> In case others had opinions regarding the de-duplication patch, I've >>

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-04 Thread Michael Paquier
On Mon, Sep 4, 2017 at 11:47 PM, Bossart, Nathan wrote: > I've made this change in v14 of the main patch. > > In case others had opinions regarding the de-duplication patch, I've > attached that again as well. + /* +* Create the relation list in a long-lived memory

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-04 Thread Bossart, Nathan
On 9/3/17, 11:46 PM, "Michael Paquier" wrote: > I did not consider first that the list portion also needed to be > modified, perhaps because I am not coding that myself... So now that > it is becoming more complicated what about just using AutovacMemCxt? > This would

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-03 Thread Michael Paquier
On Sat, Sep 2, 2017 at 3:00 AM, Bossart, Nathan wrote: > Don't we have a similar problem with makeVacuumRelation() and list_make1()? Yeah, indeed. I forgot about this portion. > I went ahead and moved the RangeVar, VacuumRelation, and List into local > variables for now,

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-08-31 Thread Michael Paquier
On Fri, Sep 1, 2017 at 12:25 AM, Bossart, Nathan wrote: > On 8/31/17, 2:24 AM, "Masahiko Sawada" wrote: >> I reviewed these patches and found a issue. > > Thanks for reviewing. > >> autovacuum worker seems not to work fine. I got an error message; >>

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-08-31 Thread Masahiko Sawada
On Thu, Aug 31, 2017 at 10:52 AM, Bossart, Nathan wrote: > On 8/30/17, 5:37 PM, "Michael Paquier" wrote: >> +VacuumRelation * >> +makeVacuumRelation(RangeVar *relation, List *va_cols, Oid oid) >> +{ >> + VacuumRelation *vacrel =

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-08-30 Thread Michael Paquier
On Thu, Aug 31, 2017 at 8:35 AM, David G. Johnston wrote: > Inspired by the syntax documentation for EXPLAIN: > > VACUUM [ ( option [, ...] ) ] [ table_def [, ...] ] > > where option can be one of: > FULL > FREEZE > VERBOSE > DISABLE_PAGE_SKIPPING > >

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-08-30 Thread David G. Johnston
On Wed, Aug 30, 2017 at 4:08 PM, Bossart, Nathan wrote: > On 8/30/17, 5:37 PM, "Michael Paquier" wrote: > > Yeah... Each approach has its cost and its advantages. It may be > > better to wait for more opinions, no many people have complained yet >

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-08-30 Thread Bossart, Nathan
On 8/30/17, 5:37 PM, "Michael Paquier" wrote: > Yeah... Each approach has its cost and its advantages. It may be > better to wait for more opinions, no many people have complained yet > that for example a list of columns using twice the same one fails. Sounds good to

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-08-30 Thread Michael Paquier
On Wed, Aug 30, 2017 at 1:47 AM, Bossart, Nathan wrote: > On 8/28/17, 11:26 PM, "Michael Paquier" wrote: >> About the de-duplication patch, I have to admit that I am still not a >> fan of doing such a thing. Another road that we could take is to >>

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-08-28 Thread Michael Paquier
On Sat, Aug 26, 2017 at 8:00 AM, Robert Haas wrote: > What happens if you say VACUUM partitioned_table (a), some_partition (b)? Using v9, if you do that: =# CREATE TABLE parent (id int) PARTITION BY RANGE (id); CREATE TABLE =# CREATE TABLE child_1_10001 partition of parent

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-08-28 Thread Michael Paquier
On Sat, Aug 26, 2017 at 8:00 AM, Robert Haas wrote: > On Thu, Aug 24, 2017 at 12:59 AM, Michael Paquier > wrote: >> Robert, Amit and other folks working on extending the existing >> partitioning facility would be in better position to answer

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-08-25 Thread Robert Haas
On Thu, Aug 24, 2017 at 12:59 AM, Michael Paquier wrote: > Robert, Amit and other folks working on extending the existing > partitioning facility would be in better position to answer that, but > I would think that we should have something as flexible as possible, > and

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-08-24 Thread Michael Paquier
On Thu, Aug 24, 2017 at 11:28 PM, Bossart, Nathan wrote: > I should also note that the dedupe_relations(...) function needs another > small fix for column lists. Since the lack of a column list means that we > should ANALYZE all columns, a duplicate table name with an empty

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-08-24 Thread Bossart, Nathan
On 8/23/17, 11:59 PM, "Michael Paquier" wrote: > Robert, Amit and other folks working on extending the existing > partitioning facility would be in better position to answer that, but > I would think that we should have something as flexible as possible, > and storing a

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-08-23 Thread Michael Paquier
On Thu, Aug 24, 2017 at 12:38 PM, Bossart, Nathan wrote: > On 8/18/17, 12:56 AM, "Michael Paquier" wrote: > According to the docs, VACUUM and ANALYZE "do not support recursing over > inheritance hierarchies" [1]. However, we need a list of OIDs

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-08-18 Thread Robert Haas
On Fri, Aug 18, 2017 at 1:56 AM, Michael Paquier wrote: > + /* > +* We have already checked the column list in vacuum(...), > +* but the columns may have disappeared since then. If > +* this happens, emit a nice warning

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-08-17 Thread Michael Paquier
On Tue, Aug 15, 2017 at 4:05 PM, Michael Paquier wrote: > On Tue, Aug 15, 2017 at 8:28 AM, Bossart, Nathan wrote: >> I’ve rebased this patch with master to create v7, which is attached. > > Thanks for the rebased patch. I am switching into review

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-08-15 Thread Michael Paquier
On Tue, Aug 15, 2017 at 8:28 AM, Bossart, Nathan wrote: > I’ve rebased this patch with master to create v7, which is attached. Thanks for the rebased patch. I am switching into review mode actively now, so I'll look at it soon. -- Michael -- Sent via pgsql-hackers

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-18 Thread Michael Paquier
On Fri, May 19, 2017 at 12:17 PM, Bossart, Nathan wrote: > Just in case it was missed among the discussion, I’d like to point out that > v5 of the patch includes the “ERROR if ANALYZE not specified” change. As long as I don't forget... +VACUUM vactst (i); Looking at the

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-18 Thread Bossart, Nathan
On 5/18/17, 8:03 PM, "Tom Lane" wrote: >”Bossart, Nathan" writes: >> On 5/18/17, 6:12 PM, "Michael Paquier" wrote: >>> Fine for me as well. I would suggest to split the patch into two parts >>> to ease review then: >>> - Rework

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-18 Thread Tom Lane
"Bossart, Nathan" writes: > On 5/18/17, 6:12 PM, "Michael Paquier" wrote: >> Fine for me as well. I would suggest to split the patch into two parts >> to ease review then: >> - Rework this error handling for one relation. >> - The main patch. >

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-18 Thread Bossart, Nathan
On 5/18/17, 6:12 PM, "Michael Paquier" wrote: > Fine for me as well. I would suggest to split the patch into two parts > to ease review then: > - Rework this error handling for one relation. > - The main patch. I’d be happy to do so, but I think part one would be

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-18 Thread Michael Paquier
On Fri, May 19, 2017 at 10:00 AM, Tom Lane wrote: > Michael Paquier writes: >> On Fri, May 19, 2017 at 9:06 AM, Michael Paquier >> wrote: >>> I am fine with an ERROR if a column list is specified without ANALYZE >>>

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-18 Thread Tom Lane
Michael Paquier writes: > On Fri, May 19, 2017 at 9:06 AM, Michael Paquier > wrote: >> I am fine with an ERROR if a column list is specified without ANALYZE >> listed in the options. But that should happen as well for the case >> where only

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-18 Thread Michael Paquier
On Fri, May 19, 2017 at 9:06 AM, Michael Paquier wrote: > I am fine with an ERROR if a column list is specified without ANALYZE > listed in the options. But that should happen as well for the case > where only one relation is listed. Perhaps this could be changed for

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-18 Thread Michael Paquier
On Fri, May 19, 2017 at 12:03 AM, Tom Lane wrote: > Robert Haas writes: >> Ugh, really? Are we sure that the current behavior is anything other >> than a bug? Point was raised already upthread by me ince, which is what lead me to the reasoning of my

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-18 Thread Masahiko Sawada
On Fri, May 19, 2017 at 12:03 AM, Tom Lane wrote: > Robert Haas writes: >> Ugh, really? Are we sure that the current behavior is anything other >> than a bug? The idea that VACUUM foo (a) implies ANALYZE doesn't >> really sit very well with me in the

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-18 Thread Tom Lane
Robert Haas writes: > Ugh, really? Are we sure that the current behavior is anything other > than a bug? The idea that VACUUM foo (a) implies ANALYZE doesn't > really sit very well with me in the first place. I'd be more inclined > to reject that with an ERROR

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-18 Thread Robert Haas
On Thu, May 18, 2017 at 2:45 AM, Masahiko Sawada wrote: > On Thu, May 18, 2017 at 3:19 PM, Michael Paquier > wrote: >> On Thu, May 18, 2017 at 2:59 PM, Masahiko Sawada >> wrote: >>> It seems to me that it's not good idea

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-18 Thread Masahiko Sawada
On Thu, May 18, 2017 at 3:19 PM, Michael Paquier wrote: > On Thu, May 18, 2017 at 2:59 PM, Masahiko Sawada > wrote: >> It seems to me that it's not good idea to forcibly set ANALYZE in >> spite of ANALYZE option is not specified. One reason is

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-18 Thread Michael Paquier
On Thu, May 18, 2017 at 2:59 PM, Masahiko Sawada wrote: > It seems to me that it's not good idea to forcibly set ANALYZE in > spite of ANALYZE option is not specified. One reason is that it would > make us difficult to grep it from such as server log. I think It's > better

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-18 Thread Masahiko Sawada
On Thu, May 18, 2017 at 11:01 AM, Michael Paquier wrote: > On Thu, May 18, 2017 at 12:06 AM, Bossart, Nathan wrote: >> I agree with you here, too. I stopped short of allowing customers to >> explicitly provide per-table options, so the example

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-17 Thread Masahiko Sawada
On Thu, May 18, 2017 at 7:38 AM, Bossart, Nathan wrote: > I’ve attached a v4 of the patch. > > Changes: > - Early in vacuum(…), emit an ERROR if any specified columns do not exist. > - Emit a WARNING and skip any specified tables or columns that disappear > before they are

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-17 Thread Michael Paquier
On Thu, May 18, 2017 at 12:06 AM, Bossart, Nathan wrote: > I agree with you here, too. I stopped short of allowing customers to > explicitly provide per-table options, so the example you provided wouldn’t > work here. This is more applicable for something like the

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-17 Thread Bossart, Nathan
On 5/16/17, 11:21 PM, "Michael Paquier" wrote: > On Wed, May 17, 2017 at 7:56 AM, Bossart, Nathan wrote: >> I think this issue already exists, as this comment in get_rel_oids(…) seems >> to indicate: >> >> /* >> * Since we don't take a

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-17 Thread Michael Paquier
On Wed, May 17, 2017 at 7:56 AM, Bossart, Nathan wrote: > I think this issue already exists, as this comment in get_rel_oids(…) seems > to indicate: > > /* > * Since we don't take a lock here, the relation might be gone, or the > * RangeVar might no longer

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-16 Thread Michael Paquier
On Fri, May 12, 2017 at 1:06 PM, Bossart, Nathan wrote: > On 5/11/17, 7:20 PM, "Michael Paquier" wrote: >> It seems to me that it would have been less invasive to loop through >> vacuum() for each relation. Do you foresee advantages in allowing >>

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-11 Thread Bossart, Nathan
On 5/11/17, 7:20 PM, "Michael Paquier" wrote: > Browsing the code Thanks for taking a look. > It seems to me that you don't need those extra square brackets around > the column list. Same remark for vacuum.sgml. I’ll remove them. My intent was to ensure that we

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-11 Thread Michael Paquier
On Fri, May 12, 2017 at 9:47 AM, Bossart, Nathan wrote: > Attached is a more complete first attempt at adding this functionality. I > added two node types: one for parsing the “relation and columns” list in the > grammar, and one for holding the relation information we

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-11 Thread Bossart, Nathan
On 5/11/17, 6:32 PM, "Tom Lane" wrote: > You probably won't get much in the near term, because we're in > stabilize-the-release mode not develop-new-features mode. > Please add your patch to the pending commitfest > https://commitfest.postgresql.org/14/ > so that we remember

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-11 Thread Tom Lane
"Bossart, Nathan" writes: > Looking forward to any feedback that you have. You probably won't get much in the near term, because we're in stabilize-the-release mode not develop-new-features mode. Please add your patch to the pending commitfest

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-10 Thread Michael Paquier
On Thu, May 11, 2017 at 11:56 AM, Tom Lane wrote: > It would be hard to reject at the grammar level, and not very friendly > either because you'd only get "syntax error". We could certainly make > the runtime code throw an error if you gave a column list without saying >

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-10 Thread Bossart, Nathan
On 5/10/17, 8:10 PM, "Masahiko Sawada" wrote: > I agree to report per-table information. Especially In case of one of > tables specified failed during vacuuming, I think we should report at > least information of tables that is done successfully so far. +1 I believe you

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-10 Thread Masahiko Sawada
On Thu, May 11, 2017 at 11:13 AM, Michael Paquier wrote: > On Thu, May 11, 2017 at 6:40 AM, Tom Lane wrote: >> "Bossart, Nathan" writes: >>> Currently, VACUUM commands allow you to specify one table or all of the >>> tables in

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-10 Thread Tom Lane
Michael Paquier writes: > On Thu, May 11, 2017 at 6:40 AM, Tom Lane wrote: >> The column list only matters for ANALYZE (or VACUUM ANALYZE). But yes, >> it should be per-table. > The grammar allows that by the way: > =# VACUUM (full) aa (a); >

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-10 Thread Michael Paquier
On Thu, May 11, 2017 at 6:40 AM, Tom Lane wrote: > "Bossart, Nathan" writes: >> Currently, VACUUM commands allow you to specify one table or all of the >> tables in the current database to vacuum. I’ve recently found myself >> wishing I could specify

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-10 Thread Bossart, Nathan
Great, I’ll keep working on this patch and will update this thread with a more complete implementation. Nathan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-10 Thread Tom Lane
"Bossart, Nathan" writes: > Currently, VACUUM commands allow you to specify one table or all of the > tables in the current database to vacuum. I’ve recently found myself wishing > I could specify multiple tables in a single VACUUM statement. For example, > this would be

Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-05-10 Thread David Fetter
On Wed, May 10, 2017 at 08:10:48PM +, Bossart, Nathan wrote: > Hi Hackers, > > Currently, VACUUM commands allow you to specify one table or all of > the tables in the current database to vacuum. I’ve recently found > myself wishing I could specify multiple tables in a single VACUUM >