Re: Include access method in listTables output

From: Georgios <gkokolatos(at)protonmail(dot)com>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Justin Pryzby <pryzby(at)telsasoft(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, David Rowley <dgrowleyml(at)gmail(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Include access method in listTables output
Date: 2020-09-01 10:27:31
Message-ID: nqWijaisx2TwPSOzei5ebun5AwLl1xWkjezNZn4OASe_Pg9oSp5h0mL57tsIzV01DnxHu0Tym7OvCcLA9AnlFISN9012kd_1cJA1o6NH664=@protonmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Tuesday, 1 September 2020 07:41, Michael Paquier <michael(at)paquier(dot)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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alexey Kondratov 2020-09-01 10:36:38 Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
Previous Message Anastasia Lubennikova 2020-09-01 10:25:27 Re: REINDEX SCHEMA/DATABASE/SYSTEM weak with dropped relations