Re: reorganizing partitioning code

From: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
To: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
Cc: alvherre(at)alvh(dot)no-ip(dot)org, pgsql-hackers(at)postgresql(dot)org
Subject: Re: reorganizing partitioning code
Date: 2018-02-16 08:36:58
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


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]

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!


> ==== 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.


> ==== 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.


Attachment Content-Type Size
v4-0001-Reorganize-partitioning-code.patch text/plain 140.9 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Masahiko Sawada 2018-02-16 08:42:34 Re: autovacuum: change priority of the vacuumed tables
Previous Message tushar 2018-02-16 08:14:05 Server crash in pg_replication_slot_advance function