Re: pg_restore direct to database is broken for --insert dumps

In http://archives.postgresql.org/pgsql-admin/2012-01/msg00008.php
it's pointed out that recent versions of pg_restore fall over on
archives made with -Fc --inserts (or --column-inserts), but only when
restoring direct to database; if you ask for text output it's perfectly
fine. Investigation shows that the problem is that individual INSERT
commands are being broken apart at arbitrary buffer boundaries --- you
don't see any problem in text output, but when the bufferloads are
submitted as separate PQexec calls, of course bad things happen.

I believe this worked okay up until my patch here:
http://git.postgresql.org/gitweb/?p=postgresql.git&a=commitdiff&h=6545a901a
which removed the mini SQL lexer in pg_backup_db.c. I had supposed that
that had no useful function except to separate COPY data from
not-COPY-data, but in reality it had another function of ensuring that
INSERT commands split across zlib bufferload boundaries would get
reassembled before they are submitted to PQexec.

Not entirely sure what to do about this. We could consider reverting
the aforesaid patch and trying to find another way of fixing that code's
failure to cope with standard-conforming strings, but I'm not sure that
there's a good way to know whether standard_conforming_strings is active
here, and anyway that code was ugly as sin and I'd rather not resurrect
it. But on the other hand, there are no clear line boundaries in the
compressed data, and we can't introduce any without (a) worsening
compression and (b) breaking compatibility with existing dump files.

Anybody have any bright ideas? I'm fresh out at the moment.

regards, tom lane

On 01/04/2012 01:13 PM, Tom Lane wrote:
> In http://archives.postgresql.org/pgsql-admin/2012-01/msg00008.php
> it's pointed out that recent versions of pg_restore fall over on
> archives made with -Fc --inserts (or --column-inserts), but only when
> restoring direct to database; if you ask for text output it's perfectly
> fine. Investigation shows that the problem is that individual INSERT
> commands are being broken apart at arbitrary buffer boundaries --- you
> don't see any problem in text output, but when the bufferloads are
> submitted as separate PQexec calls, of course bad things happen.
>
> I believe this worked okay up until my patch here:
> http://git.postgresql.org/gitweb/?p=postgresql.git&a=commitdiff&h=6545a901a
> which removed the mini SQL lexer in pg_backup_db.c. I had supposed that
> that had no useful function except to separate COPY data from
> not-COPY-data, but in reality it had another function of ensuring that
> INSERT commands split across zlib bufferload boundaries would get
> reassembled before they are submitted to PQexec.
>
> Not entirely sure what to do about this. We could consider reverting
> the aforesaid patch and trying to find another way of fixing that code's
> failure to cope with standard-conforming strings, but I'm not sure that
> there's a good way to know whether standard_conforming_strings is active
> here, and anyway that code was ugly as sin and I'd rather not resurrect
> it. But on the other hand, there are no clear line boundaries in the
> compressed data, and we can't introduce any without (a) worsening
> compression and (b) breaking compatibility with existing dump files.
>
> Anybody have any bright ideas? I'm fresh out at the moment.
>
>

I pondered this while out on my daily constitutional. The first thing
that occurred to me is that it would possibly have been better if we'd
made pg_dump not use a quoting mechanism whose behaviour is dependent on
a setting (e.g. E'' or dollar quoting). But I guess that's water under
the bridge.

Could we detect an appropriate line ending in ahwrite() after it's been
decompressed and buffer partial lines accordingly?

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 01/04/2012 01:13 PM, Tom Lane wrote:
>> Not entirely sure what to do about this. We could consider reverting
>> the aforesaid patch and trying to find another way of fixing that code's
>> failure to cope with standard-conforming strings, but I'm not sure that
>> there's a good way to know whether standard_conforming_strings is active
>> here, and anyway that code was ugly as sin and I'd rather not resurrect
>> it. But on the other hand, there are no clear line boundaries in the
>> compressed data, and we can't introduce any without (a) worsening
>> compression and (b) breaking compatibility with existing dump files.
>>
>> Anybody have any bright ideas? I'm fresh out at the moment.

