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