Обсуждение: Verifying embedded oids in *recv is a bad idea

Hi,

for performance reasons it's a good idea to use the binary protocol. But
doing so between two postgres installations is made unnecessarily hard
by the choice of embedding and verifying oids in composite and array
types.  When using extensions, even commonly used ones like hstore, the
datatype oids will often not be the same between systems, even when
using exactly the same version on the same OS.

Specifically I'm talking about

Datum
array_recv(PG_FUNCTION_ARGS)
{
...element_type = pq_getmsgint(buf, sizeof(Oid));if (element_type != spec_element_type){    /* XXX Can we allow taking
theinput element type in any cases? */    ereport(ERROR,            (errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("wrongelement type")));}
 
...
}

and

Datum
record_recv(PG_FUNCTION_ARGS)
{
.../* Process each column */for (i = 0; i < ncolumns; i++)
...    /* Verify column datatype */    coltypoid = pq_getmsgint(buf, sizeof(Oid));    if (coltypoid != column_type)
  ereport(ERROR,                (errcode(ERRCODE_DATATYPE_MISMATCH),                 errmsg("wrong data type: %u,
expected%u",                        coltypoid, column_type)));
 
...
}


given that we're giving up quite some speed and adding complexity to
make send/recv functions portable, this seems like a shame.

I'm failing to see what these checks are buying us? I mean the text
representation of a composite doesn't include type information about
contained columns either, I don't see why the binary version has to?

I'm basically thinking that we should remove the checks on the receiving
side, but leave the sending of oids in place for backward compat.

Greetings,

Andres Freund

Hello,

At Mon, 25 Apr 2016 17:17:13 -0700, Andres Freund <andres@anarazel.de> wrote in
<20160426001713.hbqdiwvf4mkzkg55@alap3.anarazel.de>
> Hi,
> 
> for performance reasons it's a good idea to use the binary protocol. But
> doing so between two postgres installations is made unnecessarily hard
> by the choice of embedding and verifying oids in composite and array
> types.  When using extensions, even commonly used ones like hstore, the
> datatype oids will often not be the same between systems, even when
> using exactly the same version on the same OS.
> 
> Specifically I'm talking about
> 
> Datum
> array_recv(PG_FUNCTION_ARGS)
> {
> ...
>     element_type = pq_getmsgint(buf, sizeof(Oid));
>     if (element_type != spec_element_type)
>     {
>         /* XXX Can we allow taking the input element type in any cases? */
>         ereport(ERROR,
>                 (errcode(ERRCODE_DATATYPE_MISMATCH),
>                  errmsg("wrong element type")));
>     }
> ...
> }
> 
> and
> 
> Datum
> record_recv(PG_FUNCTION_ARGS)
> {
> ...
>     /* Process each column */
>     for (i = 0; i < ncolumns; i++)
> ...
>         /* Verify column datatype */
>         coltypoid = pq_getmsgint(buf, sizeof(Oid));
>         if (coltypoid != column_type)
>             ereport(ERROR,
>                     (errcode(ERRCODE_DATATYPE_MISMATCH),
>                      errmsg("wrong data type: %u, expected %u",
>                             coltypoid, column_type)));
> ...
> }
> 
> 
> given that we're giving up quite some speed and adding complexity to
> make send/recv functions portable, this seems like a shame.
> 
> I'm failing to see what these checks are buying us? I mean the text
> representation of a composite doesn't include type information about
> contained columns either, I don't see why the binary version has to?
> 
> I'm basically thinking that we should remove the checks on the receiving
> side, but leave the sending of oids in place for backward compat.

FWIW, I think that typsend/typreceive are intended for the use by
COPY FROM/TO and the doc says that the following.

http://www.postgresql.org/docs/devel/static/sql-copy.html

> Binary Format
> 
> The binary format option causes all data to be stored/read as
> binary format rather than as text. It is somewhat faster than the
> text and CSV formats, but a binary-format file is less portable
> across machine architectures and PostgreSQL versions. Also, the
> binary format is very data type specific; for example it will not
> work to output binary data from a smallint column and read it
> into an integer column, even though that would work fine in text
> format.

I haven't considered much about the usefulness of this
restriction, but receiving into an array of a different type
easily leads to a crash.

# However, false matching of type oids also would do so.

I suppose that we should reconsider here if removing the checks.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center