| From: | Nadav Shatz <nadav(at)tailorbrands(dot)com> |
|---|---|
| To: | Tatsuo Ishii <ishii(at)postgresql(dot)org> |
| Cc: | pgpool-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: Removing support for FE/BE protocol version 2 |
| Date: | 2026-10-06 14:05:54 |
| Message-ID: | CACeKOO1zEg3BHqWPCuYPuKHw0_prnECfVWWdwft8b-J9VZV=vg@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgpool-hackers |
Hi Tatsuo,
Thanks for working on this!
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".
> I include this as v2-0003-Modify-pgproto-as-a-test-driver.patch.
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").
- 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.
- 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.
- write_it() now exits on failure instead of printing a warning.
That's probably fine, but it changes behavior for the existing
pgproto tests.
- Minor: read.c's copyright still says 2018-2023.
Let me know what you think.
Best,
On Mon, Oct 5, 2026 at 8:43 AM Tatsuo Ishii <ishii(at)postgresql(dot)org> wrote:
> >> 0001:
> >>
> >> - pool_error_code("0X000") looks like a typo.
> ERRCODE_FEATURE_NOT_SUPPORTED
> >> is "0A000".
> >
> > Thanks for pointing it out. You are right.
> >
> >> - PostgreSQL's message for this case is "unsupported frontend protocol
> >> %u.%u: server supports %u.0 to %u.%u". If the goal is the same
> >> behavior as PostgreSQL, maybe we could use the same text.
> >
> > Unfortunately at this point pgpool has not connected to backend
> > yet. Until connected to backend, pgpool does not know what range of
> > protocol versions are supported by the backend. Therefore, pgpool
> > cannot generate exactly the same message as PostgreSQL.
>
> 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)));
>
> >> Two more things:
> >>
> >> - A regression test that sends a v2 startup packet and checks that we
> >> get the FATAL error. It's small, and it covers the only v2 code path
> >> we keep.
> >
> > Yes, for the purpose, we need to create a small v2 client.
>
> I include this as v2-0003-Modify-pgproto-as-a-test-driver.patch. I
> modified pgproto to add "-t" option ("test-v2" as a long option),
> which just sends a v2 startup packet and reads response from pgpool,
> and exits. regression test "045.remove_proto_v2" is added.
>
> Other than above, I ran pgindent and fix Copyright notice in passing.
>
> 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
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Tatsuo Ishii | 2026-10-10 07:50:47 | Re: Removing support for FE/BE protocol version 2 |
| Previous Message | Koshino Taiki | 2026-10-05 07:36:30 | Preserve dml_adaptive state on ROLLBACK TO SAVEPOINT |