Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix newlines in \d output #362

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

henlue
Copy link
Contributor

@henlue henlue commented Aug 26, 2022

This fixes the output of \d *. There were too many newlines printed at the beginning of the output. And no newlines were printed to separate indices and sequences. E.g.:

pg:postgres@localhost/postgres=> \d *




             table "public.table1"
 Name |       Type        | Nullable | Default
------+-------------------+----------+---------
 id   | integer           | "NO"     |
 name | character varying | "YES"    |
Indexes:
  "table1_pkey" PRIMARY_KEY, UNIQUE, index (id)

             table "public.table2"
 Name |       Type        | Nullable | Default
------+-------------------+----------+---------
 id   | integer           | "NO"     |
 name | character varying | "YES"    |
Indexes:
  "table2_pkey" PRIMARY_KEY, UNIQUE, index (id)

                   Sequence "public.sequence1"
  Type  | Start | Min |         Max         | Increment | Cycles?
--------+-------+-----+---------------------+-----------+---------
 bigint | 1     | 1   | 9223372036854775807 | 1         | "NO"
                   Sequence "public.sequence2"
  Type  | Start | Min |         Max         | Increment | Cycles?
--------+-------+-----+---------------------+-----------+---------
 bigint | 1     | 1   | 9223372036854775807 | 1         | "NO"
Index "public.table1_pkey"
 Name |  Type
------+---------
 id   | integer
primary key, index, for table table1
Index "public.table2_pkey"
 Name |  Type
------+---------
 id   | integer
primary key, index, for table table2

TestWriter now also succeeds for pgsql descTable
go test ./drivers/ --dbs pgsql --cleanup=false

I restricted the postgres implementation of Tables() to only return tables, views, materialized views and sequences. Technically this would not be necessary since all calls to Tables() now specifically request the types of database objects they require. But still I think it is cleaner since other implementations (mostly information_schema) behave like this as well.

This PR depends on (PR36)[https://github.com/xo/tblfmt/pull/36] in tblfmt.

drivers/metadata/postgres/metadata.go Show resolved Hide resolved
@@ -505,6 +504,7 @@ func (w DefaultWriter) describeSequences(sp, tp string, verbose, showSystem bool
params["footer"] = "off"
params["title"] = fmt.Sprintf("Sequence \"%s.%s\"\n", s.Schema, s.Name)
err = tblfmt.EncodeAll(w.w, rows, params)
w.w.Write([]byte("\n"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it needed only in sequences? we don't add an extra newline after other calls to tblfmt.EncodeAll().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definitely is incorrect. The EncodeAll should handle all output. Possibly the EncodeAll implementation is wrong, but no additional output should be emitted here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry this will be a bit longer :-)

All other calls to EncodeAll are executed as part of backslash commands that only print a single table. They all have a single newline at the end of the table. I.e. the new prompt is displayed directly below the table. E.g:

pg:postgres@localhost/postgres=> \dt
               List of relations
 Schema |             Name             |   Type   
--------+------------------------------+----------
 public | test_table                   | table 
(1 row)
pg:postgres@localhost/postgres=>

psql behaves differently in this case. It prints one more newline after the "(1 row)".

In the output of \d this becomes an actual issue since it is printing multiple tables. E.g:

table "public.test_table_1"
 Name |  Type   | Nullable | Default
------+---------+----------+---------
 id   | integer | "YES"    |
Indexes:
  "test_index" PRIMARY_KEY, index (id)

table "public.test_table_2"     
 Name |  Type   | Nullable | Default
------+---------+----------+---------
 id   | integer | "YES"    |

Index "public.test_index_1" 
 Name |  Type   
------+---------
 id   | integer 
primary key, index, for table test_table_1

Here we definitely need the additional newline. Otherwise there would be no space between the table summary and the next table. \d is calling describeTableDetails, describeSequences and describeIndex.

In describeTableDetails the extra newline is created as part of the summary. The summary callback ends in a newline and tblfmt adds another newline (https://github.com/xo/tblfmt/blob/master/encode.go#L580).

In describeIndex I created the same behavior by adding a newline to the summary.

describeSequences is calling EncodeAll without a summary callback, so I manually added the newline here. The only way I see how to create a double newline with EncodeAll for this case would be to have a summary callback with an empty summary (an empty summary will trigger tblfmt to add a newline). Otherwise it might make sense to add an option to the tblfmt encoder to create an extra newline at the end of the table (with or without summary). I don't think there is such an option currently.

So overall I see two options:

  1. Continue to create the double newline as part of the summary. If there is no summary an empty summary would need to be created for an extra newline. A newline could also be added to the default summary to fix the first example.
  2. Add an option to the tblfmt encoder to optionally add a newline at the end of the table.

I think the second option is cleaner.

Copy link
Contributor Author

@henlue henlue Sep 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created a PR for the required changes to tblfmt. The PR additionally fixes the output of the other \d commands by adding a trailing newline by default.

This PR currently points to the tblfmt branch in my fork. I will revert the changes to go.mod once the tblfmt changes are merged.

TestWriter test now succeeds for pgsql descTable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants