Allowing REINDEX to have an optional name

Hi,

Am Dienstag, dem 10.05.2022 um 10:13 +0100 schrieb Simon Riggs:

A minor issue, and patch.

REINDEX DATABASE currently requires you to write REINDEX DATABASE
dbname, which makes this a little less usable than we might like.

REINDEX on the catalog can cause deadlocks, which also makes REINDEX
DATABASE not much use in practice, and is the reason there is no test
for REINDEX DATABASE. Another reason why it is a little less usable
than we might like.

Seems we should do something about these historic issues in the name
of product usability.

Wow, i just recently had a look into that code and talked with my
colleagues on how the current behavior annoyed me last week....and
there you are! This community rocks ;)

Attached patch allows new syntax for REINDEX DATABASE, without
needing
to specify dbname. That version of the command skips catalog tables,
as a way of avoiding the known deadlocks. Patch also adds a test.

+		/* Unqualified REINDEX DATABASE will skip catalog
tables */
+		if (objectKind == REINDEX_OBJECT_DATABASE &&
+			objectName == NULL &&
+			IsSystemClass(relid, classtuple))
+			continue;

Hmm, shouldn't we just print a NOTICE or something like this in
addition to this check to tell the user that we are *not* really
reindexing all things (and probably give him a hint to use REINDEX
SYSTEM to cover them)?

Thanks,
Bernd

On Tue, May 10, 2022 at 2:43 PM Simon Riggs
<simon.riggs@enterprisedb.com> wrote:

A minor issue, and patch.

REINDEX DATABASE currently requires you to write REINDEX DATABASE
dbname, which makes this a little less usable than we might like.

REINDEX on the catalog can cause deadlocks, which also makes REINDEX
DATABASE not much use in practice, and is the reason there is no test
for REINDEX DATABASE. Another reason why it is a little less usable
than we might like.

Seems we should do something about these historic issues in the name
of product usability.

Attached patch allows new syntax for REINDEX DATABASE, without needing
to specify dbname. That version of the command skips catalog tables,
as a way of avoiding the known deadlocks. Patch also adds a test.

From the patch it looks like with the patch applied running REINDEX
DATABASE is equivalent to running REINDEX DATABASE <current database>
except reindexing the shared catalogs. Is that correct?

Though the patch adds following change
+      Indexes on shared system catalogs are also processed, unless the
+      database name is omitted, in which case system catalog indexes
are skipped.

the syntax looks unintuitive.

I think REINDEX DATABASE reindexing the current database is a good
usability improvement in itself. But skipping the shared catalogs
needs an explicity syntax. Not sure how feasible it is but something
like REINDEX DATABASE skip SHARED/SYSTEM.

--
Best Wishes,
Ashutosh Bapat

On Tue, 10 May 2022 at 14:47, Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Tue, May 10, 2022 at 2:43 PM Simon Riggs
<simon.riggs@enterprisedb.com> wrote:

A minor issue, and patch.

REINDEX DATABASE currently requires you to write REINDEX DATABASE
dbname, which makes this a little less usable than we might like.

REINDEX on the catalog can cause deadlocks, which also makes REINDEX
DATABASE not much use in practice, and is the reason there is no test
for REINDEX DATABASE. Another reason why it is a little less usable
than we might like.

Seems we should do something about these historic issues in the name
of product usability.

Attached patch allows new syntax for REINDEX DATABASE, without needing
to specify dbname. That version of the command skips catalog tables,
as a way of avoiding the known deadlocks. Patch also adds a test.

From the patch it looks like with the patch applied running REINDEX
DATABASE is equivalent to running REINDEX DATABASE <current database>
except reindexing the shared catalogs. Is that correct?

Yes

Though the patch adds following change
+      Indexes on shared system catalogs are also processed, unless the
+      database name is omitted, in which case system catalog indexes
are skipped.

the syntax looks unintuitive.

I think REINDEX DATABASE reindexing the current database is a good
usability improvement in itself. But skipping the shared catalogs
needs an explicity syntax. Not sure how feasible it is but something
like REINDEX DATABASE skip SHARED/SYSTEM.

There are two commands:

REINDEX DATABASE does every except system catalogs
REINDEX SYSTEM does system catalogs only

So taken together, the two commands seem intuitive to me.

It is designed like this because it is dangerous to REINDEX the system
catalogs because of potential deadlocks, so we want a way to avoid
that problem.

Perhaps I can improve the docs more, will look.

--
Simon Riggs http://www.EnterpriseDB.com/

On Tue, May 10, 2022 at 7:30 PM Simon Riggs <simon.riggs@enterprisedb.com>
wrote:

On Tue, 10 May 2022 at 14:47, Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Tue, May 10, 2022 at 2:43 PM Simon Riggs
<simon.riggs@enterprisedb.com> wrote:

A minor issue, and patch.

REINDEX DATABASE currently requires you to write REINDEX DATABASE
dbname, which makes this a little less usable than we might like.

REINDEX on the catalog can cause deadlocks, which also makes REINDEX
DATABASE not much use in practice, and is the reason there is no test
for REINDEX DATABASE. Another reason why it is a little less usable
than we might like.

Seems we should do something about these historic issues in the name
of product usability.

Attached patch allows new syntax for REINDEX DATABASE, without needing
to specify dbname. That version of the command skips catalog tables,
as a way of avoiding the known deadlocks. Patch also adds a test.

From the patch it looks like with the patch applied running REINDEX
DATABASE is equivalent to running REINDEX DATABASE <current database>
except reindexing the shared catalogs. Is that correct?

Yes

Though the patch adds following change
+      Indexes on shared system catalogs are also processed, unless the
+      database name is omitted, in which case system catalog indexes
are skipped.

the syntax looks unintuitive.

I think REINDEX DATABASE reindexing the current database is a good
usability improvement in itself. But skipping the shared catalogs
needs an explicity syntax. Not sure how feasible it is but something
like REINDEX DATABASE skip SHARED/SYSTEM.

There are two commands:

REINDEX DATABASE does every except system catalogs
REINDEX SYSTEM does system catalogs only

IIUC

REINDEX DATABASE <database name> does system catalogs as well
REINDEX DATABASE does everything except system catalogs

That's confusing and unintuitive.

Not providing the database name leads to ignoring system catalogs. I won't
expect that from this syntax.

So taken together, the two commands seem intuitive to me.

It is designed like this because it is dangerous to REINDEX the system
catalogs because of potential deadlocks, so we want a way to avoid
that problem.

It's more clear if we add SKIP SYSTEM CATALOGS or some such explicit syntax.

--
Best Wishes,
Ashutosh

On Wed, May 11, 2022 at 09:54:17AM +0530, Ashutosh Bapat wrote:

REINDEX DATABASE <database name> does system catalogs as well
REINDEX DATABASE does everything except system catalogs

That's confusing and unintuitive.

Agreed. Nobody is going to remember the difference. REINDEX's
parsing grammar is designed to be extensible because we have the
parenthesized flavor. Why don't you add an option there to skip the
catalogs, like a SKIP_CATALOG?

Not providing the database name leads to ignoring system catalogs. I won't
expect that from this syntax.

I don't disagree with having a shortened grammar where the database
name is not required because one cannot reindex a database different
than the one connected to, but changing a behavior based on such a
grammar difference is not a good user experience.
--
Michael

Am Mittwoch, dem 11.05.2022 um 14:42 +0900 schrieb Michael Paquier:

Agreed.  Nobody is going to remember the difference.  REINDEX's
parsing grammar is designed to be extensible because we have the
parenthesized flavor.  Why don't you add an option there to skip the
catalogs, like a SKIP_CATALOG?

+1

Having an option is probably the best idea. Though we have REINDEX
SYSTEM, so i throw SKIP_SYSTEM into the ring as an alternative. This
would be consistent with the meaning of both commands/options.

Thanks,
Bernd

On Wed, 11 May 2022 at 05:24, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

It is designed like this because it is dangerous to REINDEX the system
catalogs because of potential deadlocks, so we want a way to avoid
that problem.

It's more clear if we add SKIP SYSTEM CATALOGS or some such explicit syntax.

Clarity is not the issue. I am opposed to a default mode that does
something bad and non-useful.

If you want to reindex the system catalogs then we already have REINDEX SYSTEM.
So REINDEX (SKIP_SYSTEM_CATALOGS OFF) DATABASE would do the same thing.
But you don't want to run either of them because of deadlocking.

The only action that makes sense is to reindex the database, skipping
the catalog tables.

So I'm proposing a command that has useful default behavior.
i.e. REINDEX DATABASE is the same as REINDEX (SKIP_SYSTEM_CATALOGS ON) DATABASE.

If you make REINDEX DATABASE the same as REINDEX (SKIP_SYSTEM_CATALOGS
OFF) DATABASE then it is just dangerous and annoying, i.e. a POLA
violation.

