Re: [HACKERS] Logical Replication and Character encoding - Mailing list pgsql-hackers

At Mon, 6 Mar 2017 11:13:48 +0100, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote in
<a7c0cc59-9c21-238c-b6a7-d6eb519ad9ee@2ndquadrant.com>
> >>> Well the length is necessary to be able to add binary format support in
> >>> future so it's definitely not an omission.
> >>
> >> Right.  So it appears the right function to use here is
> >> pq_sendcountedtext().  However, this sends the strings without null
> >> termination, so we'd have to do extra processing on the receiving end.
> >> Or we could add an option to pq_sendcountedtext() to make it do what we
> >> want.  I'd rather stick to standard pqformat.c functions for handling
> >> the protocol.
> > 
> > It seems reasonable. I changed the prototype of
> > pg_sendcountedtext as the following,
> > 
> > | extern void pq_sendcountedtext(StringInfo buf, const char *str, int slen,
> > |                bool countincludesself, bool binary);
> > 
> > I think 'binary' seems fine for the parameter here. The patch
> > size increased a bit.
> > 
> 
> Hmm I find it bit weird that binary true means NULL terminated while
> false means not NULL terminated, I would think that the behaviour would
> be exactly opposite given that text strings are the ones where NULL
> termination matters?

You're right. I renamed it as with more straightforward
name. Addition to that, it contained a stupid bug that sends a
garbage byte when non-null-terminated string is not converted.
And the comment is edited to reflect the new behavior.

> > By the way, I noticed that postmaster launches logical
> > replication launcher even if wal_level < logical. Is it right?
> 
> Yes, that came up couple of times in various threads. The logical
> replication launcher is needed only to start subscriptions and
> subscriptions don't need wal_level to be logical, only publication needs
> that.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/access/common/printsimple.c b/src/backend/access/common/printsimple.c
index 5fe1c72..62fa70d 100644
--- a/src/backend/access/common/printsimple.c
+++ b/src/backend/access/common/printsimple.c
@@ -96,7 +96,7 @@ printsimple(TupleTableSlot *slot, DestReceiver *self)                    pq_sendcountedtext(&buf,
                                 VARDATA_ANY(t),                                       VARSIZE_ANY_EXHDR(t),
 
-                                       false);
+                                       false, false);                }                break;
@@ -106,7 +106,7 @@ printsimple(TupleTableSlot *slot, DestReceiver *self)                    char    str[12];    /*
sign,10 digits and '\0' */                    pg_ltoa(num, str);
 
-                    pq_sendcountedtext(&buf, str, strlen(str), false);
+                    pq_sendcountedtext(&buf, str, strlen(str), false, false);                }                break;
@@ -116,7 +116,7 @@ printsimple(TupleTableSlot *slot, DestReceiver *self)                    char    str[23];    /*
sign,21 digits and '\0' */                    pg_lltoa(num, str);
 
-                    pq_sendcountedtext(&buf, str, strlen(str), false);
+                    pq_sendcountedtext(&buf, str, strlen(str), false, false);                }                break;
diff --git a/src/backend/access/common/printtup.c b/src/backend/access/common/printtup.c
index a2ca2d7..1ebc50f 100644
--- a/src/backend/access/common/printtup.c
+++ b/src/backend/access/common/printtup.c
@@ -353,7 +353,8 @@ printtup(TupleTableSlot *slot, DestReceiver *self)            char       *outputstr;
outputstr= OutputFunctionCall(&thisState->finfo, attr);
 
-            pq_sendcountedtext(&buf, outputstr, strlen(outputstr), false);
+            pq_sendcountedtext(&buf, outputstr, strlen(outputstr),
+                               false, false);        }        else        {
@@ -442,7 +443,7 @@ printtup_20(TupleTableSlot *slot, DestReceiver *self)        Assert(thisState->format == 0);
outputstr= OutputFunctionCall(&thisState->finfo, attr);
 
-        pq_sendcountedtext(&buf, outputstr, strlen(outputstr), true);
+        pq_sendcountedtext(&buf, outputstr, strlen(outputstr), true, false);    }    pq_endmessage(&buf);
diff --git a/src/backend/libpq/pqformat.c b/src/backend/libpq/pqformat.c
index c8cf67c..a83c73e 100644
--- a/src/backend/libpq/pqformat.c
+++ b/src/backend/libpq/pqformat.c
@@ -123,30 +123,39 @@ pq_sendbytes(StringInfo buf, const char *data, int datalen) * The data sent to the frontend by
thisroutine is a 4-byte count field * followed by the string.  The count includes itself or not, as per the *
countincludesselfflag (pre-3.0 protocol requires it to include itself).
 
- * The passed text string need not be null-terminated, and the data sent
- * to the frontend isn't either.
+ * The passed text string need not be null-terminated but should not contain
+ * null as a part. sendterminator instructs to send null-terminator. * --------------------------------
*/voidpq_sendcountedtext(StringInfobuf, const char *str, int slen,
 
-                   bool countincludesself)
+                   bool countincludesself, bool sendterminator){
-    int            extra = countincludesself ? 4 : 0;
+    int            extra_self        = countincludesself ? 4 : 0;
+    int            extra_term        = sendterminator ? 1 : 0;    char       *p;    p = pg_server_to_client(str,
slen);   if (p != str)                /* actual conversion has been done? */    {        slen = strlen(p);
 
-        pq_sendint(buf, slen + extra, 4);
+        pq_sendint(buf, slen + extra_self + extra_term, 4);        appendBinaryStringInfo(buf, p, slen);
pfree(p);   }    else    {
 
-        pq_sendint(buf, slen + extra, 4);
+        pq_sendint(buf, slen + extra_self + extra_term, 4);        appendBinaryStringInfo(buf, str, slen);
+    }
+
+    /*
+     * str may not be null-terminated. Send the terminator separately from the
+     * given string.
+     */
+    if (sendterminator)
+        appendBinaryStringInfo(buf, "",  1);}/* --------------------------------
diff --git a/src/backend/replication/logical/proto.c b/src/backend/replication/logical/proto.c
index bc6e9b5..c5e4214 100644
--- a/src/backend/replication/logical/proto.c
+++ b/src/backend/replication/logical/proto.c
@@ -417,7 +417,6 @@ logicalrep_write_tuple(StringInfo out, Relation rel, HeapTuple tuple)        Form_pg_type typclass;
      Form_pg_attribute att = desc->attrs[i];        char       *outputstr;
 
-        int            len;        /* skip dropped columns */        if (att->attisdropped)
@@ -442,9 +441,12 @@ logicalrep_write_tuple(StringInfo out, Relation rel, HeapTuple tuple)        pq_sendbyte(out,
't');   /* 'text' data follows */        outputstr = OidOutputFunctionCall(typclass->typoutput, values[i]);
 
-        len = strlen(outputstr) + 1;    /* null terminated */
-        pq_sendint(out, len, 4);        /* length */
-        pq_sendstring(out, outputstr);    /* data */
+
+        /*
+         * Here, the string is sent as binary data so the length field counts
+         * terminating NULL.
+         */
+        pq_sendcountedtext(out, outputstr, strlen(outputstr), false, true);        pfree(outputstr);
diff --git a/src/backend/tcop/fastpath.c b/src/backend/tcop/fastpath.c
index 2efed95..0ca9522 100644
--- a/src/backend/tcop/fastpath.c
+++ b/src/backend/tcop/fastpath.c
@@ -160,7 +160,8 @@ SendFunctionResult(Datum retval, bool isnull, Oid rettype, int16 format)
getTypeOutputInfo(rettype,&typoutput, &typisvarlena);            outputstr = OidOutputFunctionCall(typoutput, retval);
 
-            pq_sendcountedtext(&buf, outputstr, strlen(outputstr), false);
+            pq_sendcountedtext(&buf, outputstr, strlen(outputstr),
+                               false, false);            pfree(outputstr);        }        else if (format == 1)
diff --git a/src/include/libpq/pqformat.h b/src/include/libpq/pqformat.h
index 4df87ec..e724b04 100644
--- a/src/include/libpq/pqformat.h
+++ b/src/include/libpq/pqformat.h
@@ -19,7 +19,7 @@ extern void pq_beginmessage(StringInfo buf, char msgtype);extern void pq_sendbyte(StringInfo buf, int
byt);externvoid pq_sendbytes(StringInfo buf, const char *data, int datalen);extern void pq_sendcountedtext(StringInfo
buf,const char *str, int slen,
 
-                   bool countincludesself);
+                       bool countincludesself, bool sendterminator);extern void pq_sendtext(StringInfo buf, const char
*str,int slen);extern void pq_sendstring(StringInfo buf, const char *str);extern void pq_send_ascii_string(StringInfo
buf,const char *str);