Include access method in listTables output

On Tue, 9 Jun 2020 at 23:03, Georgios <gkokolatos@protonmail.com> wrote:

A small patch is attached [1] to see if you think it makes sense. I have not included any
differences in the tests output yet, as the idea might get discarded. However if the patch is
found useful. I shall ament the test results as needed.

It seems like a fair thing to need/want. We do already show this in
\d+ tablename, so it seems pretty fair to want it in the \d+ output
too

Please add it to the commitfest at https://commitfest.postgresql.org/28/

David

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Tuesday, June 9, 2020 1:34 PM, David Rowley <dgrowleyml@gmail.com> wrote:

On Tue, 9 Jun 2020 at 23:03, Georgios gkokolatos@protonmail.com wrote:

A small patch is attached [1] to see if you think it makes sense. I have not included any
differences in the tests output yet, as the idea might get discarded. However if the patch is
found useful. I shall ament the test results as needed.

It seems like a fair thing to need/want. We do already show this in
\d+ tablename, so it seems pretty fair to want it in the \d+ output
too

Please add it to the commitfest at https://commitfest.postgresql.org/28/

Thank you very much for your time. Added to the commitfest as suggested.

On Tue, Jun 9, 2020 at 6:45 PM Georgios <gkokolatos@protonmail.com> wrote:

Please add it to the commitfest at https://commitfest.postgresql.org/28/

Thank you very much for your time. Added to the commitfest as suggested.

Patch applies cleanly, make check & make check-world passes.

Few comments:
+ if (pset.sversion >= 120000)
+ appendPQExpBufferStr(&buf,
+ "\n     LEFT JOIN pg_catalog.pg_am am ON am.oid = c.relam");

Should we include pset.hide_tableam check along with the version check?

+ if (pset.sversion >= 120000 && !pset.hide_tableam)
+ {
+ appendPQExpBuffer(&buf,
+   ",\n  am.amname as \"%s\"",
+   gettext_noop("Access Method"));
+ }

Braces can be removed & the second line can be combined with earlier.

We could include the test in psql file by configuring HIDE_TABLEAM.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, June 15, 2020 3:20 AM, vignesh C <vignesh21@gmail.com> wrote:

On Tue, Jun 9, 2020 at 6:45 PM Georgios gkokolatos@protonmail.com wrote:

Please add it to the commitfest at https://commitfest.postgresql.org/28/

Thank you very much for your time. Added to the commitfest as suggested.

Patch applies cleanly, make check & make check-world passes.

Thank you for your time and comments! Please find v2 of the patch
attached.

Few comments:

- if (pset.sversion >= 120000)

- appendPQExpBufferStr(&buf,

- "\n LEFT JOIN pg_catalog.pg_am am ON am.oid = c.relam");

Should we include pset.hide_tableam check along with the version check?

I opted against it, since it seems more intuitive to have a single
switch and placed on the display part. A similar pattern can be found
in describeOneTableDetails(). I do not hold a strong opinion and will
gladly ament if insisted upon.

- if (pset.sversion >= 120000 && !pset.hide_tableam)

- {

- appendPQExpBuffer(&buf,

- ",\n am.amname as \"%s\"",

- gettext_noop("Access Method"));

- }

Braces can be removed & the second line can be combined with earlier.

Agreed on the braces.

Disagree on the second line as this style is in line with the rest of
code. Consistency, buf, format string and gettext_noop() are found in
their own lines within this file.

We could include the test in psql file by configuring HIDE_TABLEAM.

Agreed.

On Tue, Jun 16, 2020 at 6:13 PM Georgios <gkokolatos@protonmail.com> wrote:

Few comments:

- if (pset.sversion >= 120000)

- appendPQExpBufferStr(&buf,

- "\n LEFT JOIN pg_catalog.pg_am am ON am.oid = c.relam");

Should we include pset.hide_tableam check along with the version check?

I opted against it, since it seems more intuitive to have a single
switch and placed on the display part. A similar pattern can be found
in describeOneTableDetails(). I do not hold a strong opinion and will
gladly ament if insisted upon.

I felt we could add that check as we might be including
pg_catalog.pg_am in cases even though we really don't need it.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Saturday, June 20, 2020 3:15 PM, vignesh C <vignesh21@gmail.com> wrote:

On Tue, Jun 16, 2020 at 6:13 PM Georgios gkokolatos@protonmail.com wrote:

Few comments:

- if (pset.sversion >= 120000)