The point of this was a usability improvement, not just new syntax.

--
Simon Riggs http://www.EnterpriseDB.com/

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed

Hello

The patch applies and tests fine and I think this patch has good intentions to prevent the default behavior of REINDEX DATABASE to cause a deadlock. However, I am not in favor of simply omitting the database name after DATABASE clause because of consistency. Almost all other queries involving the DATABASE clause require database name to be given following after. For example, ALTER DATABASE [dbname].

Being able to omit database name for REINDEX DATABASE seems inconsistent to me.

The documentation states that REINDEX DATABASE only works on the current database, but it still requires the user to provide a database name and require that it must match the current database. Not very useful option, isn’t it? But it is still required from the user to stay consistent with other DATABASE clauses.

Maybe the best way is to keep the query clause as is (with the database name still required) and simply don’t let it reindex system catalog to prevent deadlock. At the end, give user a notification that system catalogs have not been reindexed, and tell them to use REINDEX SYSTEM instead.

Cary Huang
-----------------
HighGo Software Canada
www.highgo.ca

Am Freitag, dem 27.05.2022 um 19:08 +0000 schrieb Cary Huang:

[...]

The patch applies and tests fine and I think this patch has good
intentions to prevent the default behavior of REINDEX DATABASE to
cause a deadlock. However, I am not in favor of simply omitting the
database name after DATABASE clause because of consistency. Almost
all other queries involving the DATABASE clause require database name
to be given following after. For example, ALTER DATABASE [dbname]. 

Being able to omit database name for REINDEX DATABASE seems
inconsistent to me.

The documentation states that REINDEX DATABASE only works on the
current database, but it still requires the user to provide a
database name and require that it must match the current database.
Not very useful option, isn’t it? But it is still required from the
user to stay consistent with other DATABASE clauses.

Hmm, right, but you can see this from another perspective, too: For
example, ALTER DATABASE works by adjusting properties of other
databases very well, SET TABLESPACE can be used when not connected to
the target database only, so you are required to specify its name in
that case.
REINDEX DATABASE cannot reindex other databases than the one we're
connected to. Seen from that point, i currently can't see the logical
justification to have the database name there, besides of "yes, i
really meant that database i am connected to" or consistency.

Maybe the best way is to keep the query clause as is (with the
database name still required) and simply don’t let it reindex system
catalog to prevent deadlock. At the end, give user a notification
that system catalogs have not been reindexed, and tell them to use
REINDEX SYSTEM instead.

+1

Am Dienstag, dem 10.05.2022 um 15:00 +0100 schrieb Simon Riggs:

[...]

I think REINDEX DATABASE reindexing the current database is a good
usability improvement in itself. But skipping the shared catalogs
needs an explicity syntax. Not sure how feasible it is but
something
like REINDEX DATABASE skip SHARED/SYSTEM.

There are two commands:

REINDEX DATABASE does every except system catalogs
REINDEX SYSTEM does system catalogs only

So taken together, the two commands seem intuitive to me.

It is designed like this because it is dangerous to REINDEX the
system
catalogs because of potential deadlocks, so we want a way to avoid
that problem.

Perhaps I can improve the docs more, will look.

And we already have a situation where this already happens with REINDEX
DATABASE: if you use CONCURRENTLY, it skips system catalogs already and
prints a warning. In both cases there are good technical reasons to
skip catalog indexes and to change the workflow to use separate
commands.

Bernd

On Tue, May 31, 2022 at 11:04:58AM +0200, Bernd Helmle wrote:

And we already have a situation where this already happens with REINDEX
DATABASE: if you use CONCURRENTLY, it skips system catalogs already and
prints a warning. In both cases there are good technical reasons to
skip catalog indexes and to change the workflow to use separate
commands.

The case with CONCURRENTLY is different though: the option will never
work on system catalogs so we have to skip them. Echoing with others
on this thread, I don't think that we should introduce a different
behavior on what's basically the same grammar. That's just going to
lead to more confusion. So REINDEX DATABASE with or without a
database name appended to it should always mean to reindex the
catalogs on top of the existing relations.
--
Michael

On 2022-May-31, Michael Paquier wrote:

The case with CONCURRENTLY is different though: the option will never
work on system catalogs so we have to skip them. Echoing with others
on this thread, I don't think that we should introduce a different
behavior on what's basically the same grammar. That's just going to
lead to more confusion. So REINDEX DATABASE with or without a
database name appended to it should always mean to reindex the
catalogs on top of the existing relations.

