Обсуждение: pgsql: Remove reset of testtablespace from pg_regress on Windows
Remove reset of testtablespace from pg_regress on Windows testtablespace is an extra path used as tablespace location in the main regression test suite, computed from --outputdir as defined by the caller of pg_regress (current directory if undefined). This special handling was introduced as of f10589e to be specific to MSVC, as we let pg_regress' Makefile handle this cleanup in other environments. This moves the cleanup to the MSVC script running regression tests instead where needed: check, installcheck and upgradecheck. I have also checked this patch on MSVC with repeated runs of each target. Author: Kyotaro Horiguchi, Michael Paquier Discussion: https://postgr.es/m/20200219.142519.437573253063431435.horikyota.ntt@gmail.com Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/2b2a070d98b2f2c7ecc031e582cfefa400316ce3 Modified Files -------------- src/test/regress/pg_regress.c | 22 ---------------------- src/tools/msvc/vcregress.pl | 17 +++++++++++++++-- 2 files changed, 15 insertions(+), 24 deletions(-)
On 6/17/20 9:40 PM, Michael Paquier wrote: > Remove reset of testtablespace from pg_regress on Windows > > > This patch has carefully removed the ability to run the regression tests as a Windows administrative user, as I just discovered. This was the whole point of commit ce5d3424d6. I assume the testing referred to above was not as a privileged user. I think this should be reverted. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Jul 09, 2020 at 07:28:02PM -0400, Andrew Dunstan wrote: > This patch has carefully removed the ability to run the regression tests > as a Windows administrative user, as I just discovered. This was the > whole point of commit ce5d3424d6. > > I assume the testing referred to above was not as a privileged user. I > think this should be reverted. Thanks Andrew. This was discussed on the original thread and what I wanted to do a rvert if you look at its newest history: https://www.postgresql.org/message-id/20200623014036.GF50978@paquier.xyz And then, the thread just stalled.. So I was not sure if something was actually wanted or not. Now, I don't think that just a simple revert is the best answer we can provide. Just look at this comment in pg_regress.c that does not give a hint that we actually should not remove this code: - * On Windows only, clean out the test tablespace dir, or create it if it - * doesn't exist. On other platforms we expect the Makefile to take care - * of that. (We don't migrate that functionality in here because it'd be - * harder to cope with platform-specific issues such as SELinux.) - * - * XXX it would be better if pg_regress.c had nothing at all to do with - * testtablespace, and this were handled by a .BAT file or similar on - * Windows. See pgsql-hackers discussion of 2008-01-18. So instead I would like to propose the attached, reworking this comment as follows (basically a revert, except for this comment): + /* + * On Windows only, clean out the test tablespace dir, or create it if it + * doesn't exist so as it is possible to run the regression tests as a + * Windows administrative user account with the restricted token obtained + * when starting pg_regress. On other platforms we expect the Makefile + * to take care of that. + */ What do you think? -- Michael
Вложения
On 7/9/20 9:02 PM, Michael Paquier wrote: > On Thu, Jul 09, 2020 at 07:28:02PM -0400, Andrew Dunstan wrote: >> This patch has carefully removed the ability to run the regression tests >> as a Windows administrative user, as I just discovered. This was the >> whole point of commit ce5d3424d6. >> >> I assume the testing referred to above was not as a privileged user. I >> think this should be reverted. > Thanks Andrew. This was discussed on the original thread and what I > wanted to do a rvert if you look at its newest history: > https://www.postgresql.org/message-id/20200623014036.GF50978@paquier.xyz > And then, the thread just stalled.. So I was not sure if something > was actually wanted or not. > > Now, I don't think that just a simple revert is the best answer we can > provide. Just look at this comment in pg_regress.c that does not give > a hint that we actually should not remove this code: > - * On Windows only, clean out the test tablespace dir, or create it if it > - * doesn't exist. On other platforms we expect the Makefile to take care > - * of that. (We don't migrate that functionality in here because it'd be > - * harder to cope with platform-specific issues such as SELinux.) > - * > - * XXX it would be better if pg_regress.c had nothing at all to do with > - * testtablespace, and this were handled by a .BAT file or similar on > - * Windows. See pgsql-hackers discussion of 2008-01-18. > > So instead I would like to propose the attached, reworking this > comment as follows (basically a revert, except for this comment): > + /* > + * On Windows only, clean out the test tablespace dir, or create it if it > + * doesn't exist so as it is possible to run the regression tests as a > + * Windows administrative user account with the restricted token obtained > + * when starting pg_regress. On other platforms we expect the Makefile > + * to take care of that. > + */ > > What do you think? I certainly agree we should document more clearly why it's there, to help forestall anyone else who comes along and thinks it would just be neater to remove it. so +1. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Jul 09, 2020 at 10:21:57PM -0400, Andrew Dunstan wrote: > I certainly agree we should document more clearly why it's there, to > help forestall anyone else who comes along and thinks it would just be > neater to remove it. so +1. Okay. Done, then. -- Michael