- appendPQExpBufferStr(&buf,

- "\n LEFT JOIN pg_catalog.pg_am am ON am.oid = c.relam");
Should we include pset.hide_tableam check along with the version check?

I opted against it, since it seems more intuitive to have a single
switch and placed on the display part. A similar pattern can be found
in describeOneTableDetails(). I do not hold a strong opinion and will
gladly ament if insisted upon.

I felt we could add that check as we might be including
pg_catalog.pg_am in cases even though we really don't need it.

As promised, I gladly ament upon your request. Also included a fix for
a minor oversight in tests, they should now be stable. Finally in this
version, I extended a bit the logic to only include the access method column
if the relations displayed can have one, for example sequences.

On Tue, Jun 30, 2020 at 2:53 PM Georgios <gkokolatos@protonmail.com> wrote:

As promised, I gladly ament upon your request. Also included a fix for
a minor oversight in tests, they should now be stable. Finally in this
version, I extended a bit the logic to only include the access method column
if the relations displayed can have one, for example sequences.

Patch applies cleanly, make check & make check-world passes.
One comment:
+               if (pset.sversion >= 120000 && !pset.hide_tableam &&
+                       (showTables || showViews || showMatViews ||
showIndexes))
+                       appendPQExpBuffer(&buf,
+                                                         ",\n
am.amname as \"%s\"",
+
gettext_noop("Access Method"));

I'm not sure if we should include showViews, I had seen that the
access method was not getting selected for view.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

On Sun, Jul 05, 2020 at 07:13:10AM +0530, vignesh C wrote:

I'm not sure if we should include showViews, I had seen that the
access method was not getting selected for view.

+1.  These have no physical storage, so you are looking here for
relkinds that satisfy RELKIND_HAS_STORAGE().
--
Michael

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, July 6, 2020 3:12 AM, Michael Paquier <michael@paquier.xyz> wrote:

On Sun, Jul 05, 2020 at 07:13:10AM +0530, vignesh C wrote:

I'm not sure if we should include showViews, I had seen that the
access method was not getting selected for view.

+1. These have no physical storage, so you are looking here for
relkinds that satisfy RELKIND_HAS_STORAGE().

Thank you for the review.
Find attached v4 of the patch.

On Mon, Jul 6, 2020 at 1:24 PM Georgios <gkokolatos@protonmail.com> wrote:

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, July 6, 2020 3:12 AM, Michael Paquier <michael@paquier.xyz> wrote:

On Sun, Jul 05, 2020 at 07:13:10AM +0530, vignesh C wrote:

I'm not sure if we should include showViews, I had seen that the
access method was not getting selected for view.

+1. These have no physical storage, so you are looking here for
relkinds that satisfy RELKIND_HAS_STORAGE().

Thank you for the review.
Find attached v4 of the patch.

Thanks for fixing the comments.
Patch applies cleanly, make check & make check-world passes.
I was not sure if any documentation change is required or not for this
in doc/src/sgml/ref/psql-ref.sgml. Thoughts?

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Saturday, July 11, 2020 3:16 PM, vignesh C <vignesh21@gmail.com> wrote:

On Mon, Jul 6, 2020 at 1:24 PM Georgios gkokolatos@protonmail.com wrote:

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, July 6, 2020 3:12 AM, Michael Paquier michael@paquier.xyz wrote:

On Sun, Jul 05, 2020 at 07:13:10AM +0530, vignesh C wrote:

I'm not sure if we should include showViews, I had seen that the
access method was not getting selected for view.

+1. These have no physical storage, so you are looking here for
relkinds that satisfy RELKIND_HAS_STORAGE().

Thank you for the review.
Find attached v4 of the patch.

Thanks for fixing the comments.
Patch applies cleanly, make check & make check-world passes.
I was not sure if any documentation change is required or not for this
in doc/src/sgml/ref/psql-ref.sgml. Thoughts?

Thank you for the comment. I added a line in docs. Admittedly, might need tweaking.

Please find version 5 of the patch attached.

On Thu, Jul 16, 2020 at 7:35 PM Georgios <gkokolatos@protonmail.com> wrote:

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Saturday, July 11, 2020 3:16 PM, vignesh C <vignesh21@gmail.com> wrote:

On Mon, Jul 6, 2020 at 1:24 PM Georgios gkokolatos@protonmail.com wrote:

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, July 6, 2020 3:12 AM, Michael Paquier michael@paquier.xyz wrote:

On Sun, Jul 05, 2020 at 07:13:10AM +0530, vignesh C wrote:

I'm not sure if we should include showViews, I had seen that the
access method was not getting selected for view.

+1. These have no physical storage, so you are looking here for
relkinds that satisfy RELKIND_HAS_STORAGE().

Thank you for the review.
Find attached v4 of the patch.

Thanks for fixing the comments.
Patch applies cleanly, make check & make check-world passes.
I was not sure if any documentation change is required or not for this
in doc/src/sgml/ref/psql-ref.sgml. Thoughts?

Thank you for the comment. I added a line in docs. Admittedly, might need tweaking.

Please find version 5 of the patch attached.

Most changes looks fine, minor comments:

 \pset tuples_only false
 -- check conditional tableam display
--- Create a heap2 table am handler with heapam handler
+\pset expanded off
+CREATE SCHEMA conditional_tableam_display;
+CREATE ROLE conditional_tableam_display_role;
+ALTER SCHEMA conditional_tableam_display OWNER TO
conditional_tableam_display_role;
+SET search_path TO conditional_tableam_display;
 CREATE ACCESS METHOD heap_psql TYPE TABLE HANDLER heap_tableam_handler;

This comment might have been removed unintentionally, do you want to
add it back?

+-- access method column should not be displayed for sequences
+\ds+
+                        List of relations
+ Schema | Name | Type | Owner | Persistence | Size | Description
+--------+------+------+-------+-------------+------+-------------
+(0 rows)

We can include one test for view.