I was thinking the opposite: REINDEX DATABASE with or without a database
name should always process the user relations and skip system catalogs.
If the user wants to do both, then they can use REINDEX SYSTEM in
addition.

The reason for doing it like this is that there is no way to process
only user tables and skip catalogs. So this is better for
composability.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Most hackers will be perfectly comfortable conceptualizing users as entropy
sources, so let's move on." (Nathaniel Smith)

On Tue, May 31, 2022 at 02:30:32PM +0200, Alvaro Herrera wrote:

I was thinking the opposite: REINDEX DATABASE with or without a database
name should always process the user relations and skip system catalogs.
If the user wants to do both, then they can use REINDEX SYSTEM in
addition.

The reason for doing it like this is that there is no way to process
only user tables and skip catalogs. So this is better for
composability.

No objections from me to keep this distinction at the end, as long as
the the database name in the command has no impact on the chosen
behavior. Could there be a point in having a REINDEX ALL though that
would process both the user relations and the catalogs, doing the same
thing as REINDEX DATABASE today?

By the way, the patch had better avoid putting a global REINDEX
command that would process everything. As far as I recall, we've
avoided such things on purpose because they are expensive, keeping
around only cases that generate errors or skip all the relations.
So having that in a TAP test would be better, I assume, for
isolation.
--
Michael

Maybe the answer is to 1) add a parenthesized option REINDEX(SYSTEM) (to allow
the current behavior); and 2) make REINDEX DATABASE an alias which implies
"SYSTEM false"; 3) prohibit REINDEX (SYSTEM true) SYSTEM, or consider removing
"REINDEX SYSTEM;".

That avoids the opaque and surprising behavior that "REINDEX DATABASE" skips
system stuff but "REINDEX DATABASE foo" doesn't, while allowing the old
behavior (disabled by default).

On Thu, 2 Jun 2022 at 01:02, Michael Paquier <michael@paquier.xyz> wrote:

On Tue, May 31, 2022 at 02:30:32PM +0200, Alvaro Herrera wrote:

I was thinking the opposite: REINDEX DATABASE with or without a database
name should always process the user relations and skip system catalogs.
If the user wants to do both, then they can use REINDEX SYSTEM in
addition.

The reason for doing it like this is that there is no way to process
only user tables and skip catalogs. So this is better for
composability.

No objections from me to keep this distinction at the end, as long as
the the database name in the command has no impact on the chosen
behavior.

OK, that's clear. Will progress.

Could there be a point in having a REINDEX ALL though that
would process both the user relations and the catalogs, doing the same
thing as REINDEX DATABASE today?

A key point is that REINDEX SYSTEM has problems, so should be avoided.
Hence, including both database and system together in a new command
would not be great idea, at this time.

--
Simon Riggs http://www.EnterpriseDB.com/

On Tue, 28 Jun 2022 at 08:29, Simon Riggs <simon.riggs@enterprisedb.com> wrote:

On Thu, 2 Jun 2022 at 01:02, Michael Paquier <michael@paquier.xyz> wrote:

On Tue, May 31, 2022 at 02:30:32PM +0200, Alvaro Herrera wrote:

I was thinking the opposite: REINDEX DATABASE with or without a database
name should always process the user relations and skip system catalogs.
If the user wants to do both, then they can use REINDEX SYSTEM in
addition.

The reason for doing it like this is that there is no way to process
only user tables and skip catalogs. So this is better for
composability.

No objections from me to keep this distinction at the end, as long as
the the database name in the command has no impact on the chosen
behavior.

OK, that's clear. Will progress.

Attached patch is tested, documented and imho ready to be committed,
so I will mark it so in CFapp.

--
Simon Riggs http://www.EnterpriseDB.com/

On Tue, Jun 28, 2022 at 11:02:25AM +0100, Simon Riggs wrote:

Attached patch is tested, documented and imho ready to be committed,
so I will mark it so in CFapp.

The behavior introduced by this patch should be reflected in
reindexdb. See in particular reindex_one_database(), where a
REINDEX_SYSTEM is enforced first on the catalogs for the
non-concurrent mode when running the reindex on a database.

+-- unqualified reindex database
+-- if you want to test REINDEX DATABASE, uncomment the following line,
+-- but note that this adds about 0.5s to the regression tests and the
+-- results are volatile when run in parallel to other tasks. Note also
+-- that REINDEX SYSTEM is specifically not tested because it can deadlock.
+-- REINDEX (VERBOSE) DATABASE;

No need to add that IMHO.

