Thread: Include access method in listTables output

Include access method in listTables output

From
Georgios
Date:
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


Attachment

Re: Include access method in listTables output

From
David Rowley
Date:
On Tue, 9 Jun 2020 at 23:03, Georgios <gkokolatos@protonmail.com> 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



Re: Include access method in listTables output

From
Georgios
Date:

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Tuesday, June 9, 2020 1:34 PM, David Rowley <dgrowleyml@gmail.com> wrote:

> On Tue, 9 Jun 2020 at 23:03, Georgios gkokolatos@protonmail.com 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





Re: Include access method in listTables output

From
vignesh C
Date:
On Tue, Jun 9, 2020 at 6:45 PM Georgios <gkokolatos@protonmail.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.

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



Re: Include access method in listTables output

From
Georgios
Date:

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, June 15, 2020 3:20 AM, vignesh C <vignesh21@gmail.com> wrote:

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


Attachment

Re: Include access method in listTables output

From
vignesh C
Date:
On Tue, Jun 16, 2020 at 6:13 PM Georgios <gkokolatos@protonmail.com> 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



Re: Include access method in listTables output

From
Georgios
Date:

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Saturday, June 20, 2020 3:15 PM, vignesh C <vignesh21@gmail.com> wrote:

> On Tue, Jun 16, 2020 at 6:13 PM Georgios gkokolatos@protonmail.com 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


Attachment

Re: Include access method in listTables output

From
vignesh C
Date:
On Tue, Jun 30, 2020 at 2:53 PM Georgios <gkokolatos@protonmail.com> wrote:
>
>
> 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.
>

Patch applies cleanly, make check & make check-world passes.
One comment:
+               if (pset.sversion >= 120000 && !pset.hide_tableam &&
+                       (showTables || showViews || showMatViews ||
showIndexes))
+                       appendPQExpBuffer(&buf,
+                                                         ",\n
am.amname as \"%s\"",
+
gettext_noop("Access Method"));

I'm not sure if we should include showViews, I had seen that the
access method was not getting selected for view.

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



Re: Include access method in listTables output

From
Michael Paquier
Date:
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().
--
Michael

Attachment

Re: Include access method in listTables output

From
Georgios
Date:

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Monday, July 6, 2020 3:12 AM, Michael Paquier <michael@paquier.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.

>
> ---------------------------------------------------------------------------------------------------------------
>
> Michael


Attachment

Re: Include access method in listTables output

From
vignesh C
Date:
On Mon, Jul 6, 2020 at 1:24 PM Georgios <gkokolatos@protonmail.com> wrote:
>
>
>
> ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> On Monday, July 6, 2020 3:12 AM, Michael Paquier <michael@paquier.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?


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



Re: Include access method in listTables output

From
Georgios
Date:

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Saturday, July 11, 2020 3:16 PM, vignesh C <vignesh21@gmail.com> wrote:

> On Mon, Jul 6, 2020 at 1:24 PM Georgios gkokolatos@protonmail.com wrote:
>
> > ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
> > On Monday, July 6, 2020 3:12 AM, Michael Paquier michael@paquier.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.

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


Attachment

Re: Include access method in listTables output

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

 \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?

+-- 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.

+ 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?

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



Re: Include access method in listTables output

From
Georgios
Date:

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Saturday, July 25, 2020 2:41 AM, vignesh C <vignesh21@gmail.com> wrote:

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





Re: Include access method in listTables output

From
vignesh C
Date:
On Wed, Jul 29, 2020 at 7:30 PM Georgios <gkokolatos@protonmail.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



Re: Include access method in listTables output

From
vignesh C
Date:
On Sat, Aug 1, 2020 at 8:12 AM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > +-- 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.

I felt adding one test for view is good and added it.
Attached a new patch for the same.

I felt patch is in shape for committer to have a look at this.

Regards,
Vignesh

Attachment

Re: Include access method in listTables output

From
Georgios
Date:
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Thursday, 20 August 2020 07:31, Justin Pryzby <pryzby@telsasoft.com> wrote:

