Re: [PATCH v4] Add \warn to psql

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: David Fetter <david(at)fetter(dot)org>
Cc: Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>, Corey Huinker <corey(dot)huinker(at)gmail(dot)com>, PostgreSQL Development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [PATCH v4] Add \warn to psql
Date: 2019-07-02 20:10:50
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

David Fetter <david(at)fetter(dot)org> writes:
> [ v7-0001-Add-warn-to-psql.patch ]

I took a look at this. I have no quibble with the proposed feature,
and the implementation is certainly simple enough. But I'm unconvinced
about the proposed test scaffolding. Spinning up a new PG instance is a
*hell* of a lot of overhead to pay for testing something that could be
tested as per attached. Admittedly, the attached doesn't positively
prove which pipe each output string went down, but that does not strike
me as a concern large enough to justify adding a TAP test for.

I'd be happier about adding TAP infrastructure if it looked like it'd
be usable to test some of the psql areas that are unreachable by the
existing test methodology, particularly tab-complete.c and prompt.c.
But I don't see anything here that looks like it'll work for that.

I don't like what you did to command_checks_all, either --- it could
hardly say "bolted on after the fact" more clearly if you'd written
that in <blink><red> text. If we need an input-stream argument,
let's just add it in a rational place and adjust the callers.
There aren't that many of 'em, nor has the subroutine been around
all that long.

regards, tom lane

Attachment Content-Type Size
easier-warn-test.patch text/x-diff 1.5 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2019-07-02 20:16:27 Re: [PATCH v5] Show detailed table persistence in \dt+
Previous Message Alvaro Herrera 2019-07-02 19:56:18 Re: [PATCH v5] Show detailed table persistence in \dt+