REINDEX [ ( <replaceable class="parameter">option</replaceable> [,
...] ) ] { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } [
CONCURRENTLY ] <replaceable class="parameter">name</replaceable>
+REINDEX [ ( <replaceable class="parameter">option</replaceable> [,
...] ) ] { DATABASE | SYSTEM } [ CONCURRENTLY ] [ <replaceable
class="parameter">name</replaceable> ]

Shouldn't you remove DATABASE and SYSTEM from the first line, keeping
only INDEX. TABLE and SCHEMA? The second line, with its optional
"name" would cover the DATABASE and SYSTEM cases at 100%.

-        if (strcmp(objectName, get_database_name(objectOid)) != 0)
+        if (objectName && strcmp(objectName, get_database_name(objectOid)) != 0)
             ereport(ERROR,
                     (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                      errmsg("can only reindex the currently open database")));
         if (!pg_database_ownercheck(objectOid, GetUserId()))
             aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_DATABASE,
-                           objectName);
+                           get_database_name(objectOid));

This could call get_database_name() just once.

+            * You might think it would be good to include catalogs,
+            * but doing that can deadlock, so isn't much use in real world,
+            * nor can we safely test that it even works.

Not sure what you mean here exactly.
--
Michael

On Wed, 29 Jun 2022 at 05:35, Michael Paquier <michael@paquier.xyz> wrote:

On Tue, Jun 28, 2022 at 11:02:25AM +0100, Simon Riggs wrote:

Attached patch is tested, documented and imho ready to be committed,
so I will mark it so in CFapp.

Thanks for the review Michael.

The behavior introduced by this patch should be reflected in
reindexdb. See in particular reindex_one_database(), where a
REINDEX_SYSTEM is enforced first on the catalogs for the
non-concurrent mode when running the reindex on a database.

Originally, I was trying to avoid changing prior behavior, but now
that we have agreed to do so, this makes sense.

That section of code has been removed, tests updated. No changes to
docs seem to be required.

+-- unqualified reindex database
+-- if you want to test REINDEX DATABASE, uncomment the following line,
+-- but note that this adds about 0.5s to the regression tests and the
+-- results are volatile when run in parallel to other tasks. Note also
+-- that REINDEX SYSTEM is specifically not tested because it can deadlock.
+-- REINDEX (VERBOSE) DATABASE;

No need to add that IMHO.

That was more a comment to reviewer, but I think something should be
said for later developers.

REINDEX [ ( <replaceable class="parameter">option</replaceable> [,
...] ) ] { INDEX | TABLE | SCHEMA | DATABASE | SYSTEM } [
CONCURRENTLY ] <replaceable class="parameter">name</replaceable>
+REINDEX [ ( <replaceable class="parameter">option</replaceable> [,
...] ) ] { DATABASE | SYSTEM } [ CONCURRENTLY ] [ <replaceable
class="parameter">name</replaceable> ]

Shouldn't you remove DATABASE and SYSTEM from the first line, keeping
only INDEX. TABLE and SCHEMA? The second line, with its optional
"name" would cover the DATABASE and SYSTEM cases at 100%.

I agree that your proposal is clearer. Done.

-        if (strcmp(objectName, get_database_name(objectOid)) != 0)
+        if (objectName && strcmp(objectName, get_database_name(objectOid)) != 0)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("can only reindex the currently open database")));
if (!pg_database_ownercheck(objectOid, GetUserId()))
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_DATABASE,
-                           objectName);
+                           get_database_name(objectOid));

This could call get_database_name() just once.

It could, but I couldn't see any benefit in changing that for the code
under discussion.

If calling get_database_name() multiple times is an issue, I've added
a cache for that - another patch attached, if you think its worth it.

+            * You might think it would be good to include catalogs,
+            * but doing that can deadlock, so isn't much use in real world,
+            * nor can we safely test that it even works.

Not sure what you mean here exactly.

REINDEX SYSTEM can deadlock, which is why we are avoiding it.

This was a comment to later developers as to why things are done that
way. Feel free to update the wording or location, but something should
be mentioned to avoid later discussion.

Thanks for the review, new version attached.

--
Simon Riggs http://www.EnterpriseDB.com/

Simon Riggs <simon.riggs@enterprisedb.com> writes:

Thanks for the review, new version attached.

This is marked as Ready for Committer, but that seems unduly
optimistic. The cfbot shows that it's failing on all platforms ---
and not in the same way on each, suggesting there are multiple
problems.

regards, tom lane