Large object corruption during 'piped' pg_restore

Bosco Rama <postgres@boscorama.com> writes:

We've discovered (or possibly rediscovered?) a corruption when restoring large
objects.

If 'standard_conforming_strings = on' is set in our DB (which is required for
our app) then the piped restore method (e.g. pg_restore -O backup.dat | psql)
results in the large objects being corrupted.

This vaguely reminds me of some long-since-fixed bug, but since you have
neglected to give any version details, it's hard to say for sure.
What PG version was the original server? The pg_dump you used to make
the backup? The pg_restore? The target server?

regards, tom lane

Hi Tom,

Tom Lane wrote:

Bosco Rama <postgres@boscorama.com> writes:

We've discovered (or possibly rediscovered?) a corruption when restoring large
objects.

If 'standard_conforming_strings = on' is set in our DB (which is required for
our app) then the piped restore method (e.g. pg_restore -O backup.dat | psql)
results in the large objects being corrupted.

This vaguely reminds me of some long-since-fixed bug, but since you have
neglected to give any version details, it's hard to say for sure.
What PG version was the original server? The pg_dump you used to make
the backup? The pg_restore? The target server?

Yeah, I only realized I hadn't added that as I saw it being sent. :-(

All servers and client tools involved are PG 8.4.6 on Ubuntu Server 10.04.1 LTS
with all current updates applied.

The dump file was made using: pg_dump -Fc -Z1 > backup.dat

Bosco.

Bosco Rama <postgres@boscorama.com> writes:

If 'standard_conforming_strings = on' is set in our DB (which is required for
our app) then the piped restore method (e.g. pg_restore -O backup.dat | psql)
results in the large objects being corrupted.

All servers and client tools involved are PG 8.4.6 on Ubuntu Server 10.04.1 LTS
with all current updates applied.

I've been able to replicate this in 8.4; it doesn't happen in 9.0
(but probably does in all 8.x versions).

The problem is that pg_dump (or in this case really pg_restore) is
relying on libpq's PQescapeBytea() to format the bytea literal that
will be given as argument to lowrite() during the restore. When
pg_dump is producing SQL directly, or when pg_restore is connected
to a database, PQescapeBytea() mooches the standard_conforming_strings
value from the active libpq connection and gets the right answer.
In the single case where pg_restore is producing SQL without ever
opening a database connection, PQescapeBytea doesn't know what to do
and defaults to the old non-standard-strings behavior. Unfortunately
pg_restore set standard_conforming_strings=on earlier in the script
(if it was set in the original source database) so you get the wrong
thing.

The bottom line is that pg_dump can't depend on libpq's PQescapeBytea,
but needs its own copy. We have in fact done that as of 9.0, which is
what I was vaguely remembering:

Author: Tom Lane <tgl@sss.pgh.pa.us>
Branch: master Release: REL9_0_BR [b1732111f] 2009-08-04 21:56:09 +0000

Fix pg_dump to do the right thing when escaping the contents of large objects.

The previous implementation got it right in most cases but failed in one:
if you pg_dump into an archive with standard_conforming_strings enabled, then
pg_restore to a script file (not directly to a database), the script will set
standard_conforming_strings = on but then emit large object data as
nonstandardly-escaped strings.

At the moment the code is made to emit hex-format bytea strings when dumping
to a script file. We might want to change to old-style escaping for backwards
compatibility, but that would be slower and bulkier. If we do, it's just a
matter of reimplementing appendByteaLiteral().

This has been broken for a long time, but given the lack of field complaints
I'm not going to worry about back-patching.

I'm not sure whether this new complaint is enough reason to reconsider
back-patching. We cannot just backport the 9.0 patch, since it assumes
it can do bytea hex output --- we'd need to emit old style escaped
output instead. So it's a bit of work, and more to the point would
involve pushing poorly-tested code into stable branches. I doubt it
would go wrong, but in the worst-case scenario we might create failures
for blob-restore cases that work now.

So I'm not sure whether to fix it, or leave it as a known failure case
in old branches. Comments?

regards, tom lane

On Thu, Jan 20, 2011 at 6:14 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

So I'm not sure whether to fix it, or leave it as a known failure case
in old branches.  Comments?

Since there is a workaround, I think it is best to document it and
leave it as-is.

Tom Lane wrote:

So I'm not sure whether to fix it, or leave it as a known failure case
in old branches. Comments?

I understand the reluctance to fool with stable code. I have zero insight
into your installed versions distribution and backward compatibility needs
so any comment I may have here is purely selfish.

As an end user there is one area of the DB that I want to work correctly
100% of the time and that is the dump/restore tool(s). If it's not going
to work under certain circumstances it should at least tell me so and
fail. I don't think having the tool appear to work while corrupting the
data (even if documented as doing so) is a viable method of operation.

Just my $0.02

Bosco.

On Fri, Jan 21, 2011 at 12:44 PM, Bosco Rama <postgres@boscorama.com> wrote:

Tom Lane wrote:

So I'm not sure whether to fix it, or leave it as a known failure case
in old branches.  Comments?

I understand the reluctance to fool with stable code.  I have zero insight
into your installed versions distribution and backward compatibility needs
so any comment I may have here is purely selfish.

As an end user there is one area of the DB that I want to work correctly
100% of the time and that is the dump/restore tool(s).  If it's not going
to work under certain circumstances it should at least tell me so and
fail.  I don't think having the tool appear to work while corrupting the
data (even if documented as doing so) is a viable method of operation.

Yeah, I lean toward saying we should back-patch this. Yeah, it'll be
lightly tested, but it's a pretty confined change, so it's unlikely to
break anything else. ISTM the worst case scenario is that it takes
two minor releases to get it right, and even that seems fairly
unlikely.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Jan 21, 2011 at 12:44 PM, Bosco Rama <postgres@boscorama.com> wrote:

Tom Lane wrote:

So I'm not sure whether to fix it, or leave it as a known failure case
in old branches. �Comments?

As an end user there is one area of the DB that I want to work correctly
100% of the time and that is the dump/restore tool(s).

Yeah, I lean toward saying we should back-patch this.

Fair enough, I'll go do it. I just wanted to hear at least one other
person opine that it was worth taking some risk for.

regards, tom lane