Re: Include access method in listTables output

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Saturday, July 25, 2020 2:41 AM, vignesh C <vignesh21(at)gmail(dot)com> wrote:

> On Thu, Jul 16, 2020 at 7:35 PM Georgios gkokolatos(at)protonmail(dot)com wrote:
>
> > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> > On Saturday, July 11, 2020 3:16 PM, vignesh C vignesh21(at)gmail(dot)com wrote:
> >
> > > On Mon, Jul 6, 2020 at 1:24 PM Georgios gkokolatos(at)protonmail(dot)com wrote:
> > >
> > > > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> > > > On Monday, July 6, 2020 3:12 AM, Michael Paquier michael(at)paquier(dot)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.

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