Re: Include access method in listTables output

On Wed, Jul 29, 2020 at 7:30 PM Georgios <gkokolatos(at)protonmail(dot)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