Fixing typos in tests of partition_info.sql
Hi Amit,
(CC: -hackers)
I was just going through some of the tests, when I noticed that the
tests of partition_info.sql have two typos and that the last set of
tests is imprecise about the expected behavior of the functions.
Do you think that something like the attached is an improvement?
Thanks,
--
Michael
On Mon, Dec 17, 2018 at 03:40:28PM +0900, Michael Paquier wrote:
I was just going through some of the tests, when I noticed that the
tests of partition_info.sql have two typos and that the last set of
tests is imprecise about the expected behavior of the functions.Do you think that something like the attached is an improvement?
Meuh. Patch forgotten.
--
Michael
Attachments:
partition-info-typos.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/test/regress/expected/partition_info.out b/src/test/regress/expected/partition_info.out
index 202d820827..6bd2d96fb6 100644
--- a/src/test/regress/expected/partition_info.out
+++ b/src/test/regress/expected/partition_info.out
@@ -99,7 +99,7 @@ SELECT relid, parentrelid, level, isleaf
(1 row)
DROP TABLE ptif_test;
--- A table not part of a partition tree works is the only member listed.
+-- A table not part of a partition tree is the only member listed.
CREATE TABLE ptif_normal_table(a int);
SELECT relid, parentrelid, level, isleaf
FROM pg_partition_tree('ptif_normal_table');
@@ -109,7 +109,8 @@ SELECT relid, parentrelid, level, isleaf
(1 row)
DROP TABLE ptif_normal_table;
--- Views and materialized viewS cannot be part of a partition tree.
+-- Views and materialized views are not part of a partition tree,
+-- causing the functions to return NULL.
CREATE VIEW ptif_test_view AS SELECT 1;
CREATE MATERIALIZED VIEW ptif_test_matview AS SELECT 1;
SELECT * FROM pg_partition_tree('ptif_test_view');
diff --git a/src/test/regress/sql/partition_info.sql b/src/test/regress/sql/partition_info.sql
index 9b55a7fe5c..0a491d295e 100644
--- a/src/test/regress/sql/partition_info.sql
+++ b/src/test/regress/sql/partition_info.sql
@@ -54,13 +54,14 @@ SELECT relid, parentrelid, level, isleaf
DROP TABLE ptif_test;
--- A table not part of a partition tree works is the only member listed.
+-- A table not part of a partition tree is the only member listed.
CREATE TABLE ptif_normal_table(a int);
SELECT relid, parentrelid, level, isleaf
FROM pg_partition_tree('ptif_normal_table');
DROP TABLE ptif_normal_table;
--- Views and materialized viewS cannot be part of a partition tree.
+-- Views and materialized views are not part of a partition tree,
+-- causing the functions to return NULL.
CREATE VIEW ptif_test_view AS SELECT 1;
CREATE MATERIALIZED VIEW ptif_test_matview AS SELECT 1;
SELECT * FROM pg_partition_tree('ptif_test_view');
Hi,
On 2018/12/17 15:40, Michael Paquier wrote:
Hi Amit,
(CC: -hackers)I was just going through some of the tests, when I noticed that the
tests of partition_info.sql have two typos and that the last set of
tests is imprecise about the expected behavior of the functions.Do you think that something like the attached is an improvement?
You forgot to attach something. :)
Thanks,
Amit
On 2018/12/17 15:52, Michael Paquier wrote:
On Mon, Dec 17, 2018 at 03:40:28PM +0900, Michael Paquier wrote:
I was just going through some of the tests, when I noticed that the
tests of partition_info.sql have two typos and that the last set of
tests is imprecise about the expected behavior of the functions.Do you think that something like the attached is an improvement?
Meuh. Patch forgotten.
Thanks.
--- A table not part of a partition tree works is the only member listed.
+-- A table not part of a partition tree is the only member listed.How about:
-- Table that is not part of any partition tree is the only member listed
--- Views and materialized viewS cannot be part of a partition tree.
+-- Views and materialized views are not part of a partition tree,
+-- causing the functions to return NULL.How bouat:
-- Function returns NULL for relation types that cannot be part of a
-- partition tree; for example, views, materialized views, etc.
Thanks,
Amit
On Mon, Dec 17, 2018 at 04:14:07PM +0900, Amit Langote wrote:
--- A table not part of a partition tree works is the only member listed. +-- A table not part of a partition tree is the only member listed.How about:
-- Table that is not part of any partition tree is the only member listed
--- Views and materialized viewS cannot be part of a partition tree. +-- Views and materialized views are not part of a partition tree, +-- causing the functions to return NULL.How about:
-- Functions returns NULL for relation types that cannot be part of a
-- partition tree; for example, views, materialized views, etc.
Your two suggestions look fine to me, so let's just reuse your wording.
i would just use the plural for the last comment with "Functions return"
as pg_partition_tree gets called multiple times, and later on
pg_partition_root will likely be added.
Perhaps there are other suggestions?
--
Michael
On 2018/12/17 16:38, Michael Paquier wrote:
On Mon, Dec 17, 2018 at 04:14:07PM +0900, Amit Langote wrote:
--- A table not part of a partition tree works is the only member listed. +-- A table not part of a partition tree is the only member listed.How about:
-- Table that is not part of any partition tree is the only member listed
--- Views and materialized viewS cannot be part of a partition tree. +-- Views and materialized views are not part of a partition tree, +-- causing the functions to return NULL.How about:
-- Functions returns NULL for relation types that cannot be part of a
-- partition tree; for example, views, materialized views, etc.Your two suggestions look fine to me, so let's just reuse your wording.
i would just use the plural for the last comment with "Functions return"
as pg_partition_tree gets called multiple times, and later on
pg_partition_root will likely be added.
Okay, let's use "Functions" then, although I wonder if you shouldn't tweak
it later when you commit the pg_partition_root patch?
Thanks,
Amit
On Mon, Dec 17, 2018 at 04:41:01PM +0900, Amit Langote wrote:
Okay, let's use "Functions" then, although I wonder if you shouldn't tweak
it later when you commit the pg_partition_root patch?
There are already two calls to pg_partition_tree for each one of the two
relkinds tested.
--
Michael
On 2018/12/17 17:25, Michael Paquier wrote:
On Mon, Dec 17, 2018 at 04:41:01PM +0900, Amit Langote wrote:
Okay, let's use "Functions" then, although I wonder if you shouldn't tweak
it later when you commit the pg_partition_root patch?There are already two calls to pg_partition_tree for each one of the two
relkinds tested.
You're saying that we should use plural "functions" because there of 2
*instances* of calling the function pg_partition_tree in the queries that
follow the comment, but I think that would be misleading. I think the
plural would make sense if we're talking about two different functions,
but I may be wrong.
Thanks,
Amit
On Mon, Dec 17, 2018 at 05:56:08PM +0900, Amit Langote wrote:
You're saying that we should use plural "functions" because there of 2
*instances* of calling the function pg_partition_tree in the queries that
follow the comment, but I think that would be misleading. I think the
plural would make sense if we're talking about two different functions,
but I may be wrong.
Or this could just use "Function calls"? My argument is just to not
forget about updating this comment later on and minimize future noise
diffs.
--
Michael
On 2018/12/17 18:10, Michael Paquier wrote:
On Mon, Dec 17, 2018 at 05:56:08PM +0900, Amit Langote wrote:
You're saying that we should use plural "functions" because there of 2
*instances* of calling the function pg_partition_tree in the queries that
follow the comment, but I think that would be misleading. I think the
plural would make sense if we're talking about two different functions,
but I may be wrong.Or this could just use "Function calls"?
As far as the information content of this comment is concerned, I think
it'd be more useful to word this comment such that it is applicable to
different functions than to word it such that it is applicable to
different queries. More opinions would be nice.
My argument is just to not
forget about updating this comment later on and minimize future noise
diffs.
Okay, how about:
-- Various partitioning-related functions return NULL if passed relations
-- of types that cannot be part of a partition tree; for example, views,
-- materialized views, etc.
Thanks,
Amit
On Mon, Dec 17, 2018 at 06:35:03PM +0900, Amit Langote wrote:
As far as the information content of this comment is concerned, I think
it'd be more useful to word this comment such that it is applicable to
different functions than to word it such that it is applicable to
different queries. More opinions would be nice.
This argument is sensible.
Okay, how about:
-- Various partitioning-related functions return NULL if passed relations
-- of types that cannot be part of a partition tree; for example, views,
-- materialized views, etc.
Okay, this suggestion sounds fine to me. Thanks!
--
Michael
On Mon, Dec 17, 2018 at 07:49:42PM +0900, Michael Paquier wrote:
Okay, this suggestion sounds fine to me. Thanks!
And committed with all your suggestions included. Thanks for the
discussion.
--
Michael
On 2018/12/18 10:57, Michael Paquier wrote:
On Mon, Dec 17, 2018 at 07:49:42PM +0900, Michael Paquier wrote:
Okay, this suggestion sounds fine to me. Thanks!
And committed with all your suggestions included. Thanks for the
discussion.
Thank you!
Regards,
Amit