Обсуждение: Create shorthand for including all extra tests
Hi, I realized that I forgot to add the new extra test to my test scripts. So, I thought maybe we can use shorthand for including all extra tests. With that, running a full testsuite is easier without having to keep up with new tests and updates. I created an 'all' option for PG_TEST_EXTRA to enable all test suites defined under PG_TEST_EXTRA. I created the check_extra_tests_enabled() function in the Test/Utils.pm file. This function takes the test's name as an input and checks if PG_TEST_EXTRA contains 'all' or this test's name. I thought another advantage could be that this can be used in CI. But when 'wal_consistency_checking' is enabled, CI times get longer since it does resource intensive operations. Any kind of feedback would be appreciated. Regards, Nazir Bilal Yavuz Microsoft
Вложения
Nazir Bilal Yavuz <byavuz81@gmail.com> writes:
> I created an 'all' option for PG_TEST_EXTRA to enable all test suites
> defined under PG_TEST_EXTRA.
I think this is a seriously bad idea. The entire point of not including
certain tests in check-world by default is that the omitted tests are
security hazards, so a developer or buildfarm owner should review each
one before deciding whether to activate it on their machine.
regards, tom lane
> On 4 Sep 2023, at 17:01, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Nazir Bilal Yavuz <byavuz81@gmail.com> writes: >> I created an 'all' option for PG_TEST_EXTRA to enable all test suites >> defined under PG_TEST_EXTRA. > > I think this is a seriously bad idea. The entire point of not including > certain tests in check-world by default is that the omitted tests are > security hazards, so a developer or buildfarm owner should review each > one before deciding whether to activate it on their machine. I dunno, I've certainly managed to not run the tests I hoped to multiple times. I think it could be useful for sandboxed testrunners which are destroyed after each run. There is for sure a foot-gun angle to it, no question about that. -- Daniel Gustafsson
On Mon, Sep 04, 2023 at 08:16:44PM +0200, Daniel Gustafsson wrote: > > On 4 Sep 2023, at 17:01, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Nazir Bilal Yavuz <byavuz81@gmail.com> writes: > >> I created an 'all' option for PG_TEST_EXTRA to enable all test suites > >> defined under PG_TEST_EXTRA. > > > > I think this is a seriously bad idea. The entire point of not including > > certain tests in check-world by default is that the omitted tests are > > security hazards, so a developer or buildfarm owner should review each > > one before deciding whether to activate it on their machine. > > I dunno, I've certainly managed to not run the tests I hoped to multiple times. > I think it could be useful for sandboxed testrunners which are destroyed after > each run. There is for sure a foot-gun angle to it, no question about that. Other than PG_TEST_EXTRA=wal_consistency_checking, they have the same hazard: they treat the loopback interface as private, so anyone with access to loopback interface ports can hijack the test. I'd be fine with e.g. PG_TEST_EXTRA=private-lo activating all of those. We don't gain by inviting the tester to review the tests to rediscover this common factor.
Noah Misch <noah@leadboat.com> writes:
> On Mon, Sep 04, 2023 at 08:16:44PM +0200, Daniel Gustafsson wrote:
>> On 4 Sep 2023, at 17:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> I think this is a seriously bad idea. The entire point of not including
>>> certain tests in check-world by default is that the omitted tests are
>>> security hazards, so a developer or buildfarm owner should review each
>>> one before deciding whether to activate it on their machine.
> Other than PG_TEST_EXTRA=wal_consistency_checking, they have the same hazard:
> they treat the loopback interface as private, so anyone with access to
> loopback interface ports can hijack the test. I'd be fine with e.g.
> PG_TEST_EXTRA=private-lo activating all of those. We don't gain by inviting
> the tester to review the tests to rediscover this common factor.
Yeah, I could live with something like that from the security standpoint.
Not sure if it helps Nazir's use-case though. Maybe we could invent
categories that can be used in place of individual test names?
For now,
PG_TEST_EXTRA="needs-private-lo slow"
would cover the territory of "all", and I think it'd be very seldom
that we'd have to invent new categories here (though maybe I lack
imagination today).
regards, tom lane
On Mon, Sep 04, 2023 at 04:30:31PM -0400, Tom Lane wrote: > Noah Misch <noah@leadboat.com> writes: > > On Mon, Sep 04, 2023 at 08:16:44PM +0200, Daniel Gustafsson wrote: > >> On 4 Sep 2023, at 17:01, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >>> I think this is a seriously bad idea. The entire point of not including > >>> certain tests in check-world by default is that the omitted tests are > >>> security hazards, so a developer or buildfarm owner should review each > >>> one before deciding whether to activate it on their machine. > > > Other than PG_TEST_EXTRA=wal_consistency_checking, they have the same hazard: > > they treat the loopback interface as private, so anyone with access to > > loopback interface ports can hijack the test. I'd be fine with e.g. > > PG_TEST_EXTRA=private-lo activating all of those. We don't gain by inviting > > the tester to review the tests to rediscover this common factor. > > Yeah, I could live with something like that from the security standpoint. > Not sure if it helps Nazir's use-case though. Maybe we could invent > categories that can be used in place of individual test names? > For now, > > PG_TEST_EXTRA="needs-private-lo slow" > > would cover the territory of "all", and I think it'd be very seldom > that we'd have to invent new categories here (though maybe I lack > imagination today). I could imagine categories for filesystem bytes and RAM bytes. Also, while needs-private-lo has a bounded definition, "slow" doesn't. If today's one "slow" test increases check-world duration by 1.1x, we may not let a 100x-increase test use the same keyword. If one introduced needs-private-lo, the present spelling of "all" would be "needs-private-lo wal_consistency_checking". Looks okay to me. Doing nothing here wouldn't be ruinous, of course.
Hi, Thanks for the feedback! I updated the patch, 'needs-private-lo' option enables kerberos, ldap, load_balance and ssl extra tests now. > On Mon, Sep 04, 2023 at 04:30:31PM -0400, Tom Lane wrote: > > Yeah, I could live with something like that from the security standpoint. > > Not sure if it helps Nazir's use-case though. Maybe we could invent > > categories that can be used in place of individual test names? > > For now, Yes, that is not ideal for my use-case but still better. On Tue, 5 Sept 2023 at 00:09, Noah Misch <noah@leadboat.com> wrote: > > I could imagine categories for filesystem bytes and RAM bytes. Also, while > needs-private-lo has a bounded definition, "slow" doesn't. If today's one > "slow" test increases check-world duration by 1.1x, we may not let a > 100x-increase test use the same keyword. I agree. I didn't create a new category as 'slow' but still open to suggestions. I am not very familiar with perl syntax, I would like to hear your opinions on how the implementation of the check_extra_text_enabled() function could be done better. Regards, Nazir Bilal Yavuz Microsoft
Вложения
> On 4 Sep 2023, at 23:09, Noah Misch <noah@leadboat.com> wrote: > I could imagine categories for filesystem bytes and RAM bytes. Also, while > needs-private-lo has a bounded definition, "slow" doesn't. If today's one > "slow" test increases check-world duration by 1.1x, we may not let a > 100x-increase test use the same keyword. Agreed, the names should be descriptive enough to contain a boundary. Any new test which is orders of magnitude slower than an existing test suite most likely will have one/more boundary characteristics not shared with existing suites. The test in 20210423204306.5osfpkt2ggaedyvy@alap3.anarazel.de for autovacuum wraparound comes to mind as one that would warrant a new category. > If one introduced needs-private-lo, the present spelling of "all" would be > "needs-private-lo wal_consistency_checking". I think it makes sense to invent a new PG_TEST_EXTRA category which (for now) only contains wal_consistency_checking to make it consistent, such that "all" can be achieved by a set of categories. -- Daniel Gustafsson
On 04.09.23 22:30, Tom Lane wrote: > Noah Misch <noah@leadboat.com> writes: >> On Mon, Sep 04, 2023 at 08:16:44PM +0200, Daniel Gustafsson wrote: >>> On 4 Sep 2023, at 17:01, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>>> I think this is a seriously bad idea. The entire point of not including >>>> certain tests in check-world by default is that the omitted tests are >>>> security hazards, so a developer or buildfarm owner should review each >>>> one before deciding whether to activate it on their machine. > >> Other than PG_TEST_EXTRA=wal_consistency_checking, they have the same hazard: >> they treat the loopback interface as private, so anyone with access to >> loopback interface ports can hijack the test. I'd be fine with e.g. >> PG_TEST_EXTRA=private-lo activating all of those. We don't gain by inviting >> the tester to review the tests to rediscover this common factor. > > Yeah, I could live with something like that from the security standpoint. > Not sure if it helps Nazir's use-case though. Maybe we could invent > categories that can be used in place of individual test names? > For now, > > PG_TEST_EXTRA="needs-private-lo slow" > > would cover the territory of "all", and I think it'd be very seldom > that we'd have to invent new categories here (though maybe I lack > imagination today). At least the kerberos tests also appear to require a lot of randomness for their setup, and sometimes in VM environments they hang for minutes until they get that. I suppose that would go under "slow". Also, at least in my mind, when we added the kerberos and ldap tests, a partial reason for excluding them from the default run was "requires additional unusual software to be installed". The additional kerberos and ldap server software used in those tests is not covered by configure/meson, so it's a bit more DIY.
On 05.09.23 19:26, Nazir Bilal Yavuz wrote: > Thanks for the feedback! I updated the patch, 'needs-private-lo' > option enables kerberos, ldap, load_balance and ssl extra tests now. As was discussed, I don't think "needs private lo" is the only condition for these tests. At least kerberos and ldap also need extra software installed, and load_balance might need editing the system's hosts file. So someone would still need to familiarize themselves with these tests individually before setting a global option like this. Also, if we were to create test groupings like this, I think the implementation should be different. The way you have it, there is a sort of central registry of all affected tests in src/test/perl/PostgreSQL/Test/Utils.pm and a mapping of groups to tests. I would prefer a more decentralized approach where each test decides on its own whether to run, with pseudo-code conditionals like if (!(PG_TEST_EXTRA contains "ldap" or PG_TEST_EXTRA contains "needs-private-lo")) skip_all Anyway, at the moment, I don't see a sensible way to group these things beyond what we have now (effectively, "ldap" is already a group, because it affects more than one test suite). Right now, we have six possible values, which is probably just about doable to keep track of manually. If we get a lot more, then we need to look into this again, but maybe then we'll also have more patterns to group things around.
Hi, On Wed, 10 Jan 2024 at 23:48, Peter Eisentraut <peter@eisentraut.org> wrote: > > On 05.09.23 19:26, Nazir Bilal Yavuz wrote: > > Thanks for the feedback! I updated the patch, 'needs-private-lo' > > option enables kerberos, ldap, load_balance and ssl extra tests now. > > As was discussed, I don't think "needs private lo" is the only condition > for these tests. At least kerberos and ldap also need extra software > installed, and load_balance might need editing the system's hosts file. > So someone would still need to familiarize themselves with these tests > individually before setting a global option like this. > > Also, if we were to create test groupings like this, I think the > implementation should be different. The way you have it, there is a > sort of central registry of all affected tests in > src/test/perl/PostgreSQL/Test/Utils.pm and a mapping of groups to tests. > I would prefer a more decentralized approach where each test decides > on its own whether to run, with pseudo-code conditionals like > > if (!(PG_TEST_EXTRA contains "ldap" or PG_TEST_EXTRA contains > "needs-private-lo")) > skip_all > > Anyway, at the moment, I don't see a sensible way to group these things > beyond what we have now (effectively, "ldap" is already a group, because > it affects more than one test suite). Right now, we have six possible > values, which is probably just about doable to keep track of manually. > If we get a lot more, then we need to look into this again, but maybe > then we'll also have more patterns to group things around. I see your point. It looks like the best option is to reevaluate this if there are more PG_TEST_EXTRA options. -- Regards, Nazir Bilal Yavuz Microsoft
On 15.01.24 09:54, Nazir Bilal Yavuz wrote: > Hi, > > On Wed, 10 Jan 2024 at 23:48, Peter Eisentraut <peter@eisentraut.org> wrote: >> >> On 05.09.23 19:26, Nazir Bilal Yavuz wrote: >>> Thanks for the feedback! I updated the patch, 'needs-private-lo' >>> option enables kerberos, ldap, load_balance and ssl extra tests now. >> >> As was discussed, I don't think "needs private lo" is the only condition >> for these tests. At least kerberos and ldap also need extra software >> installed, and load_balance might need editing the system's hosts file. >> So someone would still need to familiarize themselves with these tests >> individually before setting a global option like this. >> >> Also, if we were to create test groupings like this, I think the >> implementation should be different. The way you have it, there is a >> sort of central registry of all affected tests in >> src/test/perl/PostgreSQL/Test/Utils.pm and a mapping of groups to tests. >> I would prefer a more decentralized approach where each test decides >> on its own whether to run, with pseudo-code conditionals like >> >> if (!(PG_TEST_EXTRA contains "ldap" or PG_TEST_EXTRA contains >> "needs-private-lo")) >> skip_all >> >> Anyway, at the moment, I don't see a sensible way to group these things >> beyond what we have now (effectively, "ldap" is already a group, because >> it affects more than one test suite). Right now, we have six possible >> values, which is probably just about doable to keep track of manually. >> If we get a lot more, then we need to look into this again, but maybe >> then we'll also have more patterns to group things around. > > I see your point. It looks like the best option is to reevaluate this > if there are more PG_TEST_EXTRA options. Ok, I'm closing this commitfest entry.