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

Hi, We are Query Tricks.
We are a project team created to provide better usability for PostgreSQL
DBAs and users.
and I'm Hyunhee Ryu, a member of the project team.

There is something I would like you to consider introducing in a new
version of the release.
This is related to \d+ table_name and \d+ index_name in psql, especially
related to lookup lists in partition tables.
We conducted the test based on PostgreSQL 14, 15 version.

The existing partition table list is printed in this format.
-- Current Partition Table List
postgres=# \d+ p_quarter_check
Partitioned table
"public.p_quarter_check"
Column | Type | Collation | Nullable | Default | Storage
| Compression | Stats target | Description
--------+-----------------------+-----------+----------+---------+----------+-------------+--------------+-------------
id | integer | | not null | | plain
| | |
dept | character varying(10) | | | | extended
| | |
name | character varying(20) | | | | extended
| | |
in_d | date | | not null | | plain
| | |
etc | text | | | | extended
| | |
Partition key: RANGE (in_d)
Indexes:
"parent_idx01" btree (id)
Partitions: in_p_q1 FOR VALUES FROM ('2023-01-01') TO ('2023-04-01'),
PARTITIONED,
in_p_q2 FOR VALUES FROM ('2023-04-01') TO ('2023-07-01'),
PARTITIONED,
in_p_q3 FOR VALUES FROM ('2023-07-01') TO ('2023-10-01'),
PARTITIONED,
in_p_q4 FOR VALUES FROM ('2023-10-01') TO ('2024-01-01'),
PARTITIONED

It doesn't matter in the normal partition structure, but I felt
uncomfortable looking up the list when there were additional subpartitions.
So to improve this inconvenience, I wrote an SQL query to query the
partition table and partition index in the format below when querying the
partition table and partition index in psql.

-- After Patch Partition Table List
postgres=# \d+ p_quarter_check
Partitioned table
"public.p_quarter_check"
Column | Type | Collation | Nullable | Default | Storage
| Compression | Stats target | Description
--------+-----------------------+-----------+----------+---------+----------+-------------+--------------+-------------
id | integer | | not null | | plain
| | |
dept | character varying(10) | | | | extended
| | |
name | character varying(20) | | | | extended
| | |
in_d | date | | not null | | plain
| | |
etc | text | | | | extended
| | |
Partition key: RANGE (in_d)
Indexes:
"parent_idx01" btree (id)
Partitions: in_p_q1 FOR VALUES FROM ('2023-01-01') TO ('2023-04-01'),
PARTITIONED,
in_p_y202301 FOR VALUES FROM ('2023-01-01') TO
('2023-02-01'),
in_p_y202302 FOR VALUES FROM ('2023-02-01') TO
('2023-03-01'),
in_p_y202303 FOR VALUES FROM ('2023-03-01') TO
('2023-04-01'),
in_p_q2 FOR VALUES FROM ('2023-04-01') TO ('2023-07-01'),
PARTITIONED,
in_p_y202304 FOR VALUES FROM ('2023-04-01') TO
('2023-05-01'),
in_p_y202305 FOR VALUES FROM ('2023-05-01') TO
('2023-06-01'),
in_p_y202306 FOR VALUES FROM ('2023-06-01') TO
('2023-07-01'),
in_p_q3 FOR VALUES FROM ('2023-07-01') TO ('2023-10-01'),
PARTITIONED,
in_p_y202307 FOR VALUES FROM ('2023-07-01') TO
('2023-08-01'),
in_p_y202308 FOR VALUES FROM ('2023-08-01') TO
('2023-09-01'),
in_p_y202309 FOR VALUES FROM ('2023-09-01') TO
('2023-10-01'),
in_p_q4 FOR VALUES FROM ('2023-10-01') TO ('2024-01-01'),
PARTITIONED,
in_p_y202310 FOR VALUES FROM ('2023-10-01') TO
('2023-11-01'),
in_p_y202311 FOR VALUES FROM ('2023-11-01') TO
('2023-12-01'),
in_p_y202312 FOR VALUES FROM ('2023-12-01') TO
('2024-01-01')

Partition Index also wrote the SQL syntax so that you can look up the list
with an intuitive structure.
--Current Partition Index
postgres=# \d+ parent_idx01
Partitioned index "public.parent_idx01"
Column | Type | Key? | Definition | Storage | Stats target
--------+---------+------+------------+---------+--------------
id | integer | yes | id | plain |
btree, for table "public.p_quarter_check"
Partitions: in_p_q1_id_idx, PARTITIONED,
in_p_q2_id_idx, PARTITIONED,
in_p_q3_id_idx, PARTITIONED,
in_p_q4_id_idx, PARTITIONED
Access method: btree

-- After Patch Partition Index
postgres=# \d+ parent_idx01
Partitioned index "public.parent_idx01"
Column | Type | Key? | Definition | Storage | Stats target
--------+---------+------+------------+---------+--------------
id | integer | yes | id | plain |
btree, for table "public.p_quarter_check"
Partitions: in_p_q1_id_idx, PARTITIONED,
in_p_y202301_id_idx,
in_p_y202302_id_idx,
in_p_y202303_id_idx,
in_p_q2_id_idx, PARTITIONED,
in_p_y202304_id_idx,
in_p_y202305_id_idx,
in_p_y202306_id_idx,
in_p_q3_id_idx, PARTITIONED,
in_p_y202307_id_idx,
in_p_y202308_id_idx,
in_p_y202309_id_idx,
in_p_q4_id_idx, PARTITIONED,
in_p_y202310_id_idx,
in_p_y202311_id_idx,
in_p_y202312_id_idx
Access method: btree

I attached the queries used to create the partition and the queries I wrote
to look up the list to the mail.
This is the patch applied to line 3370 of the 'describe.c' source file.
Based on this SQL syntax and patch file, I would like you to review the
query \d+ Partition_table_name and \d+ Partition_index_name so that the SQL
is reflected.

If you are not asking for a review in this way, please let me know how to
proceed.
Please give me a positive answer and I will wait for your feedback.
Have a nice day.

From Query Tricks / Hyunhee Ryu.

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

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) <https://github.com/Query-Tricks>

2023년 8월 26일 (토) 오전 6:01, Cary Huang <cary(dot)huang(at)highgo(dot)ca>님이 작성:

> 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

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.

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 <peter(at)eisentraut(dot)org>님이 작성:

> 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(at)gmail(dot)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(at)manitou-mail(dot)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