Reducing header interdependencies around heapam.h et al.

From: Andres Freund <andres(at)anarazel(dot)de>
To: pgsql-hackers(at)postgresql(dot)org, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
Subject: Reducing header interdependencies around heapam.h et al.
Date: 2019-01-14 00:07:01
Message-ID: 20190114000701.y4ttcb74jpskkcfb@alap3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

While working on pluggable storage (in particular, while cleaning it up
over the last few days), I grew concerned with widely heapam.h is
included in other headers. E.g. the executor (via execnodes.h,
executor.h relying on heapam.h) shouldn't depend on heapam.h details -
particularly after pluggable storage, but also before. To me that's
unnecessary leakage across abstraction boundaries.

In the attached patch I excised all heapam.h includes from other
headers. There were basically two things required to do so:

* In a few places that use HeapScanDesc (which confusingly is a typedef
in heapam.h, but the underlying struct is in relscan.h) etc, we can
easily get by just using struct HeapScanDescData * instead.

* I moved the LockTupleMode enum to to lockoptions.h - previously
executor.h tried to avoid relying on heapam.h, but it ended up
including heapam.h indirectly, which lead to a couple people
introducing new uses of the enum. We could just convert those to
ints like in other places, but I think moving to a smaller header
seems more appropriate. I don't think lockoptions.h is perfect, but
it's also not bad?

This required adding heapam.h includes to a bunch of places, but that
doesn't seem too bad. It'll possibly break a few external users, but I
think that's acceptable cost - many of those will already/will further
be broken in 12 anyway.

I think we should do the same with genam.h, but that seems better done
separately.

I've a pending local set of patches that splits relation_open/close,
heap_open/close et al into a separate set of includes, that then allows
to downgrade the heapam.h include to that new file (given that a large
percentage of the files really just want heap_open/close and nothing
else from heapam.h), which I'll rebase ontop of this if we can agree
that this change is a good idea.

Alvaro, you'd introduced the split of HeapScanDesc and HeapScanDescData
being in different files (in a3540b0f657c6352) - what do you think about
this change? I didn't revert that split, but I think we ought to
consider just relying on a forward declared struct in heapam.h instead,
this split is pretty confusing and seems to lead to more interdependence
in a lot of cases.

Greetings,

Andres Freund

Attachment Content-Type Size
0001-Don-t-include-heapam.h-from-others-headers.patch text/x-diff 36.5 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2019-01-14 00:52:32 Re: Shared buffer access rule violations?
Previous Message Tom Lane 2019-01-13 23:05:39 Re: Add client connection check during the execution of the query