Small code cleanup
One line change to remove a duplicate check.
Attachments:
v1-0001-Code-cleanup.patchapplication/octet-stream; name=v1-0001-Code-cleanup.patch; x-unix-mode=0644Download
From 51277d8b2a7e4d744350995a1e6a512ff3af3213 Mon Sep 17 00:00:00 2001
From: Mark Dilger <mark.dilger@enterprisedb.com>
Date: Sun, 31 May 2020 20:43:52 -0700
Subject: [PATCH v1] Code cleanup.
Removing a duplicate check for RELKIND_PARTITIONED_INDEX.
Essentially, an
if (relkind == RELKIND_PARTITIONED_INDEX || ...)
...
else if (relkind == RELKIND_PARTITIONED_INDEX || ...)
...
statement. The duplication doesn't appear to do any harm, but I
noticed it while refactoring some code, so I'm cleaning it up.
---
src/bin/psql/describe.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index c7639f6206..58a43425f7 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2309,7 +2309,6 @@ describeOneTableDetails(const char *schemaname,
tableinfo.relkind == RELKIND_MATVIEW ||
tableinfo.relkind == RELKIND_FOREIGN_TABLE ||
tableinfo.relkind == RELKIND_PARTITIONED_TABLE ||
- tableinfo.relkind == RELKIND_PARTITIONED_INDEX ||
tableinfo.relkind == RELKIND_TOASTVALUE)
{
/* Footer information about a table */
--
2.21.1 (Apple Git-122.3)
Mark Dilger <mark.dilger@enterprisedb.com> writes:
One line change to remove a duplicate check.
The comment just above this mentions a connection to the "Finish printing
the footer information about a table" stanza below. I think some work is
needed to clarify what's going on there --- it doesn't seem actually
buggy, but there are multiple lies embedded in these comments. I'm also
questioning somebody's decision to wedge partitioning into this logic
without refactoring any existing if's, as they seem to have done. At the
very least we're issuing useless queries here, for instance looking for
inheritance parents of matviews.
regards, tom lane
On Jun 1, 2020, at 8:53 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Mark Dilger <mark.dilger@enterprisedb.com> writes:
One line change to remove a duplicate check.
The comment just above this mentions a connection to the "Finish printing
the footer information about a table" stanza below. I think some work is
needed to clarify what's going on there --- it doesn't seem actually
buggy, but there are multiple lies embedded in these comments. I'm also
questioning somebody's decision to wedge partitioning into this logic
without refactoring any existing if's, as they seem to have done. At the
very least we're issuing useless queries here, for instance looking for
inheritance parents of matviews.
Yeah, I noticed the `git blame` last night when writing the patch that you had originally wrote the code around 2017, and that the duplication was introduced in a patch committed by others around 2018. I was hoping that you, as the original author, or somebody involved in the 2018 patch, might have a deeper understanding of what's being done and volunteer to clean up the comments.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Mark Dilger <mark.dilger@enterprisedb.com> writes:
Yeah, I noticed the `git blame` last night when writing the patch that you had originally wrote the code around 2017, and that the duplication was introduced in a patch committed by others around 2018. I was hoping that you, as the original author, or somebody involved in the 2018 patch, might have a deeper understanding of what's being done and volunteer to clean up the comments.
I don't think there's any deep dark mystery here. We have a collection of
things we need to do, each one applying to some subset of relkinds, and
the issue is how to express the control logic in a maintainable and
not-too-confusing way. Unfortunately people have pasted in new things
with little focus on "not too confusing" and more focus on "how can I make
this individual patch as short as possible". It's probably time to take a
step back and refactor.
My immediate annoyance was that the "Finish printing the footer
information about a table" comment has been made a lie by adding
partitioned indexes to the set of relkinds handled; I can cope with
considering a matview to be a table, but surely an index is not. Plus, if
partitioned indexes need to be handled here, why not also regular indexes?
The lack of any comments explaining this is really not good.
I'm inclined to think that maybe having that overall if-test just after
that comment is obsolete, and we ought to break it down into separate
segments. For instance there's no obvious reason why the first
"print foreign server name" stanza should be inside that if-test;
and the sections related to partitioning would be better off restricted
to relkinds that, um, can have partitions.
I have to admit that I don't any longer see what the connection is
between the two "footer information about a table" sections. Maybe
it was more obvious before all the partitioning stuff got shoved in,
or maybe there never was any essential connection.
Anyway the whole thing is overdue for a cosmetic workover. Do you
want to have a go at that?
regards, tom lane
On Jun 1, 2020, at 9:59 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Mark Dilger <mark.dilger@enterprisedb.com> writes:
Yeah, I noticed the `git blame` last night when writing the patch that you had originally wrote the code around 2017, and that the duplication was introduced in a patch committed by others around 2018. I was hoping that you, as the original author, or somebody involved in the 2018 patch, might have a deeper understanding of what's being done and volunteer to clean up the comments.
I don't think there's any deep dark mystery here. We have a collection of
things we need to do, each one applying to some subset of relkinds, and
the issue is how to express the control logic in a maintainable and
not-too-confusing way. Unfortunately people have pasted in new things
with little focus on "not too confusing" and more focus on "how can I make
this individual patch as short as possible". It's probably time to take a
step back and refactor.My immediate annoyance was that the "Finish printing the footer
information about a table" comment has been made a lie by adding
partitioned indexes to the set of relkinds handled; I can cope with
considering a matview to be a table, but surely an index is not. Plus, if
partitioned indexes need to be handled here, why not also regular indexes?
The lack of any comments explaining this is really not good.I'm inclined to think that maybe having that overall if-test just after
that comment is obsolete, and we ought to break it down into separate
segments. For instance there's no obvious reason why the first
"print foreign server name" stanza should be inside that if-test;
and the sections related to partitioning would be better off restricted
to relkinds that, um, can have partitions.I have to admit that I don't any longer see what the connection is
between the two "footer information about a table" sections. Maybe
it was more obvious before all the partitioning stuff got shoved in,
or maybe there never was any essential connection.Anyway the whole thing is overdue for a cosmetic workover. Do you
want to have a go at that?
Ok, sure, I'll see if I can clean that up. I ran into this while doing some refactoring of about 160 files, so I wasn't really focused on this particular file, or its features.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company