Обсуждение: Re: [HACKERS] New psql input mode problems

Peter Eisentraut <e99re41@DoCS.UU.SE>
>On Sun, 7 Nov 1999, Bruce Momjian wrote:
>
>> > This seems to be where it goes wrong. (mainloop.c)
>> > 
>> > /* No more input.  Time to quit, or \i done */
>> > if (line == NULL || (!pset->cur_cmd_interactive && *line == '\0'))
>
>This line was there in the old source as well.

Just checked and you're right..

>
>> > 
>> > When a blank line is encountered in the input 
>> > 
>> >     line = gets_fromFile(source);
>> >     
>> > returns an empty string ('\0') and terminates the processing.
>
>Same in the old one.

I think somehow the old version was subtly different.

The old version of psql read and processed the whole of
each regression sql file. The new version stops processing
as soon as it sees a blank line.

I'm not sure if there's something different with the '\n'
handling in the new version. If the '\n' is being stripped
out that could make all the difference between "\n", which
would not terminate and "" which would.

>
>> > 
>> > with the if clause reduced to checking for line == NULL psql
>> > does the work but fails badly due to the differences between
>> > results and expected. (comments, QUERY:, echo processing)
>
>As I said, that part was not my idea. I'll look into that though.

This is a difficult one, I never was any good at decisions.

There are a number of key differences.

Query text OLD  echoed prefixed with "QUERY:" NEW echoed as read.
Formatting OLD  slightly different to         NEW. (alignment and '-'s)


OLD:

QUERY: SELECT 1 AS one;
one
--- 1
(1 row)


NEW:


SELECT 1 AS one;one
-----  1
(1 row)


>
>> 
>> > 
>> > Is the intention to modify expected to agree with the new
>> > results output, or fix psql to output in the expected format?
>
>How about using the old psql for regression testing?

I like the new one better :-)

>
>> 
>> Good question.  We need to know if people like the current output
>> format, or the old one better?
>
>I send in several examples. If no one comments, that's silent approval.
>I am really hesitant to put in a compatibility format, but I might just do
>that until new regression tests are out and you ask me to.
>

If it was not too difficult this would be one way to go.

We really need the regression output maintainer (Tom?) to comment
here. I'd hate to have to do the 1st compare between the new output
and old expected by eye.


Keith.

On 1999-11-08, Keith Parks mentioned:

> Peter Eisentraut <e99re41@DoCS.UU.SE>
> >On Sun, 7 Nov 1999, Bruce Momjian wrote:
> >
> >> > This seems to be where it goes wrong. (mainloop.c)
> >> > 
> >> > /* No more input.  Time to quit, or \i done */
> >> > if (line == NULL || (!pset->cur_cmd_interactive && *line == '\0'))
> >
> >This line was there in the old source as well.
> 
> Just checked and you're right..
> 
> >
> >> > 
> >> > When a blank line is encountered in the input 
> >> > 
> >> >     line = gets_fromFile(source);
> >> >     
> >> > returns an empty string ('\0') and terminates the processing.
> >
> >Same in the old one.
> 
> I think somehow the old version was subtly different.
> 
> The old version of psql read and processed the whole of
> each regression sql file. The new version stops processing
> as soon as it sees a blank line.
> 
> I'm not sure if there's something different with the '\n'
> handling in the new version. If the '\n' is being stripped
> out that could make all the difference between "\n", which
> would not terminate and "" which would.

We have a winner!

The new gets_fromFile strips the trailing newline, so an empty line in a
file really comes in as an empty line. I don't see any reason why the
check was done as it was in the first place, so the correct line in
mainloop.c should be:
       /* No more input.  Time to quit, or \i done */       if (line == NULL)       {
           
 

as suggested.

> 
> >
> >> > 
> >> > with the if clause reduced to checking for line == NULL psql
> >> > does the work but fails badly due to the differences between
> >> > results and expected. (comments, QUERY:, echo processing)
> >
> >As I said, that part was not my idea. I'll look into that though.
> 
> This is a difficult one, I never was any good at decisions.
> 
> There are a number of key differences.
> 
> Query text OLD  echoed prefixed with "QUERY:" NEW echoed as read.

This behaviour is plagiarized from command shells.

> Formatting OLD  slightly different to         NEW. (alignment and '-'s)
> 
> 
> OLD:
> 
> QUERY: SELECT 1 AS one;
> one
> ---
>   1
> (1 row)
> 
> 
> NEW:
> 
> 
> SELECT 1 AS one;
>  one
> -----
>    1
> (1 row)
> 
> 
> >
> >> 
> >> > 
> >> > Is the intention to modify expected to agree with the new
> >> > results output, or fix psql to output in the expected format?
> >
> >How about using the old psql for regression testing?
> 
> I like the new one better :-)

Life sucks ;)

> 
> >
> >> 
> >> Good question.  We need to know if people like the current output
> >> format, or the old one better?
> >
> >I send in several examples. If no one comments, that's silent approval.
> >I am really hesitant to put in a compatibility format, but I might just do
> >that until new regression tests are out and you ask me to.
> >
> 
> If it was not too difficult this would be one way to go.
> 
> We really need the regression output maintainer (Tom?) to comment
> here. I'd hate to have to do the 1st compare between the new output
> and old expected by eye.

Here's an idea: Why don't the regression tests use a single-user postgres
backend? That way you have even more control over internals, you can run
it on an uninstalled database, and you don't rely on the particularities
of the output of some obscure front-end.

But I'm not going to tailor psql around the regression tests. That's the
wrong direction. Just run it once with the old one and then with the new
one and put those results in the current tree as a temporary solution.
-Peter

-- 
Peter Eisentraut                  Sernanders vaeg 10:115
peter_e@gmx.net                   75262 Uppsala
http://yi.org/peter-e/            Sweden

> We have a winner!
> 
> The new gets_fromFile strips the trailing newline, so an empty line in a
> file really comes in as an empty line. I don't see any reason why the
> check was done as it was in the first place, so the correct line in
> mainloop.c should be:
> 
>         /* No more input.  Time to quit, or \i done */
>         if (line == NULL)
>         {                                               
> 
> as suggested.

Good.  Already done.

> Here's an idea: Why don't the regression tests use a single-user postgres
> backend? That way you have even more control over internals, you can run
> it on an uninstalled database, and you don't rely on the particularities
> of the output of some obscure front-end.

It is generally unsafe because they don't share locking with other
backends, and some table are shared between databases.


> 
> But I'm not going to tailor psql around the regression tests. That's the
> wrong direction. Just run it once with the old one and then with the new
> one and put those results in the current tree as a temporary solution.

Yes, I am sure that will be done soon.

--  Bruce Momjian                        |  http://www.op.net/~candle maillist@candle.pha.pa.us            |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026