> I pondered this while out on my daily constitutional. The first thing
> that occurred to me is that it would possibly have been better if we'd
> made pg_dump not use a quoting mechanism whose behaviour is dependent on
> a setting (e.g. E'' or dollar quoting). But I guess that's water under
> the bridge.

> Could we detect an appropriate line ending in ahwrite() after it's been
> decompressed and buffer partial lines accordingly?

Not easily: there could be newlines embedded in data strings or SQL
identifiers. I'm not seeing any way around this except to restore the
minimal lexing capability. One thing we could probably do is to
restrict it to be used only when reading table data, and continue to
assume that object creation commands can be emitted as-is. That would
at least get us out of needing to parse dollar-quoted literals, which
aren't used in the INSERT commands. But we'd have to deal with
standard-conforming strings some way. The idea I had about that, since
we have an open database connection at hand, is to check the connected
backend's standard_conforming_strings state via PQparameterStatus. If
it doesn't have the right setting then things are going to fail anyway.

regards, tom lane

On 01/04/2012 06:20 PM, Tom Lane wrote:
> But we'd have to deal with
> standard-conforming strings some way. The idea I had about that, since
> we have an open database connection at hand, is to check the connected
> backend's standard_conforming_strings state via PQparameterStatus. If
> it doesn't have the right setting then things are going to fail anyway.
>

Do we care what it is? Or can we just issue a SET to make it what it
needs to be?

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 01/04/2012 06:20 PM, Tom Lane wrote:
>> But we'd have to deal with
>> standard-conforming strings some way. The idea I had about that, since
>> we have an open database connection at hand, is to check the connected
>> backend's standard_conforming_strings state via PQparameterStatus. If
>> it doesn't have the right setting then things are going to fail anyway.

> Do we care what it is?

Well, yeah, we care.

> Or can we just issue a SET to make it what it needs to be?

We already did, but I don't think the code in pg_backup_db has
convenient access to the value. I might be wrong about that.

regards, tom lane

On 01/04/2012 06:20 PM, Tom Lane wrote:
>> Could we detect an appropriate line ending in ahwrite() after it's been
>> decompressed and buffer partial lines accordingly?
> Not easily: there could be newlines embedded in data strings or SQL
> identifiers.
>

Should we look at eliminating those newlines for the future by using
U&"" identifiers where there are embedded newlines and unicode escapes
for newlines in data strings?

Then at least we'd possibly be able to get rid of the kludge some time
in the future.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 01/04/2012 06:20 PM, Tom Lane wrote:
>> Not easily: there could be newlines embedded in data strings or SQL
>> identifiers.

> Should we look at eliminating those newlines for the future by using
> U&"" identifiers where there are embedded newlines and unicode escapes
> for newlines in data strings?

> Then at least we'd possibly be able to get rid of the kludge some time
> in the future.

Given our high expectations for forward-compatibility of dump files, we
couldn't drop the code anytime in the foreseeable future, so I cannot
get excited about spending time on that. (AFAIR, we have *never*
intentionally broken compatibility of pg_dump files, unless you count
breaking dumps made by unreleased development versions.)

regards, tom lane

I wrote:
> Not easily: there could be newlines embedded in data strings or SQL
> identifiers. I'm not seeing any way around this except to restore the
> minimal lexing capability. One thing we could probably do is to
> restrict it to be used only when reading table data, and continue to
> assume that object creation commands can be emitted as-is. That would
> at least get us out of needing to parse dollar-quoted literals, which
> aren't used in the INSERT commands.

Attached is a patch that restores a much-simplified version of the mini
lexer; it deals only with the sorts of things that dumpTableData_insert
actually emits. Fixing the standard_conforming_strings issue turns out
to be really a one-liner, because the setting is in fact readily
available to this code. Maybe we should have done it that way to begin
with :-( ... though I admit to being glad to have gotten rid of the very
questionable dollar-quote-recognition code that was there before.

Barring objections to the approach, I'll apply and back-patch this
tomorrow.

regards, tom lane