Re: Removing support for FE/BE protocol version 2

> Thanks for working on this. +1 for removing v2 in 4.8.
>
> Some comments on v1:
>
> 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.

> - The "break;" after ereport(FATAL) is unreachable (minor).

I would prefer to leave "break" here. This is the same coding style as PostgreSQL.
For example, see around line 310 of src/backend/parser/parse_expr.c.

case T_SetToDefault:
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("DEFAULT is not allowed in this context"),
parser_errposition(pstate,
((SetToDefault *) expr)->location)));
break;

> 0002:
>
> - There is a stray blank line after "/* Send each field */" in
> cache_reporting().

Good catch. Will fix.

>> I plan to work on rest of source files that handle the v2 protocol.
>>
>> src/auth/pool_auth.c
>> src/auth/pool_hba.c
>> src/context/pool_query_context.c
>> src/protocol/pool_process_query.c
>> src/protocol/pool_proto_modules.c
>> src/query_cache/pool_memqcache.c
>> src/streaming_replication/pool_worker_child.c
>> src/test/regression/tests/010.rewrite_timestamp/timestamp/main.c
>> src/utils/error/elog.c
>>
>> I hope I am not missing anything...
>
> About elog.c: I think we should keep the v2 branch in
> send_message_to_frontend(), or at least a minimal version of it.
> With 0001, child_frontend->protoVersion is already 2 when the FATAL
> is raised, so the error goes out in the old "E<text>\n" format. That
> is what a v2 client can understand. PostgreSQL 14+ kept the old-style
> error output for this same reason. If we remove it, the client gets a
> v3 ErrorResponse it can't parse.

Good point.

> Some more places not on the list:
>
> - src/protocol/pool_proto2.c: the whole file is v2 only (AsciiRow,
> BinaryRow, CursorResponse, CompletedResponse, ErrorResponse,
> NoticeResponse, NotificationResponse, FunctionResultResponse,
> RowDescription, EmptyQueryResponse). It can be removed together with
> its entry in src/Makefile.am and the prototypes in
> src/include/protocol/pool_proto_modules.h. The callers are in
> pool_proto_modules.c, which is already on your list.

Yeah, I missed pool_proto2.c.

> - src/include/pool.h: StartupPacket_v2 (and PROTO_MAJOR_V2 once
> nothing else uses it).

We cannot drop PROTO_MAJOR_V2. See 0001 patch.

> - src/tools/pgindent/typedefs.list: the StartupPacket_v2 entry.

Right.

> - src/protocol/child.c: some v2 code is left besides
> read_startup_packet(), e.g. around line 1932:
> "(sp->major == PROTO_MAJOR_V3) ? "08S01" : NULL".
>
> After all this, MAJOR() always returns 3, so the protoMajor argument of
> do_query(), do_command(), pool_send_error_message(),
> pool_send_fatal_message(), pool_send_severity_message(), pool_get_cp()
> and pool_discard_cp() becomes unused, as does
> session_context->major/pool_get_major_version(). Same for the backend
> argument of the reporting functions after 0002. Removing those touches
> many call sites, so a separate patch at the end of the series is
> probably better.

I thought about it too. But what if protocol version 4 is introduced
and pgpool needs to handle both v3 and v4? If so, we need to keep
the protoMajor argument.

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

> - An incompatibility item in the 4.8 release notes.

Of course.

> Thanks,

Thank yo for the review and useful comments!

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