Re: reorganizing partitioning code

Horiguchi-san,

On 2018/02/16 14:07, Kyotaro HORIGUCHI wrote:
> Hello. I'd like to make a humble comment.

Thanks a lot for taking a look.

> The reorganization adds/removes header files to/from .[ch]
> files. That can easily leave useless includes and they're hardly
> noticed. Following files touched in this patch have such useless
> includes after this patch.

Yes, I agree.

> On the other hand, naive decision of this kind of cleanup can
> lead to curruption. [1] So, I don't insisit that the all of the
> following *should* amended, especially for rel.h.
>
> [1] https://www.postgresql.org/message-id/6748.1518711125@sss.pgh.pa.us

I was initially trying limit this patch's #include churn so that it only
touches .[ch] files that are somehow involved with partitioning, but ended
up with a lot of files touched. To prevent confusion and concerns, I have
updated the patch to keep the churn to minimum as best as I could.

So, while the v3 patch looked like this:

24 files changed, 2373 insertions(+), 2312 deletions(-)
create mode 100644 src/backend/utils/cache/partcache.c
create mode 100644 src/include/utils/partcache.h

v4 looks like this:

15 files changed, 2314 insertions(+), 2263 deletions(-)
create mode 100644 src/backend/utils/cache/partcache.c
create mode 100644 src/include/utils/partcache.h

> ==== nodeModifyTable.c:
>> +#include "rewrite/rewriteManip.h"
>
> rewriteManip.h is changed to include rel.h so rel.h is no longer
> need to be included in the file. (It is also included in lmgr.h
> so it was needless since before this patch, though.)

On second thought, I decided to hold back on moving
map_partition_varattnos() to rewriteManip.c, because its interface is
unlike any other functions in that file, requiring us to include rel.h in
rewriteManip.h. So, I removed this #include from nodeModifyTable.c.

> ==== hba.c:
>> +#include "catalog/objectaddress.h"
>
> objectaddress.h includes acl.h so acl.h is no longer useful.

This and a few others were necessitated by removing partition.h from
executor.h. For now, I'm putting partition.h back into executor.h,
although it would be nice to remove it eventually.

> ==== joinrels.c:
>> +#include "utils/partcache.h"
>
> partcache.h includes lsyscache.h.

Moved lsyscache.h from partcache.h to partcache.c.

> ==== prepunion.c:
>> +#include "utils/partcache.h"
>
> partcache.h includes lsyscache.h and partcache.h is included in
> rel.h. So partcache.h and lsyscache.h are not required.

Oops, why did I even include partcache.h in prepunion.c!

Removed.

> ==== utility.c:
>> +#include "utils/rel.h"
>
> rel.h includes xlog.h (and xlog.h includes fd.h). The last two
> are removable.

I've reverted the change that necessitated this and so this one.

> ==== partcache.c:
> parsenodes.h is included from some other files.
> rel.h is included from rewriteManip.h.
> partcache.h is included from rel.h
> As the result, parsenodes.h, rel.h, partcache.h are not required.

Removed.

> ==== relcache.c:
>> +#include "utils/partcache.h"
>
> lsyscache.h is included by partcache.h.

lsyscache.h was moved from partcache.h per above, so keeping here.

> ==== rel.h:
>> +#include "utils/partcache.h"
>
> partcache.h includes fmgr.h and relcache.h so the last two are
> no longer useful.

Removed both.

Attached updated version.

Thanks,
Amit