Re: [PATCH v4] Add \warn to psql

From: Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: David Fetter <david(at)fetter(dot)org>, Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>, 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-03 07:05:28
Message-ID: alpine.DEB.2.21.1907030842030.17242@lancre
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Hello Tom,

> 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.


The point is that there would be at least *one* TAP tests so that many
other features of psql, although not all, can be tested. I have been
reviewing quite a few patches without tests because of this lack of
infrastructure, and no one patch is ever going to justify a TAP test on
its own. It has to start somewhere. Currently psql coverage is abysmal,
around 40% of lines & functions are called by the whole non regression
tests, despite the hundreds of psql-relying tests. Pg is around 80%
coverage overall.

Basically, I really thing that one psql dedicated TAP test should be
added, not for \warn per se, but for other features.

> 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.

The tab complete and prompt are special interactive cases and probably
require special infrastructure to make a test believe it is running
against a tty while it is not. The point of this proposal is not to
address these special needs, but to lay a basic infra.

> I don't like what you did to command_checks_all,

Yeah, probably my fault, not David.

> 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.

I wanted to avoid breaking the function signature of it is used by some
external packages. Not caring is an option.


In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2019-07-03 07:11:31 Re: TopoSort() fix
Previous Message David Rowley 2019-07-03 06:56:25 Re: POC: converting Lists into arrays