Thread: Small code cleanup

One line change to remove a duplicate check.


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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,
andthat the duplication was introduced in a patch committed by others around 2018.  I was hoping that you, as the
originalauthor, or somebody involved in the 2018 patch, might have a deeper understanding of what's being done and
volunteerto 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,
andthat the duplication was introduced in a patch committed by others around 2018.  I was hoping that you, as the
originalauthor, or somebody involved in the 2018 patch, might have a deeper understanding of what's being done and
volunteerto 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
originalauthor, or somebody involved in the 2018 patch, might have a deeper understanding of what's being done and
volunteerto 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'treally focused on this particular file, or its features. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company