Обсуждение: warning when compiling utils/tqual.h
I noticed (by running "cd src/include ; make check" with the attached
patch applied) that since commit b89e151054 ("Introduce logical
decoding.") tqual.h now emits this warning when compiled standalone:
/pgsql/source/HEAD/src/include/utils/tqual.h:101:13: warning: ‘struct HTAB’ declared inside parameter list [enabled by
default]
/pgsql/source/HEAD/src/include/utils/tqual.h:101:13: warning: its scope is only this definition or declaration, which
isprobably not what you want [enabled by default]
The prototype in question is:
/** To avoid leaking to much knowledge about reorderbuffer implementation* details this is implemented in
reorderbuffer.cnot tqual.c.*/
extern bool ResolveCminCmaxDuringDecoding(HTAB *tuplecid_data, Snapshot
snapshot, HeapTuple htup, Buffer buffer,
CommandId *cmin, CommandId *cmax);
Now there are two easy ways to silence this:
1. add "struct HTAB;" to the previous line in tqual.h, i.e. a forward
declaration. We have very few of these; most of the time ISTM we prefer
a solution like the next one:
2. add #include "utils/hsearch.h" at the top of tqual.h.
Now my inclination is to use the second one, because it seems cleaner to
me and avoid having a forward struct decl in an unrelated header, which
will later bleed into other source files. Do we have a project
guideline saying that we prefer option two, and does that explain why we
have so few declarations as in option one?
There is of course a third choice which is to dictate that this function
ought to be declared in reorderbuffer.h; but that would have the
unpleasant side-effect that tqual.c would need to #include that.
Opinions please? I would like to have a guideline about this so I can
reason in a principled way in future cases in which this kind of
question arises.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Hi,
On 2014-03-17 13:40:53 -0300, Alvaro Herrera wrote:
> I noticed (by running "cd src/include ; make check" with the attached
> patch applied) that since commit b89e151054 ("Introduce logical
> decoding.") tqual.h now emits this warning when compiled standalone:
I think we should add such a check (refined to not rely on precompiled
headers) to check-world or something...
> 1. add "struct HTAB;" to the previous line in tqual.h, i.e. a forward
> declaration. We have very few of these; most of the time ISTM we prefer
> a solution like the next one:
That's what I am for.
We don't have *that* few of those. There's also several cases where
struct pointers are members in other structs. There you don't even need
a forward declaration (like e.g. struct HTAB* in PlannerInfo).
> There is of course a third choice which is to dictate that this function
> ought to be declared in reorderbuffer.h; but that would have the
> unpleasant side-effect that tqual.c would need to #include that.
I am pretty clearly against this.
> Opinions please? I would like to have a guideline about this so I can
> reason in a principled way in future cases in which this kind of
> question arises.
I'd welcome a clearly stated policy as well. I think forward
declarations are sensible tool if used sparingly, to decouple things
that are primarily related on the implementation level.
Greetings,
Andres Freund
-- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> I noticed (by running "cd src/include ; make check" with the attached
> patch applied) that since commit b89e151054 ("Introduce logical
> decoding.") tqual.h now emits this warning when compiled standalone:
> /pgsql/source/HEAD/src/include/utils/tqual.h:101:13: warning: ‘struct HTAB’ declared inside parameter list [enabled
bydefault]
> /pgsql/source/HEAD/src/include/utils/tqual.h:101:13: warning: its scope is only this definition or declaration, which
isprobably not what you want [enabled by default]
> The prototype in question is:
> /*
> * To avoid leaking to much knowledge about reorderbuffer implementation
> * details this is implemented in reorderbuffer.c not tqual.c.
> */
> extern bool ResolveCminCmaxDuringDecoding(HTAB *tuplecid_data,
> Snapshot snapshot,
> HeapTuple htup,
> Buffer buffer,
> CommandId *cmin, CommandId *cmax);
I guess the real question is why such a prototype is in tqual.h in
the first place. ISTM this should be pushed somewhere specific to
reorderbuffer.c. I'm -1 on having struct HTAB bleed into tqual.h
via either of the methods you suggest.
regards, tom lane
On 2014-03-17 12:50:37 -0400, Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > I noticed (by running "cd src/include ; make check" with the attached
> > patch applied) that since commit b89e151054 ("Introduce logical
> > decoding.") tqual.h now emits this warning when compiled standalone:
>
> > /pgsql/source/HEAD/src/include/utils/tqual.h:101:13: warning: ‘struct HTAB’ declared inside parameter list [enabled
bydefault]
> > /pgsql/source/HEAD/src/include/utils/tqual.h:101:13: warning: its scope is only this definition or declaration,
whichis probably not what you want [enabled by default]
>
> > The prototype in question is:
>
> > /*
> > * To avoid leaking to much knowledge about reorderbuffer implementation
> > * details this is implemented in reorderbuffer.c not tqual.c.
> > */
> > extern bool ResolveCminCmaxDuringDecoding(HTAB *tuplecid_data,
> > Snapshot snapshot,
> > HeapTuple htup,
> > Buffer buffer,
> > CommandId *cmin, CommandId *cmax);
>
> I guess the real question is why such a prototype is in tqual.h in
> the first place. ISTM this should be pushed somewhere specific to
> reorderbuffer.c. I'm -1 on having struct HTAB bleed into tqual.h
> via either of the methods you suggest.
It's the only logical decoding specific routine that's called by tqual.c
which doesn't need anything else from reorderbuffer. I'd like to avoid
more stuff bleeding in there...
Greetings,
Andres Freund
-- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services
Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-03-17 13:40:53 -0300, Alvaro Herrera wrote:
>> There is of course a third choice which is to dictate that this function
>> ought to be declared in reorderbuffer.h; but that would have the
>> unpleasant side-effect that tqual.c would need to #include that.
> I am pretty clearly against this.
Let me get this straight. reorderbuffer.c exports a function that needs
to be used by tqual.c. The obvious method to do this is to declare the
function in reorderbuffer.h and have tqual.c #include that. Apparently
you think it's better to have tqual.h declare the function. How is that
not 100% backwards? Even worse that it requires more header-inclusion
bloat for some functionality entirely unrelated to snapshots?
That sounds borderline insane from here. You need a whole lot more
justification than "I'm against it".
regards, tom lane
Andres Freund <andres@2ndquadrant.com> writes:
> On 2014-03-17 12:50:37 -0400, Tom Lane wrote:
>> I guess the real question is why such a prototype is in tqual.h in
>> the first place. ISTM this should be pushed somewhere specific to
>> reorderbuffer.c. I'm -1 on having struct HTAB bleed into tqual.h
>> via either of the methods you suggest.
> It's the only logical decoding specific routine that's called by tqual.c
> which doesn't need anything else from reorderbuffer. I'd like to avoid
> more stuff bleeding in there...
Instead, you'd like HTAB to bleed into everything that uses tqual.h?
regards, tom lane
On 2014-03-17 12:57:15 -0400, Tom Lane wrote: > Andres Freund <andres@2ndquadrant.com> writes: > > On 2014-03-17 12:50:37 -0400, Tom Lane wrote: > >> I guess the real question is why such a prototype is in tqual.h in > >> the first place. ISTM this should be pushed somewhere specific to > >> reorderbuffer.c. I'm -1 on having struct HTAB bleed into tqual.h > >> via either of the methods you suggest. > > > It's the only logical decoding specific routine that's called by tqual.c > > which doesn't need anything else from reorderbuffer. I'd like to avoid > > more stuff bleeding in there... > > Instead, you'd like HTAB to bleed into everything that uses tqual.h? Meh. a struct forward declaration isn't exactly a significant thing to expose. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 2014-03-17 12:56:12 -0400, Tom Lane wrote: > Andres Freund <andres@2ndquadrant.com> writes: > > On 2014-03-17 13:40:53 -0300, Alvaro Herrera wrote: > >> There is of course a third choice which is to dictate that this function > >> ought to be declared in reorderbuffer.h; but that would have the > >> unpleasant side-effect that tqual.c would need to #include that. > > > I am pretty clearly against this. > > Let me get this straight. reorderbuffer.c exports a function that needs > to be used by tqual.c. The obvious method to do this is to declare the > function in reorderbuffer.h and have tqual.c #include that. Apparently > you think it's better to have tqual.h declare the function. How is that > not 100% backwards? Even worse that it requires more header-inclusion > bloat for some functionality entirely unrelated to snapshots? I don't think cmin/cmax are entirely unrelated to tuple visibility. If it didn't require exposing reorderbuffer.c internals, it's implementation should have been in tqual.c. And a struct HTAB; wouldn't require including a header. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Mon, Mar 17, 2014 at 12:55 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2014-03-17 12:50:37 -0400, Tom Lane wrote:
>> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> > I noticed (by running "cd src/include ; make check" with the attached
>> > patch applied) that since commit b89e151054 ("Introduce logical
>> > decoding.") tqual.h now emits this warning when compiled standalone:
>>
>> > /pgsql/source/HEAD/src/include/utils/tqual.h:101:13: warning: 'struct HTAB' declared inside parameter list
[enabledby default]
>> > /pgsql/source/HEAD/src/include/utils/tqual.h:101:13: warning: its scope is only this definition or declaration,
whichis probably not what you want [enabled by default]
>>
>> > The prototype in question is:
>>
>> > /*
>> > * To avoid leaking to much knowledge about reorderbuffer implementation
>> > * details this is implemented in reorderbuffer.c not tqual.c.
>> > */
>> > extern bool ResolveCminCmaxDuringDecoding(HTAB *tuplecid_data,
>> > Snapshot snapshot,
>> > HeapTuple htup,
>> > Buffer buffer,
>> > CommandId *cmin, CommandId *cmax);
>>
>> I guess the real question is why such a prototype is in tqual.h in
>> the first place. ISTM this should be pushed somewhere specific to
>> reorderbuffer.c. I'm -1 on having struct HTAB bleed into tqual.h
>> via either of the methods you suggest.
>
> It's the only logical decoding specific routine that's called by tqual.c
> which doesn't need anything else from reorderbuffer. I'd like to avoid
> more stuff bleeding in there...
That's 100% irrelevant. Function declarations for backend/X/Y/Z.c are
supposed to be in include/X/Z.h with a few historical exceptions I
hope we won't add to.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company