Re: psql - add SHOW_ALL_RESULTS option

Hello.

On 2019/07/29 6:36, Fabien COELHO wrote:> 
>> Thanks for the feedback. Attached v3 with further documentation updates.
> 
> Attached v4 also fixes pg_stat_statements non regression tests, per pg 
> patch tester travis run.

Thanks. I looked this more closely.


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

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

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


+    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?


+    success = handleCopyOut(pset.db,
+                copystream,
+                ©_result)
+      && success
+      && (copystream != NULL);

success is always true at thie point so "&& success" is no longer
useful. (It is same for the COPY IN case).


+    /* 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?

+    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().


+    /* 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.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center