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