From: | Julien Rouhaud <rjuju123(at)gmail(dot)com> |
---|---|
To: | Michael Paquier <michael(at)paquier(dot)xyz> |
Cc: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Daniel Verite <daniel(at)manitou-mail(dot)org>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Kevin Grittner <kgrittn(at)gmail(dot)com> |
Subject: | Re: Add parallelism and glibc dependent only options to reindexdb |
Date: | 2019-07-25 06:50:40 |
Message-ID: | CAOBaU_bPmzUAqC7noYcO+j6H9xDcoCjrMYUiNFcFwtTRUnN=xQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Sorry for the late answer,
On Tue, Jul 23, 2019 at 9:38 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Mon, Jul 22, 2019 at 02:40:19PM +0200, Julien Rouhaud wrote:
> > Right, so I kept the long option. Also this comment was outdated, as
> > the --jobs is now just ignored with a list of indexes, so I fixed that
> > too.
>
> + if (!parallel)
> + {
> + if (user_list == NULL)
> + {
> + /*
> + * Create a dummy list with an empty string, as user requires an
> + * element.
> + */
> + process_list = pg_malloc0(sizeof(SimpleStringList));
> + simple_string_list_append(process_list, "");
> + }
> + }
> This part looks a bit hacky and this is needed because we don't have a
> list of objects when doing a non-parallel system or database reindex.
> The deal is that we just want a list with one element: the database of
> the connection. Wouldn't it be more natural to assign the database
> name here using PQdb(conn)? Then add an assertion at the top of
> run_reindex_command() checking for a non-NULL name?
Good point, fixed it that way.
>
> > I considered this, but it would require to adapt all code that declare
> > SimpleStringList stack variable (vacuumdb.c, clusterdb.c,
> > createuser.c, pg_dumpall.c and pg_dump.c), so it looked like too much
> > trouble to avoid 2 local variables here and 1 in vacuumdb.c. I don't
> > have a strong opinion here, so I can go for it if you prefer.
>
> Okay. This is a tad wider than the original patch proposal, and this
> adds two lines. So let's discard that for now and keep it simple.
Ok!
> >> +$node->issues_sql_like([qw(reindexdb -j2)],
> >> + qr/statement: REINDEX TABLE public.test1/,
> >> + 'Global and parallel reindex will issue per-table REINDEX');
> >> Would it make sense to have some tests for schemas here?
> >>
> >> One of my comments in [1] has not been answered. What about
> >> the decomposition of a list of schemas into a list of tables when
> >> using the parallel mode?
> >
> > I did that in attached v6, and added suitable regression tests.
>
> The two tests for "reindexdb -j2" can be combined into a single call,
> checking for both commands to be generated in the same output. The
> second command triggering a reindex on two schemas cannot be used to
> check for the generation of both s1.t1 and s2.t2 as the ordering may
> not be guaranteed. The commands arrays also looked inconsistent with
> the rest. Attached is an updated patch with some format modifications
> and the fixes for the tests.
Attached v8 based on your v7 + the fix for the dummy part.
Attachment | Content-Type | Size |
---|---|---|
reindex_parallel_v8.diff | application/octet-stream | 18.8 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2019-07-25 07:58:17 | Re: pg_receivewal documentation |
Previous Message | Suraj Kharage | 2019-07-25 06:47:29 | Re: Issue in to_timestamp/to_date while handling the quoted literal string |