Re: Pluggable Storage - Andres's take

> On Fri, Jan 18, 2019 at 11:22 AM Amit Khandekar <amitdkhan(dot)pg(at)gmail(dot)com> wrote:
>
> --- a/src/test/regress/expected/copy2.out
> +++ b/src/test/regress/expected/copy2.out
> @@ -1,3 +1,4 @@
> +\set HIDE_TABLEAM on
>
> I thought we wanted to avoid having to add this setting in individual
> regression tests. Can't we do this in pg_regress as a common setting ?

Yeah, you're probably right. Actually, I couldn't find anything that looks like
"common settings", and so far I've placed it into psql_start_test as a psql
argument. But not sure, maybe there is a better place.

> + /* Access method info */
> + if (pset.sversion >= 120000 && verbose && tableinfo.relam != NULL &&
> + !(pset.hide_tableam && tableinfo.relam_is_default))
> + {
> + printfPQExpBuffer(&buf, _("Access method: %s"),
> fmtId(tableinfo.relam));
>
> So this will make psql hide the access method if it's same as the
> default. I understand that this was kind of concluded in the other
> thread "Displaying and dumping of table access methods". But IMHO, if
> the hide_tableam is false, we should *always* show the access method,
> regardless of the default value. I mean, we can make it simple : off
> means never show table-access, on means always show table-access,
> regardless of the default access method. And this also will work with
> regression tests. If some regression test wants specifically to output
> the access method, it can have a "\SET HIDE_TABLEAM off" command.
>
> If we hide the method if it's default, then for a regression test that
> wants to forcibly show the table access method of all tables, it won't
> show up for tables that have default access method.

I can't imagine, what kind of test would need to forcibly show the table access
method of all the tables? Even if you need to verify tableam for something,
maybe it's even easier just to select it from pg_am?

> + if (pset.sversion >= 120000 && verbose && tableinfo.relam != NULL &&
>
> If the server does not support relam, tableinfo.relam will be NULL
> anyways. So I think sversion check is not needed.
> ------------
>
> + printfPQExpBuffer(&buf, _("Access method: %s"), fmtId(tableinfo.relam));
> fmtId is not required.
> -----------
>
> + printfPQExpBuffer(&buf, _("Access method: %s"), fmtId(tableinfo.relam));
> + printTableAddFooter(&cont, buf.data);
> + }
> +
> +
> }
>
> Last two blank lines are not needed.

Right, fixed.

> + bool hide_tableam;
> } PsqlSettings;
>
> These variables, it seems, are supposed to be grouped together by type.

Well, this grouping looks strange for me. But since I don't have a strong
opinion, I moved the variable.

> I believe you are going to add a new regression testcase for the change ?

Yep.