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