Thread: [PATCH] Tab completion for VACUUM of partitioned tables
Could maybe backpatch to v10.
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 272f799c24..06ef658afb 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -578,14 +578,23 @@ static const SchemaQuery Query_for_list_of_vacuumables = {
.catname = "pg_catalog.pg_class c",
.selcondition =
"c.relkind IN (" CppAsString2(RELKIND_RELATION) ", "
+ CppAsString2(RELKIND_PARTITIONED_TABLE) ", "
CppAsString2(RELKIND_MATVIEW) ")",
.viscondition = "pg_catalog.pg_table_is_visible(c.oid)",
.namespace = "c.relnamespace",
.result = "pg_catalog.quote_ident(c.relname)",
};
-/* Relations supporting CLUSTER are currently same as those supporting VACUUM */
-#define Query_for_list_of_clusterables Query_for_list_of_vacuumables
+/* Relations supporting CLUSTER */
+static const SchemaQuery Query_for_list_of_clusterables = {
+ .catname = "pg_catalog.pg_class c",
+ .selcondition =
+ "c.relkind IN (" CppAsString2(RELKIND_RELATION) ", "
+ CppAsString2(RELKIND_MATVIEW) ")",
+ .viscondition = "pg_catalog.pg_table_is_visible(c.oid)",
+ .namespace = "c.relnamespace",
+ .result = "pg_catalog.quote_ident(c.relname)",
+};
static const SchemaQuery Query_for_list_of_constraints_with_schema = {
.catname = "pg_catalog.pg_constraint c",
Attachment
On Wed, 29 Jul 2020 at 02:04, Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> Could maybe backpatch to v10.
>
> diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
> index 272f799c24..06ef658afb 100644
> --- a/src/bin/psql/tab-complete.c
> +++ b/src/bin/psql/tab-complete.c
> @@ -578,14 +578,23 @@ static const SchemaQuery Query_for_list_of_vacuumables = {
> .catname = "pg_catalog.pg_class c",
> .selcondition =
> "c.relkind IN (" CppAsString2(RELKIND_RELATION) ", "
> + CppAsString2(RELKIND_PARTITIONED_TABLE) ", "
> CppAsString2(RELKIND_MATVIEW) ")",
> .viscondition = "pg_catalog.pg_table_is_visible(c.oid)",
> .namespace = "c.relnamespace",
> .result = "pg_catalog.quote_ident(c.relname)",
> };
>
> -/* Relations supporting CLUSTER are currently same as those supporting VACUUM */
> -#define Query_for_list_of_clusterables Query_for_list_of_vacuumables
> +/* Relations supporting CLUSTER */
> +static const SchemaQuery Query_for_list_of_clusterables = {
> + .catname = "pg_catalog.pg_class c",
> + .selcondition =
> + "c.relkind IN (" CppAsString2(RELKIND_RELATION) ", "
> + CppAsString2(RELKIND_MATVIEW) ")",
> + .viscondition = "pg_catalog.pg_table_is_visible(c.oid)",
> + .namespace = "c.relnamespace",
> + .result = "pg_catalog.quote_ident(c.relname)",
> +};
>
> static const SchemaQuery Query_for_list_of_constraints_with_schema = {
> .catname = "pg_catalog.pg_constraint c",
Good catch. The patch looks good to me.
Regards,
--
Masahiko Sawada http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Jul 29, 2020 at 01:27:07PM +0900, Masahiko Sawada wrote: > Good catch. The patch looks good to me. While this patch is logically correct. I think that we should try to not increase more the number of queries used to scan pg_class depending on a list of relkinds. For example, did you notice that your new Query_for_list_of_vacuumables becomes the same query as Query_for_list_of_indexables? You can make your patch more simple. -- Michael
Attachment
On Wed, 29 Jul 2020 at 15:21, Michael Paquier <michael@paquier.xyz> wrote: > > On Wed, Jul 29, 2020 at 01:27:07PM +0900, Masahiko Sawada wrote: > > Good catch. The patch looks good to me. > > While this patch is logically correct. I think that we should try to > not increase more the number of queries used to scan pg_class > depending on a list of relkinds. For example, did you notice that > your new Query_for_list_of_vacuumables becomes the same query as > Query_for_list_of_indexables? Oh, I didn't realize that. Looking at target relation kinds for operations in-depth, I think that the relation list for index creation and the relation list for vacuum is different. Query_for_list_of_indexables should search for: RELKIND_RELATION RELKIND_PARTITIONED_TABLE RELKIND_MATVIEW whereas Query_for_list_of_vacuumables should search for: RELKIND_RELATION RELKIND_PARTITIONED_TABLE RELKIND_MATVIEW RELKIND_TOASTVALUE Also, Query_for_list_of_clusterables is further different from the above two lists. It should search for: RELKIND_RELATION RELKIND_MATVIEW RELKIND_TOASTVALUE Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Jul 29, 2020 at 06:41:16PM +0900, Masahiko Sawada wrote: > whereas Query_for_list_of_vacuumables should search for: > > RELKIND_RELATION > RELKIND_PARTITIONED_TABLE > RELKIND_MATVIEW > RELKIND_TOASTVALUE FWIW, I don't think that we should make toast relations suggested to the user at all for any command. This comes down to the same point that we don't have pg_toast in search_path, and going down to this level of operations is an expert-level mode, not something we should recommend to the average user in psql IMO. -- Michael
Attachment
On 2020/07/30 3:33, Justin Pryzby wrote: > On Wed, Jul 29, 2020 at 03:21:19PM +0900, Michael Paquier wrote: >> On Wed, Jul 29, 2020 at 01:27:07PM +0900, Masahiko Sawada wrote: >>> Good catch. The patch looks good to me. >> >> While this patch is logically correct. I think that we should try to >> not increase more the number of queries used to scan pg_class >> depending on a list of relkinds. For example, did you notice that >> your new Query_for_list_of_vacuumables becomes the same query as >> Query_for_list_of_indexables? You can make your patch more simple. > > I didn't notice. There's an argument for keeping them separate, but as long as > there's a #define in between, this is fine, too. > > On Wed, Jul 29, 2020 at 08:05:57PM +0900, Michael Paquier wrote: >> On Wed, Jul 29, 2020 at 06:41:16PM +0900, Masahiko Sawada wrote: >>> whereas Query_for_list_of_vacuumables should search for: >>> >>> RELKIND_RELATION >>> RELKIND_PARTITIONED_TABLE >>> RELKIND_MATVIEW >>> RELKIND_TOASTVALUE >> >> FWIW, I don't think that we should make toast relations suggested to >> the user at all for any command. This comes down to the same point >> that we don't have pg_toast in search_path, and going down to this >> level of operations is an expert-level mode, not something we should >> recommend to the average user in psql IMO. > > Right. Tom's response to that suggestion a couple years ago I thought was > pretty funny (I picture Dr. Claw at his desk using psql tab completion being > presented with a list of pg_toast.pg_toast_NNNNNN OIDs: "which TOAST table > should I vacuum next..") > > https://www.postgresql.org/message-id/14255.1536781029@sss.pgh.pa.us > |I don't actually think that's a good idea. It's more likely to clutter > |peoples' completion lists than offer anything they want. Even if someone > |actually does want to vacuum a toast table, they are not likely to be > |entering its name via tab completion; they're going to have identified > |which table they want via some query, and then they'll be doing something > |like copy-and-paste out of a query result. Isn't it better to add the comment explaining why toast tables are excluded from the tab completion for vacuum while they are vacuumable? The patch looks good to me except that. Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On 2020/07/30 10:46, Michael Paquier wrote: > On Thu, Jul 30, 2020 at 08:44:26AM +0900, Fujii Masao wrote: >> Isn't it better to add the comment explaining why toast tables are >> excluded from the tab completion for vacuum while they are vacuumable? > > Sounds sensible, still it does not apply only to vacuum. I would go > as far as just adding a comment at the beginning of the block for > schema queries: Yes, that seems better. BTW, one thing I think a bit strange is that indexes for toast tables are included in tab-completion for REINDEX, for example. That is, "REINDEX INDEX<tab>" displays "pg_toast.", and "REINDEX INDEX pg_toast.<tab>" displays indexes for toast tables. Maybe it's better to exclude them, too. But there seems no simple way to do that. So I'm ok with this current situation. > "Never include toast tables in any of those queries to avoid > unnecessary bloat in the completions." > >> The patch looks good to me except that. > > Indeed. FWIW, I would also adjust the comment on top of > Query_for_list_of_indexables to not say "index creation", but just > "supporting indexing" instead. > > Fujii-san, perhaps you would prefer taking care of this patch? I am > fine to do it if you wish. Of course I'm fine if you work on this patch. So please feel free to do that! Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On Thu, 30 Jul 2020 at 12:24, Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > > > On 2020/07/30 10:46, Michael Paquier wrote: > > On Thu, Jul 30, 2020 at 08:44:26AM +0900, Fujii Masao wrote: > >> Isn't it better to add the comment explaining why toast tables are > >> excluded from the tab completion for vacuum while they are vacuumable? > > > > Sounds sensible, still it does not apply only to vacuum. I would go > > as far as just adding a comment at the beginning of the block for > > schema queries: > > Yes, that seems better. Agreed. > BTW, one thing I think a bit strange is that indexes for toast tables > are included in tab-completion for REINDEX, for example. That is, > "REINDEX INDEX<tab>" displays "pg_toast.", and "REINDEX INDEX pg_toast.<tab>" > displays indexes for toast tables. Maybe it's better to exclude them, > too. But there seems no simple way to do that. > So I'm ok with this current situation. Yeah, that's the reason why I mentioned about toast tables. "VACUUM <tab>" displays "pg_toast." but, "VACUUM pg_to<tab>" doesn't complement anything. Regards, -- Masahiko Sawada http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Jul 30, 2020 at 02:16:04PM +0900, Masahiko Sawada wrote: > Yeah, that's the reason why I mentioned about toast tables. "VACUUM > <tab>" displays "pg_toast." but, "VACUUM pg_to<tab>" doesn't > complement anything. So am I OK with this situation. The same is true as well for CLUSTER and TRUNCATE, and "pg_to" would get completion with the toast tables only if we begin to add RELKIND_TOASTVALUE to the queries. Note that the schema completions come from _complete_from_query() where we would need to be smarter regarding the filtering of pg_namespace rows and I have not looked how to do that, but I feel that it may not be that complicated. For now I have applied the proposed patch. -- Michael