Thread: htup header reorganization breaks many extension modules

I haven't followed the details of the htup header reorganization, but I
have noticed that a number of external extension modules will be broken
because of the move of GETSTRUCT() and to a lesser extent
heap_getattr().  Of course some #ifdefs can fix that, but it seems
annoying to make everyone do that.  Maybe this could be reconsidered to
reduce the impact on other projects.

On Tue, Sep 25, 2012 at 5:30 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
> I haven't followed the details of the htup header reorganization, but I
> have noticed that a number of external extension modules will be broken
> because of the move of GETSTRUCT() and to a lesser extent
> heap_getattr().  Of course some #ifdefs can fix that, but it seems
> annoying to make everyone do that.  Maybe this could be reconsidered to
> reduce the impact on other projects.
>

But it's only add #include "access/htup_details.h"?  I'd not argue
it's harmful unless I missed your point.

Thanks,
-- 
Hitoshi Harada

Hitoshi Harada wrote:
> On Tue, Sep 25, 2012 at 5:30 PM, Peter Eisentraut <peter_e@gmx.net>
wrote:
>> I haven't followed the details of the htup header reorganization, but
I
>> have noticed that a number of external extension modules will be
broken
>> because of the move of GETSTRUCT() and to a lesser extent
>> heap_getattr().  Of course some #ifdefs can fix that, but it seems
>> annoying to make everyone do that.  Maybe this could be reconsidered
to
>> reduce the impact on other projects.

> But it's only add #include "access/htup_details.h"?  I'd not argue
> it's harmful unless I missed your point.

I guess the point is that you need an #ifdef if you want a module
to be able to build with both 9.3 and lower versions.

Otherwise the compiler will complain about the missing include
file on older versions.

Yours,
Laurenz Albe

Excerpts from Peter Eisentraut's message of mar sep 25 21:30:59 -0300 2012:
> I haven't followed the details of the htup header reorganization, but I
> have noticed that a number of external extension modules will be broken
> because of the move of GETSTRUCT() and to a lesser extent
> heap_getattr().  Of course some #ifdefs can fix that, but it seems
> annoying to make everyone do that.  Maybe this could be reconsidered to
> reduce the impact on other projects.

Hmm.  My original patch didn't have this problem :-(  What it did was to
keep htup.h the "everything needed to work on tuples" header; so
external modules would have not become broken.  I didn't realize this at
the time, which is why I didn't argue to keep it that way instead of
having the new header contain most innards.

I guess we could rename the headers, so that htup.h is what's now called
htup_details.h, and htup_basics.h for what's currently htup.h.  This
would have a lot of fallout in core code, but eliminate impact on
external modules.

That said, I am not really sure that I want to promise header
compatibility forever.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

"Albe Laurenz" <laurenz.albe@wien.gv.at> writes:
> Hitoshi Harada wrote:
>> But it's only add #include "access/htup_details.h"?  I'd not argue
>> it's harmful unless I missed your point.

> I guess the point is that you need an #ifdef if you want a module
> to be able to build with both 9.3 and lower versions.

I can't get excited about this either.  This isn't the first, or the
last, change that add-on modules can expect to have to make to track
newer Postgres versions.  If we allow Peter's complaint to become the
new project policy, we'll never be able to make any header
rearrangements at all, nor change any internal APIs.
        regards, tom lane

On 9/26/12 10:07 AM, Tom Lane wrote:
> I can't get excited about this either.  This isn't the first, or the
> last, change that add-on modules can expect to have to make to track
> newer Postgres versions.  If we allow Peter's complaint to become the
> new project policy, we'll never be able to make any header
> rearrangements at all, nor change any internal APIs.

I'm not saying we can never change anything about the internal headers,
but we can make a small effort not to create useless annoyances.

That said, could someone clarify the header comments in the new headers?We currently have
* htup.h*        POSTGRES heap tuple definitions.
* htup_details.h*        POSTGRES heap tuple header definitions.

The names of the headers don't match their documented purpose very much.Is GETSTRUCT a "detail" of the heap tuple
definition,or is it related
 
to "tuple headers"?  It's not really either, but I guess it is related
to tuple headers because you need to know about the headers to get to
the stuff past it.

When I see headers stuff.h and stuff_details.h, it makes me think that
you should only use stuff.h, and stuff_details.h are internal things.
But a lot of external modules use GETSTRUCT, so they might get confused.

Excerpts from Peter Eisentraut's message of mié sep 26 11:18:51 -0300 2012:
> On 9/26/12 10:07 AM, Tom Lane wrote:
> > I can't get excited about this either.  This isn't the first, or the
> > last, change that add-on modules can expect to have to make to track
> > newer Postgres versions.  If we allow Peter's complaint to become the
> > new project policy, we'll never be able to make any header
> > rearrangements at all, nor change any internal APIs.
>
> I'm not saying we can never change anything about the internal headers,
> but we can make a small effort not to create useless annoyances.

I proposed a possible way out of the problem elsewhere.  Please comment
on that.

> That said, could someone clarify the header comments in the new headers?
>  We currently have
>
>  * htup.h
>  *        POSTGRES heap tuple definitions.
>
>  * htup_details.h
>  *        POSTGRES heap tuple header definitions.

htup.h is what you need if you want to pass tuples around.  It's
particularly useful for other headers that want to declare their
functions as receiving or returning tuples.  htup_details.h is what you
need if you want to operate on tuples, such as creating them or
examining them.

I guess those comments aren't all that well thought out; suggestions
welcome.

> The names of the headers don't match their documented purpose very much.
>  Is GETSTRUCT a "detail" of the heap tuple definition, or is it related
> to "tuple headers"?  It's not really either, but I guess it is related
> to tuple headers because you need to know about the headers to get to
> the stuff past it.

> When I see headers stuff.h and stuff_details.h, it makes me think that
> you should only use stuff.h, and stuff_details.h are internal things.
> But a lot of external modules use GETSTRUCT, so they might get confused.

The other proposal was htup_internal.h but that would have been much
more indicative of stuff that's supposed to be used only for internal
consumption of files in backend/access/heap and such, which is why I
stayed away from that name.  I think htup_details.h is a good enough
compromise.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services