Add --tablespace option to reindexdb

On 26 Feb 2021, at 07:49, Michael Paquier <michael@paquier.xyz> wrote:

Since c5b2860, it is possible to specify a tablespace for a REINDEX,
but the equivalent option has not been added to reindexdb. Attached
is a patch to take care of that.

This includes documentation and tests.

Makes sense.

While on it, I have added tests for toast tables and indexes with a
tablespace move during a REINDEX. Those operations fail, but it is
not possible to get that into the main regression test suite because
the error messages include the relation names so that's unstable.
Well, it would be possible to do that for the non-concurrent case
using a TRY/CATCH block in a custom function but that would not work
with CONCURRENTLY. Anyway, I would rather group the whole set of
tests together, and using the --tablespace option introduced here
within a TAP test does the job.

Agreed, doing it with a TAP test removes the complication.

Some other small comments:

+ Assert(PQserverVersion(conn) >= 140000);
Are these assertions really buying us much when we already check the server
version in reindex_one_database()?

+ printf(_(" --tablespace=TABLESPACE reindex on specified tablespace\n"));
While not introduced by this patch, I realized that we have a mix of
conventions for how to indent long options which don't have a short option.
Under "Connection options" all options are left-justified but under "Options"
all long-options are aligned with space indentation for missing shorts. Some
tools do it like this, where others like createuser left justifies under
Options as well. Maybe not the most pressing issue, but consistency is always
good in user interfaces so maybe it's worth addressing (in a separate patch)?

--
Daniel Gustafsson https://vmware.com/

On Fri, Feb 26, 2021 at 11:00:13AM +0100, Daniel Gustafsson wrote:

Some other small comments:

+ Assert(PQserverVersion(conn) >= 140000);
Are these assertions really buying us much when we already check the server
version in reindex_one_database()?

I found these helpful when working on vacuumdb and refactoring the
code, though I'd agree this code may not justify going down to that.

+ printf(_(" --tablespace=TABLESPACE reindex on specified tablespace\n"));
While not introduced by this patch, I realized that we have a mix of
conventions for how to indent long options which don't have a short option.
Under "Connection options" all options are left-justified but under "Options"
all long-options are aligned with space indentation for missing shorts. Some
tools do it like this, where others like createuser left justifies under
Options as well. Maybe not the most pressing issue, but consistency is always
good in user interfaces so maybe it's worth addressing (in a separate patch)?

Yeah, consistency is good, though I am not sure which one would be
consistent here actually as there is no defined rule. For this one,
I think that I would keep what I have to be consistent with the
existing inconsistency (?). In short, I would just add --tablespace
the same way there is --concurrently, without touching the connection
option part.
--
Michael

On Feb 25, 2021, at 10:49 PM, Michael Paquier <michael@paquier.xyz> wrote:

Hi all,

Hi Michael,

Since c5b2860, it is possible to specify a tablespace for a REINDEX,
but the equivalent option has not been added to reindexdb. Attached
is a patch to take care of that.

This includes documentation and tests.

The documentation of the TABLESPACE option for REINDEX says:

+ Specifies that indexes will be rebuilt on a new tablespace.

but in your patch for reindexdb, it is less clear:

+        Specifies the tablespace to reindex on. (This name is processed as
+        a double-quoted identifier.)

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.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Feb 25, 2021, at 10:49 PM, Michael Paquier <michael@paquier.xyz> wrote:

While on it, I have added tests for toast tables and indexes with a
tablespace move during a REINDEX.

In t/090_reindexdb.pl, you add:

+# Create a tablespace for testing.
+my $ts = $node->basedir . '/regress_reindex_tbspace';
+mkdir $ts or die "cannot create directory $ts";
+# this takes care of WIN-specific path issues
+my $ets     = TestLib::perl2host($ts);
+my $tbspace = 'reindex_tbspace';
+$node->safe_psql('postgres', "CREATE TABLESPACE $tbspace LOCATION '$ets';");

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.

You also change a test:

 $node->issues_sql_like(
-       [ 'reindexdb', '-v', '-t', 'test1', 'postgres' ],
-       qr/statement: REINDEX \(VERBOSE\) TABLE public\.test1;/,
-       'reindex with verbose output');
+       [ 'reindexdb', '--concurrently', '-v', '-t', 'test1', 'postgres' ],
+       qr/statement: REINDEX \(VERBOSE\) TABLE CONCURRENTLY public\.test1;/,
+       'reindex concurrently with verbose output');

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?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Feb 25, 2021, at 10:49 PM, Michael Paquier <michael@paquier.xyz> wrote:

Anyway, I would rather group the whole set of
tests together, and using the --tablespace option introduced here
within a TAP test does the job.

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:

diff --git a/src/bin/scripts/t/090_reindexdb.pl b/src/bin/scripts/t/090_reindexdb.pl
index 6946268209..8453acc817 100644
--- a/src/bin/scripts/t/090_reindexdb.pl
+++ b/src/bin/scripts/t/090_reindexdb.pl
@@ -3,7 +3,7 @@ use warnings;

 use PostgresNode;
 use TestLib;
-use Test::More tests => 54;
+use Test::More tests => 58;

 program_help_ok('reindexdb');
 program_version_ok('reindexdb');
@@ -108,23 +108,35 @@ $node->issues_sql_like(
 # names, and CONCURRENTLY cannot be used in transaction blocks, preventing
 # the use of TRY/CATCH blocks in a custom function to filter error
 # messages.
-$node->command_fails(
+$node->command_checks_all(
        [ 'reindexdb', '-t', $toast_table, '--tablespace', $tbspace, 'postgres' ],
+       1,
+       [ ],
+       [ qr/cannot move system relation/ ],
        'reindex toast table with tablespace');
-$node->command_fails(
+$node->command_checks_all(
        [
                'reindexdb',    '--concurrently', '-t', $toast_table,
                '--tablespace', $tbspace,         'postgres'
        ],
+       1,
+       [ ],
+       [ qr/cannot move system relation/ ],
        'reindex toast table concurrently with tablespace');
-$node->command_fails(
+$node->command_checks_all(
        [ 'reindexdb', '-i', $toast_index, '--tablespace', $tbspace, 'postgres' ],
+       1,
+       [ ],
+       [ qr/cannot move system relation/ ],
        'reindex toast index with tablespace');
-$node->command_fails(
+$node->command_checks_all(
        [
                'reindexdb',    '--concurrently', '-i', $toast_index,
                '--tablespace', $tbspace,         'postgres'
        ],
+       1,
+       [ ],
+       [ qr/cannot move system relation/ ],
        'reindex toast index concurrently with tablespace');

# connection strings

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On 2 Mar 2021, at 07:04, Michael Paquier <michael@paquier.xyz> wrote:

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?

+1, no objections from me after a readthrough of this version.

--
Daniel Gustafsson https://vmware.com/

On Mar 1, 2021, at 10:04 PM, Michael Paquier <michael@paquier.xyz> wrote:

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?

Looks good.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Mar 02, 2021 at 10:01:43AM +0100, Daniel Gustafsson wrote:

+1, no objections from me after a readthrough of this version.

Thanks, Daniel and Mark. I have applied v2 from upthread, then.
--
Michael