Обсуждение: pgsql: Add ANALYZE into regression tests

Add ANALYZE into regression tests

Looks like we can end up with different plans happening on the
buildfarm, which breaks the regression tests when we include
EXPLAIN output (which is done in the regression tests for
updatable security views, to ensure that the user-defined
function isn't pushed down to a level where it could view the
rows before the security quals are applied).

This adds in ANALYZE to hopefully make the plans consistent.
The ANALYZE ends up changing the original plan too, so the
update looks bigger than it really is.  The new plan looks
perfectly valid, of course.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/b3e6593716efef901fcc847f33256c6b49958898

Modified Files
--------------
src/test/regress/expected/updatable_views.out |  232 +++++++++++--------------
src/test/regress/sql/updatable_views.sql      |    4 +
2 files changed, 102 insertions(+), 134 deletions(-)

On 04/13/2014 12:42 AM, Stephen Frost wrote:
> Add ANALYZE into regression tests
>
> Looks like we can end up with different plans happening on the
> buildfarm, which breaks the regression tests when we include
> EXPLAIN output (which is done in the regression tests for
> updatable security views, to ensure that the user-defined
> function isn't pushed down to a level where it could view the
> rows before the security quals are applied).
>
> This adds in ANALYZE to hopefully make the plans consistent.
> The ANALYZE ends up changing the original plan too, so the
> update looks bigger than it really is.  The new plan looks
> perfectly valid, of course.
>
>

Are you really sure we can do this consistently? The regression tests
have to run against all sorts of settings, including those we have no
control over via installcheck.

cheers

andrew

* Andrew Dunstan (andrew@dunslane.net) wrote:
> Are you really sure we can do this consistently? The regression
> tests have to run against all sorts of settings, including those we
> have no control over via installcheck.

Sure?  No.  However, there are quite a few existing regression tests
that do the same (with costs off, of course), and things on the
buildfarm look more-or-less clean again, so I'm at least hopeful.

I can simplify the tests some if they end up causing issues.

    Thanks,

        Stephen

On 04/13/2014 08:48 AM, Stephen Frost wrote:
> * Andrew Dunstan (andrew@dunslane.net) wrote:
>> Are you really sure we can do this consistently? The regression
>> tests have to run against all sorts of settings, including those we
>> have no control over via installcheck.
> Sure?  No.  However, there are quite a few existing regression tests
> that do the same (with costs off, of course), and things on the
> buildfarm look more-or-less clean again, so I'm at least hopeful.
>
> I can simplify the tests some if they end up causing issues.
>
>

OK, fair enough.

cheers

andrew

Yeah I think this is OK.

But the original goal seems like it would be easier and better done with an immutable function which lies and calls elog to leak information. That's the actual attack this is supposed to protect against anyways.

That would make the tests more robust against other changes causing failures. Even things like changing explain output formatting for example.

--
greg

Greg,

* Greg Stark (stark@mit.edu) wrote:
> But the original goal seems like it would be easier and better done with an
> immutable function which lies and calls elog to leak information. That's
> the actual attack this is supposed to protect against anyways.

Uh, yes, that's what the explain is about- making sure that the 'snoop'
function in the regression test doesn't get pushed down.  It *also*
runs the function which raises a notice for each item the function can
see, and verifies that only those values are returned..

> That would make the tests more robust against other changes causing
> failures. Even things like changing explain output formatting for example.

Sure, but there's a whole slew of tests that would have to change if we
changed the explain output, not just this one.  I don't think we really
want to make a policy against doing EXPLAIN in regression tests, but if
so, we'd need to go change quite a few tests which have been working
pretty well to date..

    Thanks,

        Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Greg Stark (stark@mit.edu) wrote:
>> But the original goal seems like it would be easier and better done with an
>> immutable function which lies and calls elog to leak information. That's
>> the actual attack this is supposed to protect against anyways.

> Sure, but there's a whole slew of tests that would have to change if we
> changed the explain output, not just this one.

Sure, but I think Greg's point is that this could be tested by a
black-box functional test ("does it print something it shouldn't")
rather than a white-box test that necessarily depends on a whole lot
of *other* planner choices that don't have much to do with the point
in question.  You already got bit by variances in the choice of join
type, which is not what the test is about.

I think the test is okay as-is as long as we don't see more failures
from it; but if we do see any more I'd suggest rewriting as per Greg's
suggestion rather than trying to constrain the plan choice even more
narrowly.

            regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Sure, but I think Greg's point is that this could be tested by a
> black-box functional test ("does it print something it shouldn't")
> rather than a white-box test that necessarily depends on a whole lot
> of *other* planner choices that don't have much to do with the point
> in question.  You already got bit by variances in the choice of join
> type, which is not what the test is about.

Yes, but as I said, I'm *also* doing the black-box functional test..
Perhaps that means this extra EXPLAIN test is overkill, but, after
trying to run down whatever build animal 'hamerkop' has done to break
the tablespace regression test, I'm definitely keen to err on the side
of 'more information'.

> I think the test is okay as-is as long as we don't see more failures
> from it; but if we do see any more I'd suggest rewriting as per Greg's
> suggestion rather than trying to constrain the plan choice even more
> narrowly.

The 'rewriting', in this case, would simply be removing the 'explain'
part of the test and keeping the rest.  If you or Greg see an issue with
the test that actually does the 'raise notice' for every value seen by
the 'snoop' function or it doesn't match your 'black-box' expectation,
please let me know and I can try to refine it..

    Thanks,

        Stephen