On Thu, Jan 26, 2012 at 08:23:34AM -0500, Robert Haas wrote:
> On Thu, Jan 26, 2012 at 6:55 AM, Noah Misch <noah@leadboat.com> wrote:
> > On Wed, Jan 25, 2012 at 11:53:10PM -0500, Tom Lane wrote:
> >> Not only is that code spectacularly unreadable, but has nobody noticed
> >> that this commit broke the buildfarm?
> >
> > Thanks for reporting the problem. ?This arose because the new test case
> > temporarily sets client_min_messages=DEBUG1. ?The default buildfarm
> > configuration sets log_statement=all in its postgresql.conf, so the client
> > gets those log_statement lines. ?I would fix this as attached, resetting the
> > optional logging to defaults during the test cases in question. ?Not
> > delightful, but that's what we have to work with.
>
> I'm just going to remove the test. This is not very future-proof and
It would deserve an update whenever we add a new optional-logging GUC
pertaining to user backends. Other tests require similarly-infrequent
refreshes in response to other changes. Of course, buildfarm members would
not be setting those new GUCs the day they're available. Calling for an
update to the test could reasonably fall to the first buildfarm member owner
who actually decides to use a new GUC in his configuration.
> an ugly pattern if it gets copied to other places. We need to work on
I would rather folks introduce ugliness to the test suite than introduce
behaviors having no test coverage.
> a more sensible way for ALTER TABLE to report what it did, but a
> solution based on what GUCs the build-farm happens to set doesn't seem
> like it's justified for the narrowness of the case we're testing here.
It's not based on what GUCs the buildfarm happens to set. I looked up all
GUCs that might create problems such as the one observed here. They were the
four I included in the patch, plus debug_pretty_print, debug_print_parse,
debug_print_plan and debug_print_rewritten. I concluded that the four debug_*
ones were materially less likely to ever get set in postgresql.conf for a
"make installcheck" run, so I left them out for brevity.
The setting changed *by default* for buildfarm clients is log_statement.
Buildfarm member owners can do as they wish, though.
> Whether or not we allow this case to work without a rewrite is in
> some sense arbitrary. There's no real reason it can't be done; rather,
> we're just exercising restraint to minimize the risk of future bugs.
> So I don't want to go to great lengths to test it.
I used the same strategy in another ALTER TABLE patch this CF:
http://archives.postgresql.org/message-id/20120126033956.GC15670@tornado.leadboat.com
If we pay its costs for those tests, we then may as well let this test case
ride their coattails.
