Обсуждение: Include access method in listTables output
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
Вложения
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
‐‐‐‐‐‐‐ 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
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
‐‐‐‐‐‐‐ 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 >
Вложения
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
‐‐‐‐‐‐‐ 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
Вложения
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
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
Вложения
‐‐‐‐‐‐‐ 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
Вложения
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
‐‐‐‐‐‐‐ 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
Вложения
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
‐‐‐‐‐‐‐ 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
>
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
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
Вложения
‐‐‐‐‐‐‐ 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
Вложения
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
Вложения
‐‐‐‐‐‐‐ 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
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