From: | Michael Paquier <michael(at)paquier(dot)xyz> |
---|---|
To: | Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com> |
Cc: | Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Add --tablespace option to reindexdb |
Date: | 2021-03-02 06:04:58 |
Message-ID: | YD3Vir8rG5ZQ1U9M@paquier.xyz |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Mon, Mar 01, 2021 at 10:12:54AM -0800, Mark Dilger wrote:
> Your check verifies that reindexing a system table on a new
> tablespace fails, but does not verify what the failure was. I
> wonder if you might want to make it more robust, something like:
I was not sure if that was worth adding or not, but no objections to
do so. So updated the patch to do that.
On Mon, Mar 01, 2021 at 09:47:57AM -0800, Mark Dilger wrote:
> I recognize that you are borrowing some of that from
> src/bin/pgbench/t/001_pgbench_with_server.pl, but I don't find
> anything intuitive about the name "ets" and would rather not see
> that repeated. There is nothing in TestLib::perl2host to explain
> this name choice, nor in pgbench's test, nor yours.
Okay, I have made the variable names more explicit.
> but I don't see what tests of the --concurrently option have to do
> with this patch. I'm not saying there is anything wrong with this
> change, but it seems out of place. Am I missing something?
While hacking on this feature I have just bumped into this separate
issue, where the same test just gets repeated twice. I have just
applied an independent patch to take care of this problem separately,
and backpatched it down to 12 where this got introduced.
On Mon, Mar 01, 2021 at 09:26:03AM -0800, Mark Dilger wrote:
> I think the language "to reindex on" could lead users to think that
> indexes already existent in the given tablespace will be reindexed.
> In other words, the option might be misconstrued as a way to specify
> all the indexes you want reindexed. Whatever you do here, beware
> that you are using similar language in the --help, so consider
> changing that, too.
OK. I have switched the docs to "Specifies the tablespace where
indexes are rebuilt" and --help to "tablespace where indexes are
rebuilt".
I have also removed the assertions based on the version number of the
backend, based on Daniel's input sent upthread.
What do you think?
--
Michael
Attachment | Content-Type | Size |
---|---|---|
v2-0001-Add-tablespace-option-to-reindexdb.patch | text/x-diff | 15.4 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Masahiko Sawada | 2021-03-02 06:33:14 | Removing vacuum_cleanup_index_scale_factor |
Previous Message | Joel Jacobson | 2021-03-02 05:52:07 | Re: [PATCH] regexp_positions ( string text, pattern text, flags text ) → setof int4range[] |