Re: [ psql - review request ] review request for \d+ tablename, \d+ indexname indenting

[vignesh C]

On Wed, 22 Nov 2023 at 21:54, Daniel Verite  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,
> peaktempint,
> 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

Re: [ psql - review request ] review request for \d+ tablename, \d+ indexname indenting

[Daniel Verite]

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,
peaktempint,
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

Re: [ psql - review request ] review request for \d+ tablename, \d+ indexname indenting

[Shlok Kyal]

Hi,

On Mon, 6 Nov 2023 at 13:47, 쿼리트릭스  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

Re: [ psql - review request ] review request for \d+ tablename, \d+ indexname indenting

[쿼리트릭스]

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.

Team Query Tricks
---
querytricks2023.gmail.com
Query Tricks(https://github.com/Query-Tricks)

2023년 9월 13일 (수) 오후 10:48, Peter Eisentraut 님이 작성:

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


psql-slashDplus-partition-indentation.diff
Description: Binary data

Re: [ psql - review request ] review request for \d+ tablename, \d+ indexname indenting

[Peter Eisentraut]

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.

Re: [ psql - review request ] review request for \d+ tablename, \d+ indexname indenting

[쿼리트릭스]

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.
Considering your question, we think it is enough to assume just one
subpartition level.
Because, Concidering the common partition configuration methods, we think
it is  rare case to configure subpartitions contains subpartitions.
So, we think it would be appropriate to mark up to level 1 of the
subpartition when using \d+.
If there subpartitions contains subpartitions, the keyword 'CONTAINS
SUBPARTITIONS' is added next to the partition name to indicate that the
subpartitions contains subpartitions exists.
These sources were tested on 14.5, 15.2 and 16 RC versions, respectively.
If you have any other opinions on this, please let us know. we will
actively consider it.

Team Query Tricks
---
querytricks2023.gmail.com
Query Tricks (github.com) 



2023년 8월 26일 (토) 오전 6:01, Cary Huang 님이 작성:

> 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 and describe_change.c. You should use git diff to
> generate a patch rather than maintaining 2 files yourself. Also I noticed
> that you include a "create_object.sql" file to illustrate the feature,
> which is not necessary. Instead, you should add them as a regression test
> cases in the existing regression test suite under "src/test/regress", so
> these will get run as tests to illustrate the feature. This patch changes
> the output of \d+ and it could potentially break other test cases 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 in my opinion. However,
> a sub-partition can also contain another sub-partition, which contains
> another sub-partition and so on. 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 that it starts to become annoying. Are you
> planning to set a limit on how many levels of sub-partitions to print or
> just let it print as many as it needs?
>
> thank you
>
> Cary Huang
> ---
> Highgo Software Canada
> www.highgo.ca


subpartition_indentation.diff
Description: Binary data

Re: [ psql - review request ] review request for \d+ tablename, \d+ indexname indenting

[Cary Huang]

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 
and describe_change.c. You should use git diff to generate a patch rather than 
maintaining 2 files yourself. Also I noticed that you include a 
"create_object.sql" file to illustrate the feature, which is not necessary. 
Instead, you should add them as a regression test cases in the existing 
regression test suite under "src/test/regress", so these will get run as tests 
to illustrate the feature. This patch changes the output of \d+ and it could 
potentially break other test cases 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 in my opinion. However, a sub-partition 
can also contain another sub-partition, which contains another sub-partition 
and so on. 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 that it 
starts to become annoying. Are you planning to set a limit on how many levels 
of sub-partitions to print or just let it print as many as it needs?

thank you

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