Re: Include access method in listTables output

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, June 15, 2020 3:20 AM, vignesh C <vignesh21(at)gmail(dot)com> wrote:

> On Tue, Jun 9, 2020 at 6:45 PM Georgios gkokolatos(at)protonmail(dot)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.

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