Re: [GENERAL] Gripe: bytea_output default => data corruption

Bruce Momjian wrote:

ljb wrote:

tgl@sss.pgh.pa.us wrote:

I think we should simply remove the description of *how* the escaping is
performed, and state only that the function produces a suitably escaped
literal string. Anything else is not future-proof, and could someday
break the way this wording did.

Perhaps it would be best to remove escaping details here. But the
description of PQescapeBytea() might need to be rewritten, too. Without
describing exactly what PQescapeByteaConn() does, it is hard to understand
what PQescapeBytea() does not do, and why it therefore "might give the
wrong results".

I think the actual function behavior should be documented somewhere. Even
though it might change again.

Based on this report, I have created the attached documentation patch
which clarifies the libpq behavior for escaping bytea. I am planning to
backpatch this to 9.0 as well.

My aplogies on the double-attachment. I am now attaching the right
verison.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

bruce@momjian.us wrote:

...

Based on this report, I have created the attached documentation patch
which clarifies the libpq behavior for escaping bytea. I am planning to
backpatch this to 9.0 as well.

This change says PQescapeBytea is unable to adjust its behavior based on
bytea_output, implying that PQescapeByteaConn does adjust its behavior
based on byte_output. Wrong! Neither one knows or cares about the bytea_output
parameter, which is solely for the backend to tell it how to present bytea
data to the front-end. Not for the front-ends at all.

(See why we need this behavior documented?)

Non-use of 'standard_conforming_strings' is also wrong, in the current
(pre-patched) docs. PQescapeBytea does adjust its behavior for
standard_conforming_strings, but only for the single-connection case. See
similar text for PQescapeString.

Regarding the first paragraph changed, it seems off to me. "PQescapeByteConn
escapes such bytes using either hex encoding..." tells me that when hex
encoding is used, PQescapeByte encodes only bytes that need escaping. Not
true - hex encoding encodes (not escapes) every byte.

ljb wrote:

bruce@momjian.us wrote:

...

Based on this report, I have created the attached documentation patch
which clarifies the libpq behavior for escaping bytea. I am planning to
backpatch this to 9.0 as well.

This change says PQescapeBytea is unable to adjust its behavior based on
bytea_output, implying that PQescapeByteaConn does adjust its behavior
based on byte_output. Wrong! Neither one knows or cares about the bytea_output
parameter, which is solely for the backend to tell it how to present bytea
data to the front-end. Not for the front-ends at all.

Good point, sorry.

(See why we need this behavior documented?)

Yep!

Non-use of 'standard_conforming_strings' is also wrong, in the current
(pre-patched) docs. PQescapeBytea does adjust its behavior for
standard_conforming_strings, but only for the single-connection case. See
similar text for PQescapeString.

Yep.

Regarding the first paragraph changed, it seems off to me. "PQescapeByteConn
escapes such bytes using either hex encoding..." tells me that when hex
encoding is used, PQescapeByte encodes only bytes that need escaping. Not
true - hex encoding encodes (not escapes) every byte.

OK, great feedback. I made the adjustments you suggested and tried to
pattern it after PQescapeStringConn, with adjustments where appropriate.

Here is an updated patch, again to be backpatched to 9.0. There were
obviously big mistakes in libpq docs and I am embarrassed I waited so
long to fix this; my apologies.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

bruce@momjian.us wrote:

Here is an updated patch, again to be backpatched to 9.0.

Getting better.

In change block 1, after PQescapeString synopsis:
Change: "does not take a Pgconn or error parameters"
to: "does not take Pgconn or error parameters"

In change block 4, PQescapeBytea, I don't like the change.
This: "is that PQescapeBytea does not take a PGconn or error parameters"
is wrong. PQescapeByteaConn does not take an 'error' parameter either.
Also "no way to report error conditions" is wrong. It returns NULL on an
error; it just cannot return a specific error message. (Which is OK because
there is only one, unlikely error: out of memory.)

Reading the before/after text here, I don't see anything that needs
changing except for dropping the reference to single-threaded (same as for
PQescapeString). I suggest going back to the original text and just
changing:
From: "can be used safely in single-threaded client programs that work with
only one"
to: "can only be used safely in client programs that use a single"

(Sorry for the trouble. I should have fully commented on the first one, but
I find it hard to see the changes with these "git" diffs of SGML.)

ljb wrote:

bruce@momjian.us wrote:

Here is an updated patch, again to be backpatched to 9.0.

Getting better.

In change block 1, after PQescapeString synopsis:
Change: "does not take a Pgconn or error parameters"
to: "does not take Pgconn or error parameters"

In change block 4, PQescapeBytea, I don't like the change.
This: "is that PQescapeBytea does not take a PGconn or error parameters"
is wrong. PQescapeByteaConn does not take an 'error' parameter either.
Also "no way to report error conditions" is wrong. It returns NULL on an
error; it just cannot return a specific error message. (Which is OK because
there is only one, unlikely error: out of memory.)

Reading the before/after text here, I don't see anything that needs
changing except for dropping the reference to single-threaded (same as for
PQescapeString). I suggest going back to the original text and just
changing:
From: "can be used safely in single-threaded client programs that work with
only one"
to: "can only be used safely in client programs that use a single"

(Sorry for the trouble. I should have fully commented on the first one, but
I find it hard to see the changes with these "git" diffs of SGML.)

OK, updated version attached. I keep most of the new patch for the
PQescapeBytea docs rather than use most of the original so it would
match text for other escape functions.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

bruce@momjian.us wrote:

...
OK, updated version attached. I keep most of the new patch for the
PQescapeBytea docs rather than use most of the original so it would
match text for other escape functions.

Looks good to me.

ljb wrote:

bruce@momjian.us wrote:

...
OK, updated version attached. I keep most of the new patch for the
PQescapeBytea docs rather than use most of the original so it would
match text for other escape functions.

Looks good to me.

Thanks for the reviews. Applied and backpatched to 9.0 (once git comes
back online).

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +