Thread: make installcheck-world in a clean environment
I am trying to perform "make installcheck-world" after "make clean" (or after installing a precompiled binaries), but it fails.
The error I get is related to ECPG regression test:
make -C test installcheck
make[2]: Entering directory `/home/postgres/postgres/src/interfaces/ecpg/test'
gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -O2 '-I../../../../src/port' '-I../../../../src/test/regress' '-DHOST_TUPLE="x86_64-pc-linux-gnu"' '-DSHELLPROG="/bin/sh"' '-DDLSUFFIX=".so"' -I../../../../src/include -D_GNU_SOURCE -c -o pg_regress_ecpg.o pg_regress_ecpg.c
gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -O2 -L../../../../src/port -L../../../../src/common -Wl,--as-needed -Wl,-rpath,'/usr/local/pgsql/lib',--enable-new-dtags pg_regress_ecpg.o ../../../../src/test/regress/pg_regress.o -lpgcommon -lpgport -lpthread -lz -lreadline -lrt -lcrypt -ldl -lm -o pg_regress
make -C connect all
make[3]: Entering directory `/home/postgres/postgres/src/interfaces/ecpg/test/connect'
make[3]: *** No rule to make target `test1.c', needed by `test1.o'. Stop.
make[3]: Leaving directory `/home/postgres/postgres/src/interfaces/ecpg/test/connect'
Is it a supported scenario to make installcheck-world without performing "make" first?
(If I do "make -C src/interfaces/ecpg" and then "make installcheck-world", then this error is gone. And when I set up all the extensions, all tests passed successfully.)
And even if we need to perform make, I wonder, should the recompiled ecpg binary be checked instead of installed one?
I tried to modify Makefile to target installed ecpg binary and it's libs (see the patch attached), it works, but this fix is only suited for installcheck.
So if this scenario should be supported, a more elaborated fix is needed.
Best regards,
------
Alexander Lakhin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachment
To avoid overheating of this pretty hot discussion, I would like just to propose "a more elaborated fix" (for REL_10_STABLE and master).
Is it a supported scenario to make installcheck-world without performing "make" first?
(If I do "make -C src/interfaces/ecpg" and then "make installcheck-world", then this error is gone. And when I set up all the extensions, all tests passed successfully.)
And even if we need to perform make, I wonder, should the recompiled ecpg binary be checked instead of installed one?
I tried to modify Makefile to target installed ecpg binary and it's libs (see the patch attached), it works, but this fix is only suited for installcheck.
So if this scenario should be supported, a more elaborated fix is needed.
In fact, when we perform "make installcheck" it not only requires us to build ecpg, but it also rebuilds libpostgres, libpgport and libpq (for installcheck-world).
I believe that the larger testing surface (coverage), the better, so using installed assets (libs, headers) is more useful.
Regarding "remote installcheck", that was discussed recently, the proposed patch complicates this, but opens a way to implement it correctly.
Think of distinct target "remotecheck", that will not define USE_INSTALLED_ASSETS, but will account for remote connection to server and run only supported tests.
Best regards,
------
Alexander Lakhin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachment
To avoid overheating of this pretty hot discussion, I would like just to propose "a more elaborated fix" (for REL_10_STABLE and master).In hope to keep a lid on this explosive topic, I propose the improved fix.
In fact, when we perform "make installcheck" it not only requires us to build ecpg, but it also rebuilds libpostgres, libpgport and libpq (for installcheck-world).
I believe that the larger testing surface (coverage), the better, so using installed assets (libs, headers) is more useful.
Best regards,
------
Alexander Lakhin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachment
On Mon, Apr 2, 2018 at 5:12 AM, Alexander Lakhin <exclusion@gmail.com> wrote: > Is it a supported scenario to make installcheck-world without performing > "make" first? The evidence suggests that the answer is "no". > (If I do "make -C src/interfaces/ecpg" and then "make installcheck-world", > then this error is gone. And when I set up all the extensions, all tests > passed successfully.) Your proposed fix involves inventing something called USE_INSTALLED_ASSETS, but we don't need that anywhere else, so why do we need it for ECPG? I am not opposed to fixing this, but one might also ask whether it's a good use of time, since the workaround is straightforward. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Thanks for your involvement!
We need that not only for ECPG, but for libpostgres, libpgport, and libpq too, if we want to check the installed ones (instead of recompiled).Your proposed fix involves inventing something called USE_INSTALLED_ASSETS, but we don't need that anywhere else, so why do we need it for ECPG?
Imagine that we will recompile psql (and use it) to perform installcheck for the server instance. It will work but we'll miss an opportunity to check whether the installed psql is working. I believe, it's the same with ecpg (as now, we can even remove installed ecpg and have all the tests passed).
I am not opposed to fixing this, but one might also ask whether it's a good use of time, since the workaround is straightforward.
I'm not seeing a workaround to perform more complete installcheck without modifying makefiles. So for me the question is whether the increasing the testing surface justifies this use of time.
Best regards,
------
Alexander Lakhin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On Fri, May 4, 2018 at 8:30 AM, Alexander Lakhin <exclusion@gmail.com> wrote: > I'm not seeing a workaround to perform more complete installcheck without > modifying makefiles. So for me the question is whether the increasing the > testing surface justifies this use of time. After thinking about this some more, I think the question here is definitional. A first attempt at defining 'make installcheck' is to say that it runs the tests from the build tree against the running server. Certainly, we intend to use the SQL files that are present in the build tree, not the ones that were present in the build tree where the running server was built. But what about client programs that we use to connect to the server? You're suggesting that we use the pre-installed ones, but that is actually pretty problematic because the ones we see as installed might correspond neither to the contents of the build tree nor to the running server. Conceivably we could end up having a mix of assets from three different places: (1) the running server, (2) the build tree, (3) whatever is in our path at the moment. That seems very confusing. So now I think it's probably right to define 'make installcheck' as using the assets from the build tree to test the running server. Under that definition, we're missing some dependencies, but USE_INSTALLED_ASSETS isn't a thing we need. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes: > On Fri, May 4, 2018 at 8:30 AM, Alexander Lakhin <exclusion@gmail.com> wrote: >> I'm not seeing a workaround to perform more complete installcheck without >> modifying makefiles. So for me the question is whether the increasing the >> testing surface justifies this use of time. > After thinking about this some more, I think the question here is > definitional. A first attempt at defining 'make installcheck' is to > say that it runs the tests from the build tree against the running > server. Certainly, we intend to use the SQL files that are present in > the build tree, not the ones that were present in the build tree where > the running server was built. But what about client programs that we > use to connect to the server? You're suggesting that we use the > pre-installed ones, but that is actually pretty problematic because > the ones we see as installed might correspond neither to the contents > of the build tree nor to the running server. Conceivably we could end > up having a mix of assets from three different places: (1) the running > server, (2) the build tree, (3) whatever is in our path at the moment. > That seems very confusing. So now I think it's probably right to > define 'make installcheck' as using the assets from the build tree to > test the running server. Under that definition, we're missing some > dependencies, but USE_INSTALLED_ASSETS isn't a thing we need. Nah, I disagree with this. To me, the purpose of "make installcheck" is to verify the correctness of an installation, which I take to include the client programs as well as the server. I think that "make installcheck" ought to use the installed version of any file that we actually install, and go to the build tree only for things we don't install (e.g. SQL test scripts). If the user has screwed up his PATH or other environmental aspects so that what he's testing isn't a single installation, that's his error, not something that "make installcheck" ought to work around. Indeed, maybe such aspects of his setup are intentional, and second-guessing them would completely defeat his purpose. In any case, if you want to test the build-tree assets, that's what "make check" is for. regards, tom lane
If the contents of the source tree doesn't correspond to the running server, then I'm afraid, we can't installcheck it for sure.Robert Haas <robertmhaas@gmail.com> writes:After thinking about this some more, I think the question here is definitional. A first attempt at defining 'make installcheck' is to say that it runs the tests from the build tree against the running server. Certainly, we intend to use the SQL files that are present in the build tree, not the ones that were present in the build tree where the running server was built. But what about client programs that we use to connect to the server? You're suggesting that we use the pre-installed ones, but that is actually pretty problematic because the ones we see as installed might correspond neither to the contents of the build tree nor to the running server.
I think it's supposed that we use for installcheck exactly the same source files that was used to build the server.
Regarding clients program, if we will not use/check it while installchecking, then by what means they can be tested when installed?
I think, the pgsql-packagers would like to check whether the whole installation of PostgreSQL is working, not only the server.
For me, very realistic and most useful scenario of installcheck is:
install postgresql-x.rpm
install postgresql-x.src.rpm
./configure --prefix=$target_path --enable-tap-tests, etc...
make installcheck(-world)
I see another scenario, that was discussed a month ago - remote (or server-only) installcheck.Conceivably we could end up having a mix of assets from three different places: (1) the running server, (2) the build tree, (3) whatever is in our path at the moment. That seems very confusing. So now I think it's probably right to define 'make installcheck' as using the assets from the build tree to test the running server. Under that definition, we're missing some dependencies, but USE_INSTALLED_ASSETS isn't a thing we need.
( https://www.postgresql.org/message-id/flat/CAM0nTJ6iorX_tmg5MX0mQU3z3w%3D9wk%2BpGK8zrvn7DNWqnyv%2BsQ%40mail.gmail.com )
It can be useful too and if it will be supported, then USE_INSTALLED_ASSETS usage should be extended to psql, etc.
Yes, that's the proposed patches intended for. I didn't encounter any problems with it during internal testing with Linux and mingw-builds.Nah, I disagree with this. To me, the purpose of "make installcheck" is to verify the correctness of an installation, which I take to include the client programs as well as the server. I think that "make installcheck" ought to use the installed version of any file that we actually install, and go to the build tree only for things we don't install (e.g. SQL test scripts).
Even modified configs could lead to test failures (for example, lc_messages can break server log checking), so the installcheck should be performed against some clean and determinated installation anyway.If the user has screwed up his PATH or other environmental aspects so that what he's testing isn't a single installation, that's his error, not something that "make installcheck" ought to work around. Indeed, maybe such aspects of his setup are intentional, and second-guessing them would completely defeat his purpose. In any case, if you want to test the build-tree assets, that's what "make check" is for.
Best regards,
------
Alexander Lakhin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Exactly what order of steps are you executing that doesn't work? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
06.07.2018 00:39, Peter Eisentraut wrote:
Exactly what order of steps are you executing that doesn't work?In Centos 7, using the master branch from git:
./configure --enable-tap-tests
make install
make install -C contrib
chown -R postgres:postgres /usr/local/pgsql/
/usr/local/pgsql/bin/initdb -D /usr/local/pgsql/data
/usr/local/pgsql/bin/pg_ctl start -l logfile -D /usr/local/pgsql/data
make clean
# Also you can just install binary packages to get the same state.
make installcheck-world
# This check fails.
Best regards,
------
Alexander Lakhin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Alexander Lakhin <exclusion@gmail.com> writes: > 06.07.2018 00:39, Peter Eisentraut wrote: >> Exactly what order of steps are you executing that doesn't work? > In Centos 7, using the master branch from git: > ./configure --enable-tap-tests > make install > make install -C contrib > chown -R postgres:postgres /usr/local/pgsql/ > /usr/local/pgsql/bin/initdb -D /usr/local/pgsql/data > /usr/local/pgsql/bin/pg_ctl start -l logfile -D /usr/local/pgsql/data > /make clean/ > # Also you can just install binary packages to get the same state. > make installcheck-world > # This check fails. I do not think that should be expected to work. It would require that "make installcheck" first invoke "make all" (to rebuild the stuff you threw away with "make clean"), which is rather antithetical to its purpose. Specifically, installcheck is supposed to be checking something you already built; so having it do a fresh build seems to introduce version-skew hazards that we don't need. regards, tom lane
11.07.2018 23:15, Tom Lane wrote:
If I understand correctly, the installcheck target should not use the stuff in the build directory (that is thrown away with 'make clean'), but instead should use/check the installed assets./make clean/ # Also you can just install binary packages to get the same state.make installcheck-world # This check fails.I do not think that should be expected to work. It would require that "make installcheck" first invoke "make all" (to rebuild the stuff you threw away with "make clean"), which is rather antithetical to its purpose. Specifically, installcheck is supposed to be checking something you already built; so having it do a fresh build seems to introduce version-skew hazards that we don't need.
In fact with REL_10_STABLE you can run "make clean && make installcheck" and it works just fine (without calling 'make all'). It's sufficient to have a working instance running. And I think, this behavior is correct — "installcheck" supposed to check not something we built ("check" is supposed to do that), but something we installed.
And only if I run "make installcheck-world" with REL_10_STABLE I get an error related to ECPG, I've referred to in the first message in this thread.
In the master branch there was some changes that prevent "make clean && make installcheck" scenario to run, but I think it can (and should) be fixed too.
(When I prepared the patches, there were no differences between these branches in this aspect.)
So for me the question is what assets should the installcheck target be checking? Installed or built ones? For example, if psql/pg_dump/ecpg/... is installed in /usr/local/pgsql/bin/, should it be checked by the installcheck? And if we target the installed stuff then why do we need to build something (or have something built) for installcheck?
Best regards,
------
Alexander Lakhin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On 06.07.18 09:45, Alexander Lakhin wrote: > ./configure --enable-tap-tests > make install > make install -C contrib > chown -R postgres:postgres /usr/local/pgsql/ > /usr/local/pgsql/bin/initdb -D /usr/local/pgsql/data > /usr/local/pgsql/bin/pg_ctl start -l logfile -D /usr/local/pgsql/data > /make clean/ > # Also you can just install binary packages to get the same state. > > make installcheck-world > # This check fails. For me, this fails at In file included from ../../src/include/postgres.h:47, from chklocale.c:17: ../../src/include/utils/elog.h:71:10: fatal error: utils/errcodes.h: No such file or directory That's probably fixable. But it's different from what you had initially reported. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hello Peter, 14.07.2018 13:57, Peter Eisentraut wrote: > On 06.07.18 09:45, Alexander Lakhin wrote: >> ./configure --enable-tap-tests >> make install >> make install -C contrib >> chown -R postgres:postgres /usr/local/pgsql/ >> /usr/local/pgsql/bin/initdb -D /usr/local/pgsql/data >> /usr/local/pgsql/bin/pg_ctl start -l logfile -D /usr/local/pgsql/data >> /make clean/ >> # Also you can just install binary packages to get the same state. >> >> make installcheck-world >> # This check fails. > For me, this fails at > > In file included from ../../src/include/postgres.h:47, > from chklocale.c:17: > ../../src/include/utils/elog.h:71:10: fatal error: utils/errcodes.h: No > such file or directory > > That's probably fixable. But it's different from what you had initially > reported. This error wasn't present in master at the time of initial report (in April). As "git bisect" shows such errors appeared after 372728b. So in REL_10_STABLE (or in master before 372728b) you could "make installcheck" (but not installcheck-world) in a clean environment.
Alexander Lakhin <exclusion@gmail.com> writes: > 14.07.2018 13:57, Peter Eisentraut wrote: >> On 06.07.18 09:45, Alexander Lakhin wrote: >>> ./configure --enable-tap-tests >>> make install >>> make install -C contrib >>> chown -R postgres:postgres /usr/local/pgsql/ >>> /usr/local/pgsql/bin/initdb -D /usr/local/pgsql/data >>> /usr/local/pgsql/bin/pg_ctl start -l logfile -D /usr/local/pgsql/data >>> /make clean/ >>> # Also you can just install binary packages to get the same state. >>> >>> make installcheck-world >>> # This check fails. I remain pretty skeptical that this is a sensible way to proceed, especially not if what you're testing is installed binary packages. You're creating yet *another* hazard for version-skew-like problems, namely that there's no certainty that you gave configure arguments that're compatible with what the installed packages used. But having said that ... >> For me, this fails at >> In file included from ../../src/include/postgres.h:47, >> from chklocale.c:17: >> ../../src/include/utils/elog.h:71:10: fatal error: utils/errcodes.h: No >> such file or directory >> That's probably fixable. But it's different from what you had initially >> reported. > This error wasn't present in master at the time of initial report (in > April). As "git bisect" shows such errors appeared after 372728b. > So in REL_10_STABLE (or in master before 372728b) you could "make > installcheck" (but not installcheck-world) in a clean environment. ... I think this was actually induced by 3b8f6e75f, for which we already had to make some adjustments to ensure that the generated headers got built when needed. This seems like another such case, so I stuck in a couple more submake-generated-headers dependencies to fix it. The original complaint about ecpg remains; I'm not terribly excited about messing with that. regards, tom lane
I wrote: > The original complaint about ecpg remains; I'm not terribly excited > about messing with that. Well, I got curious as to why we were seeing such a weird error, and eventually traced it to this stuff in ecpg/test/Makefile.regress: # Standard way to invoke the ecpg preprocessor ECPG = ../../preproc/ecpg --regression -I$(srcdir)/../../include -I$(srcdir) # Files that most or all ecpg preprocessor test outputs depend on ECPG_TEST_DEPENDENCIES = ../../preproc/ecpg$(X) \ $(srcdir)/../regression.h \ $(srcdir)/../../include/sqlca.h \ $(srcdir)/../../include/sqlda.h \ $(srcdir)/../../include/sqltypes.h \ $(srcdir)/../../include/sql3types.h %.c: %.pgc $(ECPG_TEST_DEPENDENCIES) $(ECPG) -o $@ $< Now, the fine gmake manual quoth: `%' in a prerequisite of a pattern rule stands for the same stem that was matched by the `%' in the target. In order for the pattern rule to apply, its target pattern must match the file name under consideration and all of its prerequisites (after pattern substitution) must name files that exist or can be made. So the problem is that (after make clean) ../../preproc/ecpg doesn't exist, and the Makefiles under test/ have no idea how to build it, and thus this pattern rule is inapplicable. Thus you end up getting "No rule to make target" errors. While it seems straightforward to fix this for "make check" scenarios --- just go make ecpg before trying to make the tests --- I think these rules are very surprising for "make installcheck" cases. You'd expect "make installcheck" to test the installed ecpg, as indeed Alexander pointed out at the start of the thread. But it's not doing that. Alexander's USE_INSTALLED_ASSETS patch attempted to fix that, and I now see the point of it, but it still seems pretty hacky and invasive (why should an ecpg-only problem be touching stuff outside ecpg)? Also, unless I'm still missing something, it doesn't fix the problem with "make clean check": ecpg won't have been built before we try to build the test programs. I'd suggest trying to simplify the USE_INSTALLED_ASSETS patch so it doesn't muck with the rules for building pg_regress. I don't find that to be very relevant to the problem. regards, tom lane
31.07.2018 01:16, Tom Lane wrote:
I believe that `installed_instance_path\bin\pg_config --configure` can show the arguments, which can be used to perform ./configure and then make installcheck for binary packages.Alexander Lakhin <exclusion@gmail.com> writes:14.07.2018 13:57, Peter Eisentraut wrote:On 06.07.18 09:45, Alexander Lakhin wrote:./configure --enable-tap-tests make install make install -C contrib chown -R postgres:postgres /usr/local/pgsql/ /usr/local/pgsql/bin/initdb -D /usr/local/pgsql/data /usr/local/pgsql/bin/pg_ctl start -l logfile -D /usr/local/pgsql/data /make clean/ # Also you can just install binary packages to get the same state. make installcheck-world # This check fails.I remain pretty skeptical that this is a sensible way to proceed, especially not if what you're testing is installed binary packages. You're creating yet *another* hazard for version-skew-like problems, namely that there's no certainty that you gave configure arguments that're compatible with what the installed packages used.
I understand that it should be done on the same platform and with exactly the same PG version, but I think it's the only right way to check the binaries (to perform user-centric testing).
Best regards,
------
Alexander Lakhin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Yes, it's exactly the problem I was trying to fix.I wrote:The original complaint about ecpg remains; I'm not terribly excited about messing with that.Well, I got curious as to why we were seeing such a weird error, and eventually traced it to this stuff in ecpg/test/Makefile.regress: # Standard way to invoke the ecpg preprocessor ECPG = ../../preproc/ecpg --regression -I$(srcdir)/../../include -I$(srcdir) # Files that most or all ecpg preprocessor test outputs depend on ECPG_TEST_DEPENDENCIES = ../../preproc/ecpg$(X) \$(srcdir)/../regression.h \$(srcdir)/../../include/sqlca.h \$(srcdir)/../../include/sqlda.h \$(srcdir)/../../include/sqltypes.h \$(srcdir)/../../include/sql3types.h %.c: %.pgc $(ECPG_TEST_DEPENDENCIES)$(ECPG) -o $@ $< Now, the fine gmake manual quoth: `%' in a prerequisite of a pattern rule stands for the same stem that was matched by the `%' in the target. In order for the pattern rule to apply, its target pattern must match the file name under consideration and all of its prerequisites (after pattern substitution) must name files that exist or can be made. So the problem is that (after make clean) ../../preproc/ecpg doesn't exist, and the Makefiles under test/ have no idea how to build it, and thus this pattern rule is inapplicable. Thus you end up getting "No rule to make target" errors.
In fact, after fixing ECPG usage with installcheck, I found that "make installcheck" then rebuilds libpostgres, libpgport and libpq (for installcheck-world). (I mentioned it in this thread before.) Later I proposed a more comprehensive patch that allows us to make installcheck cleanly (without building something).While it seems straightforward to fix this for "make check" scenarios --- just go make ecpg before trying to make the tests --- I think these rules are very surprising for "make installcheck" cases. You'd expect "make installcheck" to test the installed ecpg, as indeed Alexander pointed out at the start of the thread. But it's not doing that. Alexander's USE_INSTALLED_ASSETS patch attempted to fix that, and I now see the point of it, but it still seems pretty hacky and invasive (why should an ecpg-only problem be touching stuff outside ecpg)? Also, unless I'm still missing something, it doesn't fix the problem with "make clean check": ecpg won't have been built before we try to build the test programs.
So the problem is not ecpg-only, the ecpg's failure to build is prominent part of it, but there are another assets getting built under the hood.
I can simplify the patch to fix only the ECPG failure, and then prepare a distinct patch for libs, if it makes sense.I'd suggest trying to simplify the USE_INSTALLED_ASSETS patch so it doesn't muck with the rules for building pg_regress. I don't find that to be very relevant to the problem.
Best regards,
------
Alexander Lakhin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Alexander Lakhin <exclusion@gmail.com> writes: > 31.07.2018 02:42, Tom Lane wrote: >> Alexander's USE_INSTALLED_ASSETS patch attempted to fix that, and I now >> see the point of it, but it still seems pretty hacky and invasive (why >> should an ecpg-only problem be touching stuff outside ecpg)? Also, >> unless I'm still missing something, it doesn't fix the problem with >> "make clean check": ecpg won't have been built before we try to build the >> test programs. > In fact, after fixing ECPG usage with installcheck, I found that "make > installcheck" then rebuilds libpostgres, libpgport and libpq (for > installcheck-world). (I mentioned it in this thread before.) Later I > proposed a more comprehensive patch that allows us to make installcheck > cleanly (without building something). > So the problem is not ecpg-only, the ecpg's failure to build is > prominent part of it, but there are another assets getting built under > the hood. Well, there's a lot of moving parts here, and it's not clear to me what makes the most sense. We potentially have the following things that ECPG "make check" generates or references in the build tree, but we might wish for "make installcheck" to use preinstalled versions of instead: 1. Server executables and other "installed" server support files. 2. PG cluster (datadir and running server). 3. pg_regress and libraries it depends on, such as libpq. 4. ecpg preprocessor. 5. ecpg's exported C header files, needed to compile test programs. 6. ecpg's libraries, used by test programs; also libpq. Currently, installcheck references an existing running cluster (#2) and therefore a fortiori #1 as well. That's fine. I believe we reference a local pg_regress and libraries (#3) in both cases, and that's also fine, because we don't install pg_regress at all. (Well, PGXS does, but ecpg tests shouldn't rely on that.) So it boils down to what to do about #4/#5/#6. I suppose we could define "make installcheck" as referring only to using an installed server, not any ecpg support, in which case we don't need any big surgery to the ecpg makefiles. I'm not sure how consistent a definition that is, but maybe it makes sense by analogy to pg_regress. Another point here is that if you did "make check" and then "make installcheck", it presumably would not rebuild the test programs, meaning they'd still have been built using the local resources not the installed ones (or vice versa for the other order). So there's also a difference between "build" and "test execution", which seems relevant, and it's one we don't really have any make-target nomenclature to distinguish. Given that you'd expect "make all" to use local copies of the build resources, perhaps there should be a separate target named along the lines of "make tests-from-install" that uses installed ecpg+other resources to compile the test programs. Anyway, as long as the installcheck target is dependent on "all" not something else, maybe it's fine as is. I'm definitely not sure what would be a consistent design for doing it differently. regards, tom lane
31.07.2018 22:01, Tom Lane wrote:
I would split 3 to:Well, there's a lot of moving parts here, and it's not clear to me what makes the most sense. We potentially have the following things that ECPG "make check" generates or references in the build tree, but we might wish for "make installcheck" to use preinstalled versions of instead: 1. Server executables and other "installed" server support files. 2. PG cluster (datadir and running server). 3. pg_regress and libraries it depends on, such as libpq. 4. ecpg preprocessor. 5. ecpg's exported C header files, needed to compile test programs. 6. ecpg's libraries, used by test programs; also libpq. Currently, installcheck references an existing running cluster (#2) and therefore a fortiori #1 as well. That's fine. I believe we reference a local pg_regress and libraries (#3) in both cases, and that's also fine, because we don't install pg_regress at all. (Well, PGXS does, but ecpg tests shouldn't rely on that.) So it boils down to what to do about #4/#5/#6.
3. client libraries, such as libpq.
7. pg_regress.
I missed the fact that pg_regress is installed in lib/pgxs/src/test/regress/, and now it raises a question for me - why this pg_regress instance should be used only for PGXS tests?
Leaving the question aside, I propose the fix to use the preinstalled versions of 3-6.
Yes, this is important question and I have no complete answer for now, but maybe these test programs could be built as temporary?I suppose we could define "make installcheck" as referring only to using an installed server, not any ecpg support, in which case we don't need any big surgery to the ecpg makefiles. I'm not sure how consistent a definition that is, but maybe it makes sense by analogy to pg_regress. Another point here is that if you did "make check" and then "make installcheck", it presumably would not rebuild the test programs, meaning they'd still have been built using the local resources not the installed ones (or vice versa for the other order). So there's also a difference between "build" and "test execution", which seems relevant, and it's one we don't really have any make-target nomenclature to distinguish.
If I understand you correctly, the attached patch does just this.Given that you'd expect "make all" to use local copies of the build resources, perhaps there should be a separate target named along the lines of "make tests-from-install" that uses installed ecpg+other resources to compile the test programs.
To illustrate it I also attached the log of "make -C src/interfaces/ecpg; make installcheck" and the log of "make installcheck" with patch applied (both procedures executed immediately after "make clean").
I would propose the following design for "make installcheck":Anyway, as long as the installcheck target is dependent on "all" not something else, maybe it's fine as is. I'm definitely not sure what would be a consistent design for doing it differently.
1. For every thing that has installed version, use it.
2. For every thing that has no installed version and is not a subject to build (e.g. .../t/*.sql), use it's version in the source tree.
3. For every thing that has no installed version and should be built (e.g. src/interfaces/ecpg/test/connect/*.pgc), build one-time version and use it.
Regarding to ecpg tests it means that we should use installed ecpg, installed libs, installed *.h, and compile src/interfaces/ecpg/test/connect/*.pgc into a temporary binary.
Maybe I miss some use cases, where existing design suits better, but in general I think that the more test coverage (more installed things checked), the better.
Best regards,
------
Alexander Lakhin
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachment
On Mon, May 07, 2018 at 01:07:15PM -0400, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> After thinking about this some more, I think the question here is >> definitional. A first attempt at defining 'make installcheck' is to >> say that it runs the tests from the build tree against the running >> server. Certainly, we intend to use the SQL files that are present in >> the build tree, not the ones that were present in the build tree where >> the running server was built. But what about client programs that we >> use to connect to the server? You're suggesting that we use the >> pre-installed ones, but that is actually pretty problematic because >> the ones we see as installed might correspond neither to the contents >> of the build tree nor to the running server. Conceivably we could end >> up having a mix of assets from three different places: (1) the running >> server, (2) the build tree, (3) whatever is in our path at the moment. >> That seems very confusing. So now I think it's probably right to >> define 'make installcheck' as using the assets from the build tree to >> test the running server. Under that definition, we're missing some >> dependencies, but USE_INSTALLED_ASSETS isn't a thing we need. > > Nah, I disagree with this. To me, the purpose of "make installcheck" > is to verify the correctness of an installation, which I take to include > the client programs as well as the server. I think that "make > installcheck" ought to use the installed version of any file that we > actually install, and go to the build tree only for things we don't > install (e.g. SQL test scripts). > > If the user has screwed up his PATH or other environmental aspects so that > what he's testing isn't a single installation, that's his error, not > something that "make installcheck" ought to work around. Indeed, maybe > such aspects of his setup are intentional, and second-guessing them would > completely defeat his purpose. In any case, if you want to test the > build-tree assets, that's what "make check" is for. I agree with Tom's position, and this is the behavior that Postgres is using for ages for check and installcheck. If there are no objections, I'll mark the patch as rejected and move on to other things. -- Michael
Attachment
Hello Michael, 12.09.2018 10:20, Michael Paquier wrote: > On Mon, May 07, 2018 at 01:07:15PM -0400, Tom Lane wrote: >> Robert Haas <robertmhaas@gmail.com> writes: >>> After thinking about this some more, I think the question here is >>> definitional. A first attempt at defining 'make installcheck' is to >>> say that it runs the tests from the build tree against the running >>> server. Certainly, we intend to use the SQL files that are present in >>> the build tree, not the ones that were present in the build tree where >>> the running server was built. But what about client programs that we >>> use to connect to the server? You're suggesting that we use the >>> pre-installed ones, but that is actually pretty problematic because >>> the ones we see as installed might correspond neither to the contents >>> of the build tree nor to the running server. Conceivably we could end >>> up having a mix of assets from three different places: (1) the running >>> server, (2) the build tree, (3) whatever is in our path at the moment. >>> That seems very confusing. So now I think it's probably right to >>> define 'make installcheck' as using the assets from the build tree to >>> test the running server. Under that definition, we're missing some >>> dependencies, but USE_INSTALLED_ASSETS isn't a thing we need. >> Nah, I disagree with this. To me, the purpose of "make installcheck" >> is to verify the correctness of an installation, which I take to include >> the client programs as well as the server. I think that "make >> installcheck" ought to use the installed version of any file that we >> actually install, and go to the build tree only for things we don't >> install (e.g. SQL test scripts). >> >> If the user has screwed up his PATH or other environmental aspects so that >> what he's testing isn't a single installation, that's his error, not >> something that "make installcheck" ought to work around. Indeed, maybe >> such aspects of his setup are intentional, and second-guessing them would >> completely defeat his purpose. In any case, if you want to test the >> build-tree assets, that's what "make check" is for. > I agree with Tom's position, and this is the behavior that Postgres is > using for ages for check and installcheck. If there are no objections, > I'll mark the patch as rejected and move on to other things. > -- > Michael It seems, that you miss a major part of the discussion (we have discussed the issue from other positions later). And I don't think that age of the behavior should prevail over it's reasonability. Best regards, ------ Alexander Lakhin Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Alexander Lakhin <exclusion@gmail.com> writes: > 12.09.2018 10:20, Michael Paquier wrote: >> On Mon, May 07, 2018 at 01:07:15PM -0400, Tom Lane wrote: >>> Nah, I disagree with this. To me, the purpose of "make installcheck" >>> is to verify the correctness of an installation, which I take to include >>> the client programs as well as the server. I think that "make >>> installcheck" ought to use the installed version of any file that we >>> actually install, and go to the build tree only for things we don't >>> install (e.g. SQL test scripts). >> I agree with Tom's position, and this is the behavior that Postgres is >> using for ages for check and installcheck. If there are no objections, >> I'll mark the patch as rejected and move on to other things. > It seems, that you miss a major part of the discussion (we have > discussed the issue from other positions later). I think the main reason this patch isn't moving forward is that it's not clear to most people what you're trying to fix. And I'd lay a big part of the blame for that on the fact that the patch includes no documentation changes at all, not even additional Makefile comments. Perhaps the SGML text about how to use "make installcheck" needs to be expanded to clarify what we expect to be already installed. The patch itself contains some pretty dubious/confusing things too. For instance, the first hunk: @@ -244,7 +244,13 @@ CPPFLAGS = @CPPFLAGS@ override CPPFLAGS := $(ICU_CFLAGS) $(CPPFLAGS) +ifdef USE_INSTALLED_ASSETS +USE_INCLUDEDIR = 1 +endif ifdef PGXS +USE_INCLUDEDIR = 1 +endif +ifdef USE_INCLUDEDIR override CPPFLAGS := -I$(includedir_server) -I$(includedir_internal) $(CPPFLAGS) else # not PGXS override CPPFLAGS := -I$(top_srcdir)/src/include $(CPPFLAGS) immediately leads the reader to wonder where else USE_INCLUDEDIR is used, and the answer is nowhere, so why'd you define it? I think it'd be better to replace the first seven lines with the usual locution for OR: ifneq (,$(PGXS)$(USE_INSTALLED_ASSETS)) and likewise further down for USE_LIBDIR. I also note you didn't fix the comment you falsified about "# not PGXS". It seems wrong to have changed src/interfaces/ecpg/Makefile the way you did. Surely it is the responsibility of src/interfaces/ecpg/test/Makefile to handle a "make installcheck" request correctly, whether it is issued from the parent directory or locally. (Personally I do "make installcheck" in the test/ subdirectory quite often, so I'd be unhappy if it doesn't work right when started from there.) The change in pgxs.mk doesn't make a lot of sense to me either; even if it's not actually syntactically wrong, what's the point given what you did in Makefile.global? I do not believe that the changes in either plpgsql/src or test/isolation are appropriate. If the former is needed then it would at least imply that plperl, plpython, and pltcl are all broken too, and probably also that all the contrib makefiles are broken, and I don't believe any of that. As for test/isolation, there is nothing that it installs, so why would it need a change in behavior? Nor do I understand the need for changes in test/regress. I'm prepared to believe that ECPG might need some work, because it's got a complicated situation and few people pay any attention to it anyway. But all this other stuff works perfectly fine under "make installcheck" today, and has done for years, and you have not explained why changing it would be an improvement. regards, tom lane
Hello Tom, Thank you for the very detailed review. Please look at the improved version of the patch (and the `make installcheck` log with it). 16.11.2018 04:19, Tom Lane writes: >> It seems, that you miss a major part of the discussion (we have >> discussed the issue from other positions later). > I think the main reason this patch isn't moving forward is that it's not > clear to most people what you're trying to fix. And I'd lay a big part of > the blame for that on the fact that the patch includes no documentation > changes at all, not even additional Makefile comments. Perhaps the SGML > text about how to use "make installcheck" needs to be expanded to clarify > what we expect to be already installed. Now I've tried to describe in regress.sgml the behavior, which I want to implement. I'm not sure that we should name all the assets, that we expect to be installed, because it can change. > The patch itself contains some pretty dubious/confusing things too. > For instance, the first hunk: > > @@ -244,7 +244,13 @@ CPPFLAGS = @CPPFLAGS@ > > override CPPFLAGS := $(ICU_CFLAGS) $(CPPFLAGS) > > +ifdef USE_INSTALLED_ASSETS > +USE_INCLUDEDIR = 1 > +endif > ifdef PGXS > +USE_INCLUDEDIR = 1 > +endif > +ifdef USE_INCLUDEDIR > override CPPFLAGS := -I$(includedir_server) -I$(includedir_internal) $(CPPFLAGS) > else # not PGXS > override CPPFLAGS := -I$(top_srcdir)/src/include $(CPPFLAGS) > > immediately leads the reader to wonder where else USE_INCLUDEDIR is used, > and the answer is nowhere, so why'd you define it? I think it'd be better > to replace the first seven lines with the usual locution for OR: > > ifneq (,$(PGXS)$(USE_INSTALLED_ASSETS)) > > and likewise further down for USE_LIBDIR. I also note you didn't fix the > comment you falsified about "# not PGXS". Thanks for the notice. I fixed it. > It seems wrong to have changed src/interfaces/ecpg/Makefile the way you > did. Surely it is the responsibility of src/interfaces/ecpg/test/Makefile > to handle a "make installcheck" request correctly, whether it is issued > from the parent directory or locally. (Personally I do "make > installcheck" in the test/ subdirectory quite often, so I'd be unhappy > if it doesn't work right when started from there.) Thanks again, I've moved the change into ecpg/test. > The change in pgxs.mk doesn't make a lot of sense to me either; even if it's > not actually syntactically wrong, what's the point given what you did in > Makefile.global? I've modified the patch to use the installed version of pg_regress. It simplifies a lot. The main idea of the change is to not build pg_regress. > I do not believe that the changes in either plpgsql/src or test/isolation > are appropriate. If the former is needed then it would at least imply > that plperl, plpython, and pltcl are all broken too, and probably also > that all the contrib makefiles are broken, and I don't believe any of > that. As for test/isolation, there is nothing that it installs, so why > would it need a change in behavior? Regarding test/isolation, we need to change the behavior to build pg_isolation_regress and isolationtester only. We don't need to build all the libraries needed as they are already installed in $(DESTDIR)/lib/. By the way, I wonder why "pg_isolation_regress" and "isolationtester" binaries are not installed as "pg_regress" is. As to plpgsql and other PLs, yes, it was my omission. We don't need to build pg_regress when we use $(pg_regress_installcheck) for any of these, In contrib/ I've found only one usage of pg_regress_installcheck (contrib/test_decoding). Fixed it too. We don't need to build the test-decoding lib as it's installed too. Also there is an issue with src/test/regress/. We install refint.so and autoinc.so into $(DESTDIR)/lib/, but don't install regress.so. It makes impossible to pass "--dlpath='$(DESTDIR)$(libdir)/'" to pg_regress. So for now I leave dlpath unchanged and it requires building of all the .so's. > Nor do I understand the need for changes in test/regress. I'm prepared to > believe that ECPG might need some work, because it's got a complicated > situation and few people pay any attention to it anyway. But all this > other stuff works perfectly fine under "make installcheck" today, and > has done for years, and you have not explained why changing it would > be an improvement. The main purpose of the change is to make it possible to perform `make installcheck` for the installation produced by the binary packages (pgdg, for example). The scenario is simple. If I've installed binary packages, how can I check that the installation is working as expected? (Assuming that I have corresponding source tarball.) Currently, I can remove bin/ecpg, lib/libgport, lib/pgxs/, include/ from the installation directory and `make installcheck-world` will succeed. But it will not, if I remove bin/psql or lib/adminpack.so. So the current behavior is at least inconsistent but moreover it's unsuitable for the user-centric testing of installation. I do understand that the developer-centric approach was working for years, but may be it can (or should) be changed someday? Best regards, Alexander
Attachment
> On Sun, Nov 18, 2018 at 8:31 AM Alexander Lakhin <exclusion@gmail.com> wrote: > > I've modified the patch to use the installed version of pg_regress. It > simplifies a lot. The main idea of the change is to not build pg_regress. Hi, I've noticed that for this patch cfbot show strange error USE_INSTALLED_ASSETS=1 make all make[2]: Entering directory `/home/travis/build/postgresql-cfbot/postgresql/src/test/regress' make -C ../../../contrib/spi make[3]: Entering directory `/home/travis/build/postgresql-cfbot/postgresql/contrib/spi' make[3]: Nothing to be done for `all'. make[3]: Leaving directory `/home/travis/build/postgresql-cfbot/postgresql/contrib/spi' make[2]: Leaving directory `/home/travis/build/postgresql-cfbot/postgresql/src/test/regress' rm -rf ./testtablespace mkdir ./testtablespace '/usr/local/pgsql/lib/pgxs/src/test/regress/pg_regress' --inputdir=. --bindir='/usr/local/pgsql/bin' --bindir='/home/travis/build/postgresql-cfbot/postgresql/src/bin/pg_upgrade/tmp_check/install//usr/local/pgsql/bin' --port=54464 --dlpath=. --max-concurrent-tests=20 --bindir='/home/travis/build/postgresql-cfbot/postgresql/src/bin/pg_upgrade/tmp_check/install//usr/local/pgsql/bin' --port=54464 --schedule=./parallel_schedule make[1]: /usr/local/pgsql/lib/pgxs/src/test/regress/pg_regress: Command not found make[1]: *** [installcheck-parallel] Error 127 make[1]: Leaving directory `/home/travis/build/postgresql-cfbot/postgresql/src/test/regress' make: *** [installcheck-parallel] Error 2 make: Leaving directory `/home/travis/build/postgresql-cfbot/postgresql' The same error I've got when tried to run it on my laptop, is it something expected?
Hello, 30.11.2018 23:59, Dmitry Dolgov wrote: >> On Sun, Nov 18, 2018 at 8:31 AM Alexander Lakhin <exclusion@gmail.com> wrote: >> >> I've modified the patch to use the installed version of pg_regress. It >> simplifies a lot. The main idea of the change is to not build pg_regress. > Hi, > > I've noticed that for this patch cfbot show strange error > > USE_INSTALLED_ASSETS=1 make all > ... > make[1]: Leaving directory > `/home/travis/build/postgresql-cfbot/postgresql/src/test/regress' > make: *** [installcheck-parallel] Error 2 > make: Leaving directory `/home/travis/build/postgresql-cfbot/postgresql' > > The same error I've got when tried to run it on my laptop, is it something > expected? Thank you for bringing this to my attention. I've fixed the patch. (pg_upgrade/test.sh passes custom DESTDIR to `make` but the same DESTDIR is needed for `make install`) Best regards, Alexander
Attachment
Hello, 01.12.2018 09:12, Alexander Lakhin wrote: > 30.11.2018 23:59, Dmitry Dolgov wrote: >> Hi, >> >> I've noticed that for this patch cfbot show strange error >> >> USE_INSTALLED_ASSETS=1 make all > I've fixed the patch. Rebased the patch once more after d3c09b9b. Best regards, Alexander
Attachment
On Mon, Dec 03, 2018 at 11:58:13AM +0300, Alexander Lakhin wrote: > Rebased the patch once more after d3c09b9b. Moved to next CF, waiting on author as the patch conflicts with HEAD. -- Michael
Attachment
On Mon, Dec 03, 2018 at 11:58:13AM +0300, Alexander Lakhin wrote: > Rebased the patch once more after d3c09b9b. The patch is ready for committer, so it has not attracted much attention. Perhaps Teodor or Alexander could look at it? I have moved the patch to next CF with the same status. -- Michael
Attachment
04.02.2019 5:09, Michael Paquier wrote: > On Mon, Dec 03, 2018 at 11:58:13AM +0300, Alexander Lakhin wrote: >> Rebased the patch once more after d3c09b9b. > Moved to next CF, waiting on author as the patch conflicts with HEAD. > -- > Michael Hello Michael, It's very strange, I looked at http://cfbot.cputube.org/next.html from time to time (last time, two days ago) and the patch passed all checks just fine. Also I've tried to apply make-installcheck-v5.patch from https://www.postgresql.org/message-id/dabc52ea-14b6-3109-26b1-e83a9dc62cc0@gmail.com, it applies cleanly. Can you explain, what exactly conflicts with HEAD? Best regards, Alexander
Hi, On 2019-02-04 11:11:03 +0900, Michael Paquier wrote: > On Mon, Dec 03, 2018 at 11:58:13AM +0300, Alexander Lakhin wrote: > > Rebased the patch once more after d3c09b9b. > > The patch is ready for committer, so it has not attracted much > attention. Perhaps Teodor or Alexander could look at it? I'm confused. I don't see the patch as ready, given the discussion, nor does https://commitfest.postgresql.org/22/1672/ contain a record of it ever being set to RFC? Did you intend to reply to a different thread? Greetings, Andres Freund
On Thu, Feb 14, 2019 at 11:40:29AM -0800, Andres Freund wrote: > I'm confused. I don't see the patch as ready, given the discussion, nor > does > https://commitfest.postgresql.org/22/1672/ > contain a record of it ever being set to RFC? Did you intend to reply to > a different thread? Indeed, the status I posted looks incorrect. Thanks for pointing it out! -- Michael
Attachment
Hi Alexander, On 2/18/19 8:07 AM, Michael Paquier wrote: > On Thu, Feb 14, 2019 at 11:40:29AM -0800, Andres Freund wrote: >> I'm confused. I don't see the patch as ready, given the discussion, nor >> does >> https://commitfest.postgresql.org/22/1672/ >> contain a record of it ever being set to RFC? Did you intend to reply to >> a different thread? > > Indeed, the status I posted looks incorrect. Thanks for pointing it > out! It appears you have addressed all or most of the review comments but there seems to be little enthusiasm for this patch. If it doesn't attract more review or a committer during this CF, I think we should mark it as rejected. Regards, -- -David david@pgmasters.net
Hello David, 25.03.2019 10:25, David Steele wrote: > Hi Alexander, > > On 2/18/19 8:07 AM, Michael Paquier wrote: >> On Thu, Feb 14, 2019 at 11:40:29AM -0800, Andres Freund wrote: >>> I'm confused. I don't see the patch as ready, given the discussion, nor >>> does >>> https://commitfest.postgresql.org/22/1672/ >>> contain a record of it ever being set to RFC? Did you intend to >>> reply to >>> a different thread? >> >> Indeed, the status I posted looks incorrect. Thanks for pointing it >> out! > > It appears you have addressed all or most of the review comments but > there seems to be little enthusiasm for this patch. > > If it doesn't attract more review or a committer during this CF, I > think we should mark it as rejected. Please, delay rejecting it till the end of the commitfest, I'll try to find some enthusiasm amongst my colleagues. Best regards, Alexander
On Mon, Mar 25, 2019 at 8:30 PM Alexander Lakhin <exclusion@gmail.com> wrote: > 25.03.2019 10:25, David Steele wrote: > > If it doesn't attract more review or a committer during this CF, I > > think we should mark it as rejected. > Please, delay rejecting it till the end of the commitfest, I'll try to > find some enthusiasm amongst my colleagues. Hi Alexander, A new CF is here and this is in "Needs Review". Would you like to provide a rebased patch, or should it really be withdrawn? Thanks, -- Thomas Munro https://enterprisedb.com
Hello Thomas, 01.07.2019 13:47, Thomas Munro wrote: > > A new CF is here and this is in "Needs Review". Would you like to > provide a rebased patch, or should it really be withdrawn? The rebased patch is attached, but I still can't find anyone interested in reviewing it. So let's withdraw it. With hope for better days, Alexander
Attachment
Alexander Lakhin <exclusion@gmail.com> writes: > 01.07.2019 13:47, Thomas Munro wrote: >> A new CF is here and this is in "Needs Review". Would you like to >> provide a rebased patch, or should it really be withdrawn? > The rebased patch is attached, but I still can't find anyone interested > in reviewing it. > So let's withdraw it. I agree with withdrawing this, and have marked it that way in the CF app. I think the fundamental problem here is that nobody except Alexander is unhappy with the current behavior of "make installcheck", and it is not very clear whether this patch might change the behavior in ways that do make others unhappy. So I think it's best to set it aside pending more people agreeing that there's a problem to solve. regards, tom lane