+ if (pset.sversion >= 120000 && !pset.hide_tableam &&
+ (showTables || showMatViews || showIndexes))
+ appendPQExpBuffer(&buf,
+   ",\n  am.amname as \"%s\"",
+   gettext_noop("Access Method"));
+
  /*
  * As of PostgreSQL 9.0, use pg_table_size() to show a more accurate
  * size of a table, including FSM, VM and TOAST tables.
@@ -3772,6 +3778,12 @@ listTables(const char *tabtypes, const char
*pattern, bool verbose, bool showSys
  appendPQExpBufferStr(&buf,
  "\nFROM pg_catalog.pg_class c"
  "\n     LEFT JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace");
+
+ if (pset.sversion >= 120000 && !pset.hide_tableam &&
+ (showTables || showMatViews || showIndexes))

If conditions in both the places are the same, do you want to make it a macro?

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Saturday, July 25, 2020 2:41 AM, vignesh C <vignesh21@gmail.com> wrote:

On Thu, Jul 16, 2020 at 7:35 PM Georgios gkokolatos@protonmail.com wrote:

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Saturday, July 11, 2020 3:16 PM, vignesh C vignesh21@gmail.com wrote:

On Mon, Jul 6, 2020 at 1:24 PM Georgios gkokolatos@protonmail.com wrote:

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, July 6, 2020 3:12 AM, Michael Paquier michael@paquier.xyz wrote:

On Sun, Jul 05, 2020 at 07:13:10AM +0530, vignesh C wrote:

I'm not sure if we should include showViews, I had seen that the
access method was not getting selected for view.

+1. These have no physical storage, so you are looking here for
relkinds that satisfy RELKIND_HAS_STORAGE().

Thank you for the review.
Find attached v4 of the patch.

Thanks for fixing the comments.
Patch applies cleanly, make check & make check-world passes.
I was not sure if any documentation change is required or not for this
in doc/src/sgml/ref/psql-ref.sgml. Thoughts?

Thank you for the comment. I added a line in docs. Admittedly, might need tweaking.
Please find version 5 of the patch attached.

Most changes looks fine, minor comments:

Thank you for your comments. Please find the individual answers inline for completeness.

I'm having issues understanding where you are going with the reviews, can you fully describe
what is it that you wish to be done?

\pset tuples_only false
-- check conditional tableam display
--- Create a heap2 table am handler with heapam handler
+\pset expanded off
+CREATE SCHEMA conditional_tableam_display;
+CREATE ROLE conditional_tableam_display_role;
+ALTER SCHEMA conditional_tableam_display OWNER TO
conditional_tableam_display_role;
+SET search_path TO conditional_tableam_display;
CREATE ACCESS METHOD heap_psql TYPE TABLE HANDLER heap_tableam_handler;

This comment might have been removed unintentionally, do you want to
add it back?

It was intentional as heap2 table am handler is not getting created. With
the code additions the comment does seem out of place and thus removed.

+-- access method column should not be displayed for sequences
+\ds+

- List of relations

-   Schema | Name | Type | Owner | Persistence | Size | Description
+--------+------+------+-------+-------------+------+-------------
+(0 rows)

We can include one test for view.

The list of cases in the test for both including and excluding storage is by no means
exhaustive. I thought that this is acceptable. Adding a test for the view, will still
not make it the test exhaustive. Are you sure you only suggest the view to be included
or you would suggest an exhaustive list of tests.

- if (pset.sversion >= 120000 && !pset.hide_tableam &&

- (showTables || showMatViews || showIndexes))

- appendPQExpBuffer(&buf,

- ",\n am.amname as \"%s\"",

- gettext_noop("Access Method"));

-   /*
-   As of PostgreSQL 9.0, use pg_table_size() to show a more accurate
-   size of a table, including FSM, VM and TOAST tables.
@@ -3772,6 +3778,12 @@ listTables(const char *tabtypes, const char
*pattern, bool verbose, bool showSys
appendPQExpBufferStr(&buf,
"\nFROM pg_catalog.pg_class c"
"\n LEFT JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace");

-
- if (pset.sversion >= 120000 && !pset.hide_tableam &&

- (showTables || showMatViews || showIndexes))

If conditions in both the places are the same, do you want to make it a macro?

No, I would rather not if you may. I think that a macro would not add to the readability
rather it would remove from it. Those are two simple conditionals used in the same function
very close to each other.

On Wed, Jul 29, 2020 at 7:30 PM Georgios <gkokolatos@protonmail.com> wrote:

I'm having issues understanding where you are going with the reviews, can you fully describe
what is it that you wish to be done?

I'm happy with the patch, that was the last of the comments that I had
from my side. Only idea here is to review every line of the code
before passing it to the committer.

\pset tuples_only false
-- check conditional tableam display
--- Create a heap2 table am handler with heapam handler
+\pset expanded off
+CREATE SCHEMA conditional_tableam_display;
+CREATE ROLE conditional_tableam_display_role;
+ALTER SCHEMA conditional_tableam_display OWNER TO
conditional_tableam_display_role;
+SET search_path TO conditional_tableam_display;
CREATE ACCESS METHOD heap_psql TYPE TABLE HANDLER heap_tableam_handler;

This comment might have been removed unintentionally, do you want to
add it back?

It was intentional as heap2 table am handler is not getting created. With
the code additions the comment does seem out of place and thus removed.

Thanks for clarifying it, I wasn't sure if it was completely intentional.

+-- access method column should not be displayed for sequences
+\ds+

- List of relations

-   Schema | Name | Type | Owner | Persistence | Size | Description
+--------+------+------+-------+-------------+------+-------------
+(0 rows)

We can include one test for view.

The list of cases in the test for both including and excluding storage is by no means
exhaustive. I thought that this is acceptable. Adding a test for the view, will still
not make it the test exhaustive. Are you sure you only suggest the view to be included
or you would suggest an exhaustive list of tests.

I had noticed this case while reviewing, you can take a call on it. It
is very difficult to come to a conclusion on what needs to be included
and what need not be included. I had noticed you have added a test
case for sequence. I felt views are similar in this case.

- if (pset.sversion >= 120000 && !pset.hide_tableam &&

- (showTables || showMatViews || showIndexes))

- appendPQExpBuffer(&buf,

- ",\n am.amname as \"%s\"",

- gettext_noop("Access Method"));

-   /*
-   As of PostgreSQL 9.0, use pg_table_size() to show a more accurate
-   size of a table, including FSM, VM and TOAST tables.
@@ -3772,6 +3778,12 @@ listTables(const char *tabtypes, const char
*pattern, bool verbose, bool showSys
appendPQExpBufferStr(&buf,
"\nFROM pg_catalog.pg_class c"
"\n LEFT JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace");

-
- if (pset.sversion >= 120000 && !pset.hide_tableam &&

- (showTables || showMatViews || showIndexes))

If conditions in both the places are the same, do you want to make it a macro?

No, I would rather not if you may. I think that a macro would not add to the readability
rather it would remove from it. Those are two simple conditionals used in the same function
very close to each other.

Ok, we can ignore this.

Regards,
Vignesh

On Sat, Aug 1, 2020 at 8:12 AM vignesh C <vignesh21@gmail.com> wrote:

+-- access method column should not be displayed for sequences
+\ds+

- List of relations

-   Schema | Name | Type | Owner | Persistence | Size | Description
+--------+------+------+-------+-------------+------+-------------
+(0 rows)

We can include one test for view.

I felt adding one test for view is good and added it.
Attached a new patch for the same.

I felt patch is in shape for committer to have a look at this.

Regards,
Vignesh

On Mon, Aug 17, 2020 at 11:10:05PM +0530, vignesh C wrote:

On Sat, Aug 1, 2020 at 8:12 AM vignesh C <vignesh21@gmail.com> wrote:

+-- access method column should not be displayed for sequences
+\ds+

- List of relations

-   Schema | Name | Type | Owner | Persistence | Size | Description
+--------+------+------+-------+-------------+------+-------------
+(0 rows)

We can include one test for view.

I felt adding one test for view is good and added it.
Attached a new patch for the same.

I felt patch is in shape for committer to have a look at this.

The patch tester shows it's failing xmllint ; could you send a fixed patch ?

/usr/bin/xmllint --path . --noout --valid postgres.sgml
ref/psql-ref.sgml:1189: parser error : Opening and ending tag mismatch: link line 1187 and para

http://cfbot.cputube.org/georgios-kokolatos.html

--
Justin

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Thursday, 20 August 2020 07:31, Justin Pryzby <pryzby@telsasoft.com> wrote:

On Mon, Aug 17, 2020 at 11:10:05PM +0530, vignesh C wrote:

On Sat, Aug 1, 2020 at 8:12 AM vignesh C vignesh21@gmail.com wrote:

+-- access method column should not be displayed for sequences
+\ds+

- List of relations

-   Schema | Name | Type | Owner | Persistence | Size | Description
+--------+------+------+-------+-------------+------+-------------
+(0 rows)
We can include one test for view.

I felt adding one test for view is good and added it.
Attached a new patch for the same.
I felt patch is in shape for committer to have a look at this.

The patch tester shows it's failing xmllint ; could you send a fixed patch ?

/usr/bin/xmllint --path . --noout --valid postgres.sgml
ref/psql-ref.sgml:1189: parser error : Opening and ending tag mismatch: link line 1187 and para

http://cfbot.cputube.org/georgios-kokolatos.html

Thank you for pointing it out!

Please find version 7 attached which hopefully addresses the error along with a proper
expansion of the test coverage and removal of recently introduced
whitespace warnings.

On Thu, Aug 20, 2020 at 08:16:19AM +0000, Georgios wrote:

Please find version 7 attached which hopefully addresses the error along with a proper
expansion of the test coverage and removal of recently introduced
whitespace warnings.

+CREATE ROLE conditional_tableam_display_role;
As a convention, regression tests need to have roles prefixed with
"regress_" or this would cause some buildfarm members to turn red.
Please see -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS (you could use
that in your environment for example).

So, as of the tests.. The role gets added to make sure that when
using \d+ on the full schema as well as the various \d*+ variants we
have a consistent owner. The addition of the relation size for the
sequence and the btree index in the output generated is a problem
though, because that's not really portable when compiling with other
page sizes. It is true that there are other tests failing in this
case, but I think that we should try to limit that if we can. In
short, I agree that having some tests is better than nothing, but I
would suggest to reduce their scope, as per the attached.

Adding \dE as there are no foreign tables does not make much sense,
and also I wondered why \dt+ was not added.

Does the attached look correct to you?
--
Michael

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Tuesday, 1 September 2020 07:41, Michael Paquier <michael@paquier.xyz> wrote:

On Thu, Aug 20, 2020 at 08:16:19AM +0000, Georgios wrote:

Please find version 7 attached which hopefully addresses the error along with a proper
expansion of the test coverage and removal of recently introduced
whitespace warnings.

+CREATE ROLE conditional_tableam_display_role;
As a convention, regression tests need to have roles prefixed with
"regress_" or this would cause some buildfarm members to turn red.
Please see -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS (you could use
that in your environment for example).

I was wondering about the name. I hoped that it would either come up during review, or that it would be fine.

Thank you for the detailed explanation.

So, as of the tests.. The role gets added to make sure that when
using \d+ on the full schema as well as the various \d*+ variants we
have a consistent owner. The addition of the relation size for the
sequence and the btree index in the output generated is a problem
though, because that's not really portable when compiling with other
page sizes. It is true that there are other tests failing in this
case, but I think that we should try to limit that if we can. In
short, I agree that having some tests is better than nothing, but I
would suggest to reduce their scope, as per the attached.

I could not agree more. I have only succumbed to the pressure of reviewing.

Adding \dE as there are no foreign tables does not make much sense,
and also I wondered why \dt+ was not added.

Does the attached look correct to you?

You have my :+1: