Re: Removing support for FE/BE protocol version 2

Hi,

Thanks for v4. I applied it on master. The build is clean (no new
warnings, also with -Wextra and -Wunused-parameter), the full
regression suite passes on PG16 (80/80), and 045 passes. On the wire
the reply is 'E' + "FATAL: unsupported frontend protocol 2.0\n" +
'\0', and the log has a single FATAL line. All the v3 points are fixed.

Since we are now removing the protoMajor-specific code entirely, here
is what is still left. None of it is a bug:

- pool_hba.c (around lines 1342 and 1380): two
"frontend->protoVersion == PROTO_MAJOR_V3" checks that are now
always true. This file was on your original list.

- child.c (around line 1031): "new_sp->major == PROTO_MAJOR_V3 &&
sp->application_name".

- The MAJOR() macro in pool.h has no callers now. So
pool_get_major_version(), pool_set_major_version() (called only from
child.c) and session_context->major are dead too, as is the
pool_get_major_version() stub in the 010 test.

- pool_get_cp() and pool_discard_cp() still take protoMajor and compare
it with sp->major. It is harmless, but the argument could be dropped
along with the rest.

- pool.h: sbuf/sbufsz in POOL_CONNECTION are now only initialized and
freed in pool_stream.c. Their only user was pool_read_string(), which
is gone (the comment still says "buffer for pool_read_string").

- pgproto/read.c:411 still uses %m.

- pgproto/send.c write_it(): "int errsave = errno;" needs pgindent
(alignment and a blank line after the declaration).

- pool_proto_modules.h: the header comment ends with a dangling comma
("pool_proto_modules.c,"), and there is a double blank line after
log_backend_messages().

Other than these, the patches look good to me. Doing the items above in
a follow-up commit is probably fine too.

Best,

On Mon, Oct 12, 2026 at 8:05 AM Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:

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

--
Nadav Shatz
Tailor Brands | CTO