Обсуждение: [ psql - review request ] review request for \d+ tablename, \d+ indexname indenting

The following review has been posted through the commitfest application:
make installcheck-world:  tested, failed
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            not tested

Hello

Thank you for the patch and the effort to enhance \d+ 's output on partitioned tables that contain sub-partitions.
However,the patch does not apply and I notice that this patch is generated as a differ file from 2 files, describe.c
anddescribe_change.c. You should use git diff to generate a patch rather than maintaining 2 files yourself. Also I
noticedthat you include a "create_object.sql" file to illustrate the feature, which is not necessary. Instead, you
shouldadd them as a regression test cases in the existing regression test suite under "src/test/regress", so these will
getrun as tests to illustrate the feature. This patch changes the output of \d+ and it could potentially break other
testcases so you should fix them in the patch in addition to providing the feature
 

Now, regarding the feature, I see that you intent to print the sub partitions' partitions in the output, which is okay
inmy opinion. However, a sub-partition can also contain another sub-partition, which contains another sub-partition and
soon. So it is possible that sub-partitions can span very, very deep. Your example assumes only 1 level of
sub-partitions.Are you going to print all of them out in \d+? If so, it would definitely cluster the output so much
thatit starts to become annoying. Are you planning to set a limit on how many levels of sub-partitions to print or just
letit print as many as it needs?
 

thank you

Cary Huang
-----------------------
Highgo Software Canada
www.highgo.ca

On 12.09.23 09:27, 쿼리트릭스 wrote:
> Thank you for letting me know more about the test method.
> As you said, we applied the patch using git diff and created a test case 
> on the src/test/regress/sql.

Because of the change of the psql output, a lot of existing test cases 
are now failing.  You should run "make check" and fix up the failures. 
Also, your new test file "subpartition_indentation" isn't actually run 
because it was not added to src/test/regress/parallel_schedule.  I 
suspect you probably don't want to add a new test file for this but 
instead see if the existing tests cover this case.

Hi,

On Mon, 6 Nov 2023 at 13:47, 쿼리트릭스 <querytricks2023@gmail.com> wrote:
>
> The error was corrected and a new diff file was created.
> The diff file was created based on 16 RC1.
> We confirmed that 5 places where errors occurred when performing make check were changed to ok.
>

I went through Cfbot and still see that some tests are failing.
links:
https://cirrus-ci.com/task/6408253983162368
https://cirrus-ci.com/task/5000879099609088
https://cirrus-ci.com/task/6126779006451712
https://cirrus-ci.com/task/5563829053030400
https://cirrus-ci.com/task/6689728959873024

Failure:
[16:42:37.674] Summary of Failures:
[16:42:37.674]
[16:42:37.674] 5/270 postgresql:regress / regress/regress ERROR 28.88s
exit status 1
[16:42:37.674] 7/270 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade
ERROR 46.73s exit status 1
[16:42:37.674] 56/270 postgresql:recovery /
recovery/027_stream_regress ERROR 38.51s exit status 1

Thanks
Shlok Kumar Kyal

    Shlok Kyal wrote:

> > The error was corrected and a new diff file was created.
> > The diff file was created based on 16 RC1.
> > We confirmed that 5 places where errors occurred when performing
> > make check were changed to ok.

Reviewing the patch, I see these two problems in the current version
(File: psql-slashDplus-partition-indentation.diff, Date: 2023-09-19 00:19:34)

* There are changes in the regression tests that do not concern this
feature and should not be there.

For instance this hunk:

--- a/src/test/regress/expected/foreign_data.out
+++ b/src/test/regress/expected/foreign_data.out
@@ -742,8 +742,6 @@ COMMENT ON COLUMN ft1.c1 IS 'ft1.c1';
 Check constraints:
     "ft1_c2_check" CHECK (c2 <> ''::text)
     "ft1_c3_check" CHECK (c3 >= '01-01-1994'::date AND c3 <=
'01-31-1994'::date)
-Not-null constraints:
-    "ft1_c1_not_null" NOT NULL "c1"
 Server: s0
 FDW options: (delimiter ',', quote '"', "be quoted" 'value')

It seems to undo a test for a recent feature adding "Not-null
constraints" to \d, which suggests that you've been running tests
against and older version than the source tree you're diffing
against. These should be the same version, and also the latest
one (git HEAD) or as close as possible to the latest when the
patch is submitted.

* The new query with \d on partitioned tables does not work with
  Postgres servers 12 or 13:


postgres=# CREATE TABLE measurement (
    city_id        int not null,
    logdate        date not null,
    peaktemp        int,
    unitsales        int
) PARTITION BY RANGE (logdate);

postgres=# \d measurement
ERROR:    syntax error at or near "."
LINE 2: ...  0 AS level,    c.relkind,       false AS i.inhdetach...


Setting the CommitFest status to WoA.


Best regards,
--
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite

On Wed, 22 Nov 2023 at 21:54, Daniel Verite <daniel@manitou-mail.org> wrote:
>
>         Shlok Kyal wrote:
>
> > > The error was corrected and a new diff file was created.
> > > The diff file was created based on 16 RC1.
> > > We confirmed that 5 places where errors occurred when performing
> > > make check were changed to ok.
>
> Reviewing the patch, I see these two problems in the current version
> (File: psql-slashDplus-partition-indentation.diff, Date: 2023-09-19 00:19:34)
>
> * There are changes in the regression tests that do not concern this
> feature and should not be there.
>
> For instance this hunk:
>
> --- a/src/test/regress/expected/foreign_data.out
> +++ b/src/test/regress/expected/foreign_data.out
> @@ -742,8 +742,6 @@ COMMENT ON COLUMN ft1.c1 IS 'ft1.c1';
>  Check constraints:
>      "ft1_c2_check" CHECK (c2 <> ''::text)
>      "ft1_c3_check" CHECK (c3 >= '01-01-1994'::date AND c3 <=
> '01-31-1994'::date)
> -Not-null constraints:
> -    "ft1_c1_not_null" NOT NULL "c1"
>  Server: s0
>  FDW options: (delimiter ',', quote '"', "be quoted" 'value')
>
> It seems to undo a test for a recent feature adding "Not-null
> constraints" to \d, which suggests that you've been running tests
> against and older version than the source tree you're diffing
> against. These should be the same version, and also the latest
> one (git HEAD) or as close as possible to the latest when the
> patch is submitted.
>
> * The new query with \d on partitioned tables does not work with
>   Postgres servers 12 or 13:
>
>
> postgres=# CREATE TABLE measurement (
>     city_id         int not null,
>     logdate         date not null,
>     peaktemp        int,
>     unitsales       int
> ) PARTITION BY RANGE (logdate);
>
> postgres=# \d measurement
> ERROR:  syntax error at or near "."
> LINE 2: ...  0 AS level,        c.relkind,         false AS i.inhdetach...
>
>
> Setting the CommitFest status to WoA.

I have changed the status of the CommitFest entry to "Returned with
Feedback" as Shlok's and Daniel's suggestions are not handled. Feel
free to address them and add a new commitfest entry for the same.

Regards,
Vignesh