Include access method in listTables output

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

Include access method in listTables output

Georgios
Hi,

Postgres 12 introduced TableAm api. Although as far as I can see, currently only heap is
included as access method, it is fair to imagine that users will start adding their own methods
and more methods to be included in Postgres core.

With that in mind, it might be desirable for a user to see the access method when describing
in verbose mode, eg. `\d+`.

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.

Cheers,
//Georgios



0001-Include-AM-in-listTables-output.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Include access method in listTables output

David Rowley
On Tue, 9 Jun 2020 at 23:03, Georgios <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Include access method in listTables output

Georgios


‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Tuesday, June 9, 2020 1:34 PM, David Rowley <[hidden email]> wrote:

> On Tue, 9 Jun 2020 at 23:03, Georgios [hidden email] 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.

>
> David




Reply | Threaded
Open this post in threaded view
|

Re: Include access method in listTables output

vignesh C
On Tue, Jun 9, 2020 at 6:45 PM Georgios <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Include access method in listTables output

Georgios


‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, June 15, 2020 3:20 AM, vignesh C <[hidden email]> wrote:

> On Tue, Jun 9, 2020 at 6:45 PM Georgios [hidden email] 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.

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


0001-Include-AM-in-listTables-output-v2.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Include access method in listTables output

vignesh C
On Tue, Jun 16, 2020 at 6:13 PM Georgios <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Include access method in listTables output

Georgios


‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Saturday, June 20, 2020 3:15 PM, vignesh C <[hidden email]> wrote:

> On Tue, Jun 16, 2020 at 6:13 PM Georgios [hidden email] 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.

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


0001-Include-AM-in-listTables-output-v3.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Include access method in listTables output

vignesh C
On Tue, Jun 30, 2020 at 2:53 PM Georgios <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Include access method in listTables output

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Include access method in listTables output

Georgios


‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, July 6, 2020 3:12 AM, Michael Paquier <[hidden email]> 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.

>
> ---------------------------------------------------------------------------------------------------------------
>
> Michael


0001-Include-AM-in-listTables-output-v4.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Include access method in listTables output

vignesh C
On Mon, Jul 6, 2020 at 1:24 PM Georgios <[hidden email]> wrote:

>
>
>
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Monday, July 6, 2020 3:12 AM, Michael Paquier <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Include access method in listTables output

Georgios


‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Saturday, July 11, 2020 3:16 PM, vignesh C <[hidden email]> wrote:

> On Mon, Jul 6, 2020 at 1:24 PM Georgios [hidden email] wrote:
>
> > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> > On Monday, July 6, 2020 3:12 AM, Michael Paquier [hidden email] 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.

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


0001-Include-AM-in-listTables-output-v5.patch (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Include access method in listTables output

vignesh C
On Thu, Jul 16, 2020 at 7:35 PM Georgios <[hidden email]> wrote:

>
>
>
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Saturday, July 11, 2020 3:16 PM, vignesh C <[hidden email]> wrote:
>
> > On Mon, Jul 6, 2020 at 1:24 PM Georgios [hidden email] wrote:
> >
> > > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> > > On Monday, July 6, 2020 3:12 AM, Michael Paquier [hidden email] 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


Reply | Threaded
Open this post in threaded view
|

Re: Include access method in listTables output

Georgios


‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Saturday, July 25, 2020 2:41 AM, vignesh C <[hidden email]> wrote:

> On Thu, Jul 16, 2020 at 7:35 PM Georgios [hidden email] wrote:
>
> > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> > On Saturday, July 11, 2020 3:16 PM, vignesh C [hidden email] wrote:
> >
> > > On Mon, Jul 6, 2020 at 1:24 PM Georgios [hidden email] wrote:
> > >
> > > > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> > > > On Monday, July 6, 2020 3:12 AM, Michael Paquier [hidden email] 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.


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




Reply | Threaded
Open this post in threaded view
|

Re: Include access method in listTables output

vignesh C
On Wed, Jul 29, 2020 at 7:30 PM Georgios <[hidden email]> 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