Re: Extensibility of the PostgreSQL wire protocol

Thank you Kuntal,

On Fri, Feb 19, 2021 at 4:36 AM Kuntal Ghosh <kuntalghosh.2007@gmail.com>
wrote:

> On Thu, Feb 18, 2021 at 9:32 PM Jan Wieck <jan@wi3ck.info> wrote:
>
>
> Few comments in the extension code (although experimental):
>
> 1. In telnet_srv.c,
> + static int        pe_port;
> ..
> +       DefineCustomIntVariable("telnet_srv.port",
> +                                                       "Telnet server
> port.",
> +                                                       NULL,
> +                                                       &pe_port,
> +                                                       pe_port,
> +                                                       1024,
> +                                                       65536,
> +                                                       PGC_POSTMASTER,
> +                                                       0,
> +                                                       NULL,
> +                                                       NULL,
> +                                                       NULL);
>
> The variable pe_port should be initialized to a value which is > 1024
> and < 65536. Otherwise, the following assert will fail,
> TRAP: FailedAssertion("newval >= conf->min", File: "guc.c", Line:
> 5541, PID: 12100)
>
>
Right, forgot to turn on Asserts.


> 2. The function pq_putbytes shouldn't be used by anyone other than
> old-style COPY out.
> +       pq_putbytes(msg, strlen(msg));
> Otherwise, the following assert will fail in the same function:
>     /* Should only be called by old-style COPY OUT */
>     Assert(DoingCopyOut);
>

I would argue that the Assert needs to be changed. It is obvious that the
Assert in place is meant to guard against direct usage of pg_putbytes() in
an attempt to force all code to use pq_putmessage() instead. This is good
when speaking libpq wire protocol since all messages there are prefixed
with a one byte message type. It does not apply to other protocols.

I propose to create another global boolean IsNonLibpqFrontend which the
protocol extension will set to true when accepting the connection and the
above then will change to

    Assert(DoingCopyOut || IsNonLibpqFrontend);


Regards, Jan



>
> --
> Thanks & Regards,
> Kuntal Ghosh
> Amazon Web Services
>


-- 
Jan Wieck