> On Mon, Aug 17, 2020 at 11:10:05PM +0530, vignesh C wrote:
>
> > On Sat, Aug 1, 2020 at 8:12 AM vignesh C vignesh21@gmail.com wrote:
> >
> > > > > +-- 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.
> > > > >
> >
> > I felt adding one test for view is good and added it.
> > Attached a new patch for the same.
> > I felt patch is in shape for committer to have a look at this.
>
> The patch tester shows it's failing xmllint ; could you send a fixed patch ?
>
> /usr/bin/xmllint --path . --noout --valid postgres.sgml
> ref/psql-ref.sgml:1189: parser error : Opening and ending tag mismatch: link line 1187 and para
>
> http://cfbot.cputube.org/georgios-kokolatos.html

Thank you for pointing it out!

Please find version 7 attached which hopefully addresses the error along with a proper
expansion of the test coverage and removal of recently introduced
whitespace warnings.

>
>
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>
> Justin


Attachment

Re: Include access method in listTables output

From
Michael Paquier
Date:
On Thu, Aug 20, 2020 at 08:16:19AM +0000, Georgios wrote:
> Please find version 7 attached which hopefully addresses the error along with a proper
> expansion of the test coverage and removal of recently introduced
> whitespace warnings.

+CREATE ROLE    conditional_tableam_display_role;
As a convention, regression tests need to have roles prefixed with
"regress_" or this would cause some buildfarm members to turn red.
Please see -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS (you could use
that in your environment for example).

So, as of the tests..  The role gets added to make sure that when
using \d+ on the full schema as well as the various \d*+ variants we
have a consistent owner.  The addition of the relation size for the
sequence and the btree index in the output generated is a problem
though, because that's not really portable when compiling with other
page sizes.  It is true that there are other tests failing in this
case, but I think that we should try to limit that if we can.  In
short, I agree that having some tests is better than nothing, but I
would suggest to reduce their scope, as per the attached.

Adding \dE as there are no foreign tables does not make much sense,
and also I wondered why \dt+ was not added.

Does the attached look correct to you?
--
Michael

Attachment

Re: Include access method in listTables output

From
Georgios
Date:

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Tuesday, 1 September 2020 07:41, Michael Paquier <michael@paquier.xyz> wrote:

> On Thu, Aug 20, 2020 at 08:16:19AM +0000, Georgios wrote:
>
> > Please find version 7 attached which hopefully addresses the error along with a proper
> > expansion of the test coverage and removal of recently introduced
> > whitespace warnings.
>
> +CREATE ROLE conditional_tableam_display_role;
> As a convention, regression tests need to have roles prefixed with
> "regress_" or this would cause some buildfarm members to turn red.
> Please see -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS (you could use
> that in your environment for example).


I was wondering about the name. I hoped that it would either come up during review, or that it would be fine.

Thank you for the detailed explanation.

>
> So, as of the tests.. The role gets added to make sure that when
> using \d+ on the full schema as well as the various \d*+ variants we
> have a consistent owner. The addition of the relation size for the
> sequence and the btree index in the output generated is a problem
> though, because that's not really portable when compiling with other
> page sizes. It is true that there are other tests failing in this
> case, but I think that we should try to limit that if we can. In
> short, I agree that having some tests is better than nothing, but I
> would suggest to reduce their scope, as per the attached.

I could not agree more. I have only succumbed to the pressure of reviewing.

>
> Adding \dE as there are no foreign tables does not make much sense,
> and also I wondered why \dt+ was not added.
>
> Does the attached look correct to you?

You have my :+1:

>
>
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>
> Michael





Re: Include access method in listTables output

From
Michael Paquier
Date:
On Tue, Sep 01, 2020 at 10:27:31AM +0000, Georgios wrote:
> On Tuesday, 1 September 2020 07:41, Michael Paquier <michael@paquier.xyz> wrote:
>> Adding \dE as there are no foreign tables does not make much sense,
>> and also I wondered why \dt+ was not added.
>>
>> Does the attached look correct to you?
>
> You have my :+1:

OK, applied then.
--
Michael

Attachment