Обсуждение: [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",

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

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

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