Re: Removing support for FE/BE protocol version 2

Hi Nadav,

Thank you for your detailed review!

> I applied both patches on master. The build is clean
> (no new warnings) and the full regression suite passes on PG16. I
> checked the bytes on the wire: 'E' + "FATAL: unsupported frontend
> protocol 2.0\n" + '\0'. Looks good.

I'm glad to hear that.

> 0001:
>
> - src/protocol/pool_proto_modules.c, ProcessBackendResponse(): when
> the "if (MAJOR(backend) == PROTO_MAJOR_V3) { switch ... }" was
> flattened, the closing brace of the switch was lost. Now
>
> default:
> status = SimpleForwardToFrontend(kind, frontend,
> backend);
> break;
>
> pool_get_session_context(false)->flush_pending = false;
> }
>
> so the flush_pending reset is unreachable. In master it ran after
> every backend message, and pool_process_query.c reads flush_pending.
> The regression tests pass anyway, so nothing seems to cover it.

Good catch! Will fix.

> - pool_proto_modules.h still declares the functions that were in
> pool_proto2.c: AsciiRow, BinaryRow, CompletedResponse,
> CursorResponse, EmptyQueryResponse, FunctionResultResponse,
> NotificationResponse, RowDescription, ErrorResponse, NoticeResponse.
> The file header comment still mentions pool_proto2.c too.
>
> - More v2-only code that is now dead:
> - FunctionCall() in pool_proto_modules.c (FunctionCall3() is used
> instead)
> - the tail of CopyDataRows() after the copyin/copyout branches (the
> "string == NULL" check, the pool_write() calls, and the "\." check)
> - pool_read_string() in pool_stream.c (no callers left)
> - the num_fields argument of ProcessBackendResponse() (unused now;
> the comment in pool_process_query.c still says "V2 protocol")
>
> - StartupPacket_v2 is still in pool.h and typedefs.list.
>
> - The do_query() stub in
> src/test/regression/tests/010.rewrite_timestamp/timestamp/main.c
> still has the old 4-argument signature.
>
> - Some MAJOR(backend) checks are now always true: child.c
> ("MAJOR(backend) == 3", twice), pool_query_context.c (twice) and
> pool_memqcache.c. You want to keep the protoMajor plumbing for a
> possible v4, so maybe these should stay too. Just mentioning them.

I changed my mind. Probably keeping the protoMajor for v4 is not
realistic, therefore it seems a waster of effort. I guess introducing
v4 will not happen within 5 years. So let's just remove protoMajor
specific branches and other things like argument to function.

> - elog.c: trailing whitespace in the new comment ("protocol ").
>
> 0002:
>
> - write_it() is still not the original: it is now public and calls
> exit(1) on failure. If connect_v2() needs the exit, maybe add a
> separate helper and keep write_it() as it was.
>
> - connect_v2(): if read() of the message kind fails, we print an error
> but continue and use the uninitialized kind. I think it should exit.
> Also a typo: "canot".

You are right regarding all points. Will fix. Also I change "canot" to
"could not" as the error already happend when the error message
written.

> - read_string_internal(): after pg_realloc(), p still points into the
> old buffer, so strings longer than 512 bytes break. This bug was
> already in read_string(), but now connect_v2() uses it too. Something
> like "p = buf + (PROTO_ALLOC_SIZE * (alloc_factor - 1));" after the
> realloc should fix it.

Good catch. Will fix. Maybe I should backpatch the fix.

> - Minor: the code now uses %m in fprintf() in several places (main.c,
> read.c, send.c). That is glibc-only; on macOS it prints a literal
> "m". The original code used strerror(errno).

PostgreSQL already uses %m and I thought it's safe. Actually
PostgreSQL replaces %m to %s in elog.c. So only ereport/elog API can
use %m without breaking portability. Will fix.

Attached are the v4 patches.

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