Re: Removing support for FE/BE protocol version 2

Hi Nadav,

Thank you for the review!

> A few things i noticed reviewing the changes.
>
>> Since we cannot send v3 error response message to v2 frontend, I end
>> up with the code below:
>>
>> + case PROTO_MAJOR_V2: /* V2 protocol is not supported */
>> + ereport(FATAL,
>> + (errmsg("FATAL: unsupported frontend
> protocol %u.%u",
>> + sp->major,
> sp->minor)));
>
> I think there is a problem in the v2 branch of
> send_message_to_frontend() in elog.c:
>
> pool_send_to_frontend((edata->elevel < ERROR) ? "N" : "E", 1, false);
> pool_send_to_frontend(message, strlen(message), false);
> pool_send_to_frontend("\n", 1, true);
>
> A v2 ErrorResponse is 'E' followed by a null-terminated string, but the
> terminating '\0' is never sent. A real v2 client (libpq) reads until
> the '\0', so it would probably get EOF and report "server closed the
> connection unexpectedly" instead of our message. PostgreSQL's old-style
> path includes the terminator (pq_putmessage_v2(..., buf.len + 1)).
>
> PostgreSQL also adds the severity prefix there ("%s: ",
> error_severity(edata->elevel)) rather than in the message. If we do
> the same in elog.c, the errmsg() can be just "unsupported frontend
> protocol %u.%u". With "FATAL: " in errmsg(), I think pgpool's own log
> would show "FATAL: FATAL: unsupported frontend protocol 2.0".

Agreed. I fixed send_message_to_frontend() in elog.c. I mostly copy
the v2 branch in PostgreSQL's send_message_to_frontend()).

> Thanks! Some comments:
>
> - connect_v2() reads up to 512 bytes until EOF and prints them, so the
> test passes even without the '\0' above. Could it read up to the
> '\0' like libpq does, and fail if EOF comes first? Then the test
> would cover this. Also, if the reply is 512 bytes or more, p is not
> null-terminated before the fprintf("%s").

Yes, we could. I added read_string_internal(), which accepts int file
descriptor, rather than PQConn, and let connect_v2() call it for this
purpose.

> - connect_v2() always uses "/tmp" as the socket directory and ignores
> -h when it starts with '/'. regress.sh lets you change
> PGSOCKET_DIR (-s), so the test fails when /tmp is not used. Maybe
> use -h as the socket directory like libpq does, and pass it from
> test.sh.

I modified connect_v2() to recognize -h when it starts with '/'. I
also modified test.sh to give "-h localhost" to pgproto so that the
test script does not depend on the socket directory.

> - The new -t/--test-v2 option should be documented in
> doc/src/sgml/ref/pgproto.sgml.
>
> - The PostgreSQL >= 14 and pgpool >= 4.8 checks in test.sh don't seem
> necessary: pgpool rejects the packet before any backend sees it, and
> the test always runs with pgpool from the same tree.

Ok. The check removed.

> - write_it() now exits on failure instead of printing a warning.
> That's probably fine, but it changes behavior for the existing
> pgproto tests.

I undo the modification to write_it().

> - Minor: read.c's copyright still says 2018-2023.

Fixed.

Attached are the v3 patch which addresses above.

Regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp