Thread

Unreadable, unmaintainable code in tab completion

Tom Lane <tgl@sss.pgh.pa.us> — 2018-09-20T22:07:34Z

At some point, somebody had the bright idea that various queries
in tab-complete.c should be named like this:

Query_for_list_of_tsvmf
Query_for_list_of_tmf
Query_for_list_of_tpm
Query_for_list_of_tm

Even assuming that you can fill in the unstated "relations of these
relkinds", I challenge anybody to know right offhand what those sets
correspond to semantically.  Or to figure out what should change if
a new relkind gets added.  Don't expect to get any help from the
comments, because there are none.

It did not help any when somebody else decided to cram the new
partitioned-table relkind into some (not all) of these queries without
renaming them, so that even this crummy naming principle no longer
applies at all.

Meanwhile, immediately adjacent to those we have a far better design
example:

/* Relations supporting INSERT, UPDATE or DELETE */
static const SchemaQuery Query_for_list_of_updatables = {

Here we have something where it's actually possible to decide in
a principled fashion whether a new relkind belongs in the query's
relkind set, and where we aren't faced with a need to rename the
query to reflect such a change.  And it's way more readable, if you
ask me.

So I think we ought to rename these query variables on that model.
Looking through the references to each of these, it appears that
what they are really doing is:

Query_for_list_of_tsvmf		SELECT, TABLE, GRANT/REVOKE [table], \dp
Query_for_list_of_tmf		ANALYZE
Query_for_list_of_tpm		CREATE INDEX ON
Query_for_list_of_tm		VACUUM, CLUSTER, REINDEX TABLE

Here's what I propose:

* Rename Query_for_list_of_tsvmf to Query_for_list_of_selectables,
and then add

	/* Currently, selectable rels are exactly the ones you can GRANT on */
	#define Query_for_list_of_grantables Query_for_list_of_selectables

and use those two names as appropriate at the call sites.  If the
relkinds for those two behaviors ever diverge, we can replace the
#define with a separate constant, but we needn't do so today.

* Rename Query_for_list_of_tmf to Query_for_list_of_analyzables.

* Rename Query_for_list_of_tpm to Query_for_list_of_indexables.

* Rename Query_for_list_of_tm to Query_for_list_of_vacuumables,
and add a comment saying that this currently covers CLUSTER as well.
(Or we could add a #define like that for the GRANT case.)

* Change the reference in REINDEX TABLE to reference
Query_for_list_of_indexables not Query_for_list_of_vacuumables.

The last item will have the effect that partitioned tables are offered
for REINDEX TABLE.  Now, if you actually try that, you get

WARNING:  REINDEX of partitioned tables is not yet implemented, skipping "mlparted"
NOTICE:  table "mlparted" has no indexes
REINDEX

but I think tab-complete.c has exactly no business knowing that
that isn't implemented yet.  Are we going to remember to change
it when that does get implemented?  Are we going to put in a
server-version-specific completion rule?  I'd say the answers
are "maybe not" and "definitely not", so we might as well just
allow it now and not have to fix this code when it gets changed.

I haven't written the actual patch yet, but the above sketch
seems sufficient to explain it.  Any objections, or bikeshedding
on the names?

			regards, tom lane