> On Jun 1, 2020, at 9:59 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>
> Mark Dilger <mark.dil...@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
