Re: [HACKERS] warning when compiling utils/tqual.h
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] warning when compiling utils/tqual.h
Alvaro Herrera alvhe...@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 by default] /pgsql/source/HEAD/src/include/utils/tqual.h:101:13: warning: its scope is only this definition or declaration, which is 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. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] warning when compiling utils/tqual.h
On 2014-03-17 12:50:37 -0400, Tom Lane wrote: Alvaro Herrera alvhe...@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 by default] /pgsql/source/HEAD/src/include/utils/tqual.h:101:13: warning: its scope is only this definition or declaration, which is 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] warning when compiling utils/tqual.h
Andres Freund and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] warning when compiling utils/tqual.h
Andres Freund and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] warning when compiling utils/tqual.h
On 2014-03-17 12:57:15 -0400, Tom Lane wrote: Andres Freund and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] warning when compiling utils/tqual.h
On 2014-03-17 12:56:12 -0400, Tom Lane wrote: Andres Freund and...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] warning when compiling utils/tqual.h
On Mon, Mar 17, 2014 at 12:55 PM, Andres Freund and...@2ndquadrant.com wrote: On 2014-03-17 12:50:37 -0400, Tom Lane wrote: Alvaro Herrera alvhe...@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 by default] /pgsql/source/HEAD/src/include/utils/tqual.h:101:13: warning: its scope is only this definition or declaration, which is 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
