Include access method in listTables output

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 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