Thread: coverage additions
I just enabled --enabled-llvm on the coverage reporting machine, which made src/backend/jit/jit.c go from 60/71 % (line/function wise) to 78/85 % ... and src/backend/jit/llvm from not appearing at all in the report to 78/94 %. That's a good improvement. If there are other obvious improvements to be had, please let me know. (We have PG_TEST_EXTRA="ssl ldap" currently, do we have any more extra tests now?) -- Álvaro Herrera
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> I just enabled --enabled-llvm on the coverage reporting machine, which
> made src/backend/jit/jit.c go from 60/71 % (line/function wise) to 78/85 % ...
> and src/backend/jit/llvm from not appearing at all in the report to
> 78/94 %. That's a good improvement.
> If there are other obvious improvements to be had, please let me know.
I was going to suggest that adding some or all of
-DCOPY_PARSE_PLAN_TREES
-DWRITE_READ_PARSE_PLAN_TREES
-DRAW_EXPRESSION_COVERAGE_TEST
to your CPPFLAGS might improve the reported coverage in backend/nodes/,
and perhaps other places.
regards, tom lane
On 2019-May-30, Tom Lane wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > I just enabled --enabled-llvm on the coverage reporting machine, which > > made src/backend/jit/jit.c go from 60/71 % (line/function wise) to 78/85 % ... > > and src/backend/jit/llvm from not appearing at all in the report to > > 78/94 %. That's a good improvement. > > > If there are other obvious improvements to be had, please let me know. > > I was going to suggest that adding some or all of > > -DCOPY_PARSE_PLAN_TREES > -DWRITE_READ_PARSE_PLAN_TREES > -DRAW_EXPRESSION_COVERAGE_TEST > > to your CPPFLAGS might improve the reported coverage in backend/nodes/, > and perhaps other places. I did that, and it certainly increased backend/nodes numbers considerably. Thanks. (extensible.c remains at 0% though, as does its companion nodeCustom.c). -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Apparently, for ecpg you have to do "make checktcp" in order for some of the tests to run, and "make check-world" doesn't do that. Not sure what's a good fix for this; do we want to add "make -C src/interfaces/ecpg/test checktcp" to what "make check-world" does, or do we rather what to add checktcp as a dependency of "make check" in src/interfaces/ecpg? Or do we just not want this test to be run by default, and thus I should add "make -C src/interfaces/ecpg/test checktcp" to coverage.pg.org's shell script? Maybe all we need is a way to have it run using the PG_EXTRA_TEST thingy, but I'm not sure how that works ...? -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Apparently, for ecpg you have to do "make checktcp" in order for some of
> the tests to run, and "make check-world" doesn't do that. Not sure
> what's a good fix for this; do we want to add "make -C
> src/interfaces/ecpg/test checktcp" to what "make check-world" does,
> or do we rather what to add checktcp as a dependency of "make check" in
> src/interfaces/ecpg?
> Or do we just not want this test to be run by default, and thus I should
> add "make -C src/interfaces/ecpg/test checktcp" to coverage.pg.org's
> shell script?
I believe it's intentionally not run by default because it opens up
an externally-accessible server port.
regards, tom lane
On Thu, May 30, 2019 at 01:52:20PM -0400, Alvaro Herrera wrote: > If there are other obvious improvements to be had, please let me know. > (We have PG_TEST_EXTRA="ssl ldap" currently, do we have any more extra > tests now?) You can add kerberos to this list, to give: PG_TEST_EXTRA='ssl ldap kerberos' -- Michael
Attachment
On 2019-May-30, Michael Paquier wrote: > On Thu, May 30, 2019 at 01:52:20PM -0400, Alvaro Herrera wrote: > > If there are other obvious improvements to be had, please let me know. > > (We have PG_TEST_EXTRA="ssl ldap" currently, do we have any more extra > > tests now?) > > You can add kerberos to this list, to give: > PG_TEST_EXTRA='ssl ldap kerberos' Ah, now I remember that I tried this before, but it requires some extra packages installed in the machine I think, and those create running services. Did you note that src/backend/libpq does not even list the gssapi file? -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, 2019-05-30 at 17:54 -0400, Tom Lane wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > Apparently, for ecpg you have to do "make checktcp" in order for > > some of > > the tests to run, and "make check-world" doesn't do that. Not sure > > what's a good fix for this; do we want to add "make -C > > src/interfaces/ecpg/test checktcp" to what "make check-world" does, > > or do we rather what to add checktcp as a dependency of "make > > check" in > > src/interfaces/ecpg? > > Or do we just not want this test to be run by default, and thus I > > should > > add "make -C src/interfaces/ecpg/test checktcp" to > > coverage.pg.org's > > shell script? > > I believe it's intentionally not run by default because it opens up > an externally-accessible server port. Correct, iirc. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Meskes at (Debian|Postgresql) dot Org Jabber: michael at xmpp dot meskes dot org VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL
On 2019-Jun-02, Michael Meskes wrote: > On Thu, 2019-05-30 at 17:54 -0400, Tom Lane wrote: > > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > > Or do we just not want this test to be run by default, and thus I > > > should add "make -C src/interfaces/ecpg/test checktcp" to > > > coverage.pg.org's shell script? > > > > I believe it's intentionally not run by default because it opens up > > an externally-accessible server port. > > Correct, iirc. Okay ... I added a "make -C src/interfaces/ecpg/test checktcp". Now function-wise ecpg seems reasonable almost everywhere except compatlib (though line-wise things are not so great). -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, Jun 01, 2019 at 12:55:47AM -0400, Alvaro Herrera wrote: > Ah, now I remember that I tried this before, but it requires some extra > packages installed in the machine I think, and those create running > services. Did you note that src/backend/libpq does not even list the > gssapi file? Do you mean the header file be-gssapi-common.h? It is stored in src/backend/libpq/ which is obviously incorrect. I think that it should be moved to src/include/libpq/be-gssapi-common.h. Its identification marker even says that. Perhaps that's because of MSVC? Stephen? -- Michael
Attachment
On 2019-Jun-04, Michael Paquier wrote: > On Sat, Jun 01, 2019 at 12:55:47AM -0400, Alvaro Herrera wrote: > > Ah, now I remember that I tried this before, but it requires some extra > > packages installed in the machine I think, and those create running > > services. Did you note that src/backend/libpq does not even list the > > gssapi file? > > Do you mean the header file be-gssapi-common.h? Actually, I meant be-gssapi-common.c, but I suppose having the file appear at all would be dependent on whether the GSSAPI stuff is compiled in, which seems to require yet another configure switch that we don't have in the coverage machine. But yeah, I think be-gssapi-common.h be in src/backend/libpq is against our established practice and we should put it in src/include/libpq. Which in turn makes me think that perhaps src/include/libpq/libpq.h needs some splitting or something, because the be-openssl-common.c file does not seem to have a corresponding header ... -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Jun 04, 2019 at 04:07:17PM -0400, Alvaro Herrera wrote: > On 2019-Jun-04, Michael Paquier wrote: >> On Sat, Jun 01, 2019 at 12:55:47AM -0400, Alvaro Herrera wrote: >>> Ah, now I remember that I tried this before, but it requires some extra >>> packages installed in the machine I think, and those create running >>> services. Did you note that src/backend/libpq does not even list the >>> gssapi file? >> >> Do you mean the header file be-gssapi-common.h? > > Actually, I meant be-gssapi-common.c, but I suppose having the file > appear at all would be dependent on whether the GSSAPI stuff is compiled > in, which seems to require yet another configure switch that we don't > have in the coverage machine. Not sure I still follow.. In src/backend/libpq we have be-gssapi-common.c and be-gssapi-common.c, both getting added only if with_gssapi is enabled. > Which in turn makes me think that perhaps src/include/libpq/libpq.h > needs some splitting or something, because the be-openssl-common.c file > does not seem to have a corresponding header ... Yeah, it seems that there could be ways to split that in a smarter way. -- Michael
Attachment
On Thu, Jun 06, 2019 at 06:14:45PM +0900, Michael Paquier wrote: > Not sure I still follow.. In src/backend/libpq we have > be-gssapi-common.c and be-gssapi-common.c, both getting added only if > with_gssapi is enabled. I am going to spawn a new thread with a patch for the header file. I think that we had better fix that before v12 ships. -- Michael