Re: psql - add SHOW_ALL_RESULTS option - Mailing list pgsql-hackers

Hello Kyotaro-san,

> Thanks. I looked this more closely.

Indeed! Thanks for this detailed review.

> + * Marshal the COPY data.  Either subroutine will get the
> + * connection out of its COPY state, then call PQresultStatus()
> + * once and report any error.
>
> This comment doesn't explain what the result value means.

Ok, added.

> + * When our command string contained a COPY FROM STDIN or COPY TO STDOUT,
> + * the PGresult associated with these commands must be processed.  In that
> + * event, we'll marshal data for the COPY.
>
> I think this is not needed. This phrase was needed to explain why
> we need to loop over subsequent results after PQexec in the
> current code, but in this patch PQsendQuery is used instead,
> which doesn't suffer somewhat confusing behavior. All results are
> handled without needing an unusual processing.

Hmmm. More or less. "COPY" commands have two results, one for taking over 
the input or output streams more or less directly at the protocol level, 
and one for the final summary, which is quite special compared to other 
commands, all that managed in "copy.c". So ISTM that the comment is 
somehow still appropriate.

The difference with the previous behavior is that other results could be 
immediately discarded but these ones, while now they are all processed.

I've kept this comment and added another one to try to make that clear.

> + * Update result if further processing is necessary.  (Returning NULL
> + * prevents the command status from being printed, which we want in that
> + * case so that the status line doesn't get taken as part of the COPY data.)
>
> It seems that the purpose of the returned PGresult is only
> printing status of this COPY. If it is true, I'd like to see
> something like the following example.
>
> | Returns result in the case where queryFout is safe to output
> | result status. That is, in the case of COPY IN, or in the case
> | where COPY OUT is written to other than pset.queryFout.

I have tried to improved the comment based on your suggestion.

> +    if (!AcceptResult(result, false))
> +    {
> +      /* some error occured, record that */
> +      ShowNoticeMessage(¬es);
>
> The comment in the original code was:
>
> -      /*
> -       * Failure at this point is always a server-side failure or a
> -       * failure to submit the command string.  Either way, we're
> -       * finished with this command string.
> -       */
>
> The first half of the comment seems to be true for this
> patch. Don't we preserve that comment?

Ok. Some form put back.

> +    success = handleCopyOut(pset.db,
> +                copystream,
> +                ©_result)
> +      && success
> +      && (copystream != NULL);
>
> success is always true at thie point so "&& success" is no longer
> useful.

Ok.

> (It is same for the COPY IN case).

Ok.

> +    /* must handle COPY before changing the current result */
> +    result_status = PQresultStatus(result);
> +    if (result_status == PGRES_COPY_IN ||
> +      result_status == PGRES_COPY_OUT)
>
> I didn't get "before changing the curren result" in the comment. Isn't 
> "handle COPY stream if any" enough?

Alas, I think not.

The issue is that I need to know whether this is the last result (eg \gset 
applies only on the last result), so I'll call PQgetResult() to get that.

However, on COPY, this is the second "final" result which says how much 
was copied. If I have not send/received the data, the count will not be 
right.

> +    if (result_status == PGRES_COPY_IN ||
> +      result_status == PGRES_COPY_OUT)
> +    {
> +      ShowNoticeMessage(¬es);
> +      HandleCopyResult(&result);
> +    }
>
> It seems that it is wrong that this ignores the return value of
> HandleCopyResult().

Yep. Fixed.

> +    /* timing measure before printing the last result */
> +    if (last && pset.timing)
>
> I'm not sure whether we reached any consensus with ths
> behavior. This means the timing includes result-printing time of
> other than the last one. If we don't want include printing time
> at all, we can exclude it with a small amount of additional
> complexity.

I think that this point is desperate, because the way timing is 
implemented client-side.

Although we could try to stop timing before each result processing, it 
would not prevent the server to go on with other queries and send back 
results, psql to receive further results (next_result), so the final 
figures would be stupid anyway, just another form of stupid.

Basically the approach cannot work with combined queries: It only worked 
before because the intermediate results were coldly discarded.

Maybe the server to report its execution times for each query somehow, but 
then the communication time would not be included.

I have added a comment about why timing does not make much sense with 
combined queries.

Attached a v5.

-- 
Fabien.