Andrew Gierth wrote:
> But what you have in the regression tests _now_ is, as far as I can
> tell, only working by chance; with "rules" and "window" being in the
> same parallel group, and window.sql creating a view, you have an
> obvious race condition, unless I'm missing something.
>
You're right. rules.sql does this, among other things that might get
impacted:
SELECT viewname, definition FROM pg_views WHERE schemaname <>
'information_schema' ORDER BY viewname;
Both rules and window are part of the same parallel group:
test: select_views portals_p2 rules foreign_key cluster dependency guc
bitmapops combocid tsearch tsdicts foreign_data window xmlmap
Which means that views created in the window test could absolutely cause
the rules test to fail given a bad race condition. Either rules or
window needs to be moved to another section of the test schedule. (I
guess you could cut down the scope of "rules" to avoid this particular
problem, but hacking other people's regression tests to work around
issues caused by yours is never good practice). I also agree with
Andrew's sentiment that including a view on top of the new window
implementations is good practice, just for general robustness.
Also, while not a strict requirement, I personally hate seeing things
with simple names used in the regression tests, like how "v" is used in
this patch. The better written regression tests use a preface for all
their names to decrease the chance of a conflict here; for example, the
rules test names all of its views with names like rtest_v1 so they're
very clearly not going to conflict with views created by other tests.
No cleverness there can eliminate the conflict with rules though, when
it's looking at every view in the system.
It looks like a lot of progress has been made on this patch through its
review. But there's enough open issues still that I think it could use
a bit more time to mature before we try to get it committed--the fact
that it's been getting bounced around for weeks now and the regression
tests aren't even completely settled down yet is telling. The feature
seems complete, useful, and functionally solid, but still in need of
some general cleanup and re-testing afterwards. I'm going to mark this
one "Returned with Feedback" for now. Please continue to work on
knocking all these issues out, this should be a lot easier to get
committed in our next CF.
--
Greg Smith 2ndQuadrant Baltimore, MD
PostgreSQL Training, Services and Support
greg@2ndQuadrant.com www.2ndQuadrant.com
