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
