On 12/6/23 18:28, David G. Johnston wrote:
On Wed, Dec 6, 2023 at 4:09 PM Joe Conway <m...@joeconway.com <mailto:m...@joeconway.com>> wrote:

    On 12/6/23 14:47, Joe Conway wrote:
     > On 12/6/23 13:59, Daniel Verite wrote:
     >>      Andrew Dunstan wrote:
     >>
     >>> IMNSHO, we should produce either a single JSON
     >>> document (the ARRAY case) or a series of JSON documents, one
    per row
     >>> (the LINES case).
     >>
     >> "COPY Operations" in the doc says:
     >>
     >> " The backend sends a CopyOutResponse message to the frontend,
    followed
     >>     by zero or more CopyData messages (always one per row),
    followed by
     >>     CopyDone".
     >>
     >> In the ARRAY case, the first messages with the copyjsontest
     >> regression test look like this (tshark output):
     >>
     >> PostgreSQL
     >>      Type: CopyOut response
     >>      Length: 13
     >>      Format: Text (0)
     >>      Columns: 3
     >>      Format: Text (0)

     > Anything receiving this and looking for a json array should know
    how to
     > assemble the data correctly despite the extra CopyData messages.

    Hmm, maybe the real problem here is that Columns do not equal "3" for
    the json mode case -- that should really say "1" I think, because the
    row is not represented as 3 columns but rather 1 json object.

    Does that sound correct?

    Assuming yes, there is still maybe an issue that there are two more
    "rows" that actual output rows (the "[" and the "]"), but maybe those
    are less likely to cause some hazard?


What is the limitation, if any, of introducing new type codes for these.  n = 2..N for the different variants?  Or even -1 for "raw text"?  And document that columns and structural rows need to be determined out-of-band.  Continuing to use 1 (text) for this non-csv data seems like a hack even if we can technically make it function.  The semantics, especially for the array case, are completely discarded or wrong.

I am not following you here. SendCopyBegin looks like this currently:

8<--------------------------------
SendCopyBegin(CopyToState cstate)
{
        StringInfoData buf;
        int                     natts = list_length(cstate->attnumlist);
        int16           format = (cstate->opts.binary ? 1 : 0);
        int                     i;

        pq_beginmessage(&buf, PqMsg_CopyOutResponse);
        pq_sendbyte(&buf, format);  /* overall format */
        pq_sendint16(&buf, natts);
        for (i = 0; i < natts; i++)
                pq_sendint16(&buf, format); /* per-column formats */
        pq_endmessage(&buf);
        cstate->copy_dest = COPY_FRONTEND;
}
8<--------------------------------

The "1" is saying are we binary mode or not. JSON mode will never be sending in binary in the current implementation at least. And it always aggregates all the columns as one json object. So the correct answer is (I think):
8<--------------------------------
*************** SendCopyBegin(CopyToState cstate)
*** 146,154 ****

        pq_beginmessage(&buf, PqMsg_CopyOutResponse);
        pq_sendbyte(&buf, format);  /* overall format */
!       pq_sendint16(&buf, natts);
!       for (i = 0; i < natts; i++)
!               pq_sendint16(&buf, format); /* per-column formats */
        pq_endmessage(&buf);
        cstate->copy_dest = COPY_FRONTEND;
  }
--- 150,169 ----

        pq_beginmessage(&buf, PqMsg_CopyOutResponse);
        pq_sendbyte(&buf, format);  /* overall format */
!       if (!cstate->opts.json_mode)
!       {
!               pq_sendint16(&buf, natts);
!               for (i = 0; i < natts; i++)
!                       pq_sendint16(&buf, format); /* per-column formats */
!       }
!       else
!       {
!               /*
!                * JSON mode is always one non-binary column
!                */
!               pq_sendint16(&buf, 1);
!               pq_sendint16(&buf, 0);
!       }
        pq_endmessage(&buf);
        cstate->copy_dest = COPY_FRONTEND;
  }
8<--------------------------------

That still leaves the need to fix the documentation:

" The backend sends a CopyOutResponse message to the frontend, followed
   by zero or more CopyData messages (always one per row), followed by
   CopyDone"

probably "always one per row" would be changed to note that json array format outputs two extra rows for the start/end bracket.

In fact, as written the patch does this:
8<--------------------------------
COPY (SELECT x.i, 'val' || x.i as v FROM
      generate_series(1, 3) x(i) WHERE false)
TO STDOUT WITH (FORMAT JSON, FORCE_ARRAY);
[
]
8<--------------------------------

Not sure if that is a problem or not.

--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com



Reply via email to