Thread: pg_restore ignore error patch
Dear patchers, please find a small patch submission so that "pg_restore" ignores some sql errors. The implementation seems quite reasonnable to me, but pg-gods may have a different opinion. Two fields are added to the ArchiveHandler to trigger the behavior. A count summary of ignored sql errors is printed at the end of the restoration. I did not fixed the "set session auth" attempt as option "-O" can already be used to avoid it. It validates for me, but it seems that pg_dump/pg_restore is just *NOT* tested at all in the validation:-(. So at least I tested it. My tests suggest that a feature of pg_restore is that it is only expected to work with its own server. Indeed, it generates some new syntax, such as $$ quoting, which is not compatible with older servers. This fact does not seem to appear in the documentation. Have a nice day, -- Fabien Coelho - coelho@cri.ensmp.fr
Attachment
Fabien COELHO said: > > > My tests suggest that a feature of pg_restore is that it is only > expected to work with its own server. Indeed, it generates some new > syntax, such as $$ quoting, which is not compatible with older servers. > This fact does not seem to appear in the documentation. > Wrong. It does appear in the documentation, and it can also be disabled. http://developer.postgresql.org/docs/postgres/app-pgdump.html says: -X disable-dollar-quoting --disable-dollar-quoting This option disables the use of dollar quoting for function bodies, and forces them to be quoted using SQL standard string syntax. cheers andrew
Fabien COELHO <coelho@cri.ensmp.fr> writes:
> please find a small patch submission so that "pg_restore" ignores some sql
> errors.
Yeah, we've been talking about doing that for awhile. But please define
"some errors" --- what do you ignore exactly?
+ if (AH->n_errors)
+ /* translator: %s stands for "error" or "errors" */
+ fprintf(stderr, _("warning: %d %s ignored on restore\n"),
+ /* translator: in sentence warning: 123 errors ignored... */
+ AH->n_errors, AH->n_errors>1? _("errors"): _("error"));
Please read the message style guidelines: the above goes directly
against the advice for writing translatable messages.
Also, it might be wise to return a nonzero exit code when any errors are
ignored. I'm not sure about that, but it might be best to err on the
side of caution...
regards, tom lane
> > My tests suggest that a feature of pg_restore is that it is only > > expected to work with its own server. Indeed, it generates some new > > syntax, such as $$ quoting, which is not compatible with older servers. > > This fact does not seem to appear in the documentation. > > Wrong. It does appear in the documentation, and it can also be disabled. Good! > http://developer.postgresql.org/docs/postgres/app-pgdump.html says: I looked briefly at *pg_restore* documentation, as I was interested in pg_restore. But the logic is just mine;-) Well, it means that you must decide a dump time if you may have to restore in an older version. I can guess why it eased the implementation, but it does not look good. -- Fabien Coelho - coelho@cri.ensmp.fr
> > please find a small patch submission so that "pg_restore" ignores some sql
> > errors.
>
> Yeah, we've been talking about doing that for awhile. But please define
> "some errors" --- what do you ignore exactly?
Connection errors are not ignored.
The error is triggered directly by the execute sql function, so it is hard
to know what is going on there. There are 3 instances. I've skipped the
"set session authorization" stuff, but 2 others are filtered.
A more complete implementation would allow to ignore errors on language
restoration or things like that, or initial cleanups... But that would
require to change the current code structure a lot, so as to avoid direct
exits... Moreover it does not look really necessary from my point of view.
I just aim at having direct "pg_restore" behave as "pg_restore|psql".
> + if (AH->n_errors)
> + /* translator: %s stands for "error" or "errors" */
> + fprintf(stderr, _("warning: %d %s ignored on restore\n"),
> + /* translator: in sentence warning: 123 errors ignored... */
> + AH->n_errors, AH->n_errors>1? _("errors"): _("error"));
>
> Please read the message style guidelines: the above goes directly
> against the advice for writing translatable messages.
Ok.
> Also, it might be wise to return a nonzero exit code when any errors are
> ignored. I'm not sure about that, but it might be best to err on the
> side of caution...
Ok.
I'll resubmit.
--
Fabien Coelho - coelho@cri.ensmp.fr
Fabien COELHO said: > > > Well, it means that you must decide a dump time if you may have to > restore in an older version. I can guess why it eased the > implementation, but it does not look good. > You want to be able to backup from version x to version y for some y<x? We support upgrades, not downgrades. What doesn't look good about that? Upgrading *does* look good - pg_dump understands older versions going back quite some way, but it sensibly assumes that the dump will be restored into a database running its version. The main reason we allowed for disabling dollar quoting is that the current CVS version fixes some other nasty problems, and people might wish to use it in preference to the released 7.4 version, with which it is otherwise compatible, before the next release, and 7.4 of course does not understand dollar quoting. cheers andrew
Dear Andrew, > > Well, it means that you must decide a dump time if you may have to > > restore in an older version. I can guess why it eased the > > implementation, but it does not look good. > > You want to be able to backup from version x to version y for some y<x? We > support upgrades, not downgrades. What doesn't look good about that? Ok, I understand your comment about upgrading. However most of the time I use dump/restore as a backup/transfer facility, and seldom as an upgrade tool. I transfer some data from one server to the other. On such occasion, I use my laptop to connect with server X, I download the data, then I restore them to server Y. The versions on my laptop and both servers are likely to be different. My laptop is likely to have some development version, and the servers may be in 7.3 or 7.4. This may not be the intended use of pg_dump and pg_restore, but this is what I do. Hence from this it would be nice if pg_restore could restore to older versions. Hence my comment "it does not look good". It is just a question of perspective. Have a nice day, -- Fabien Coelho - coelho@cri.ensmp.fr
Fabien COELHO said: > > > I transfer some data from one server to the other. On such occasion, I > use my laptop to connect with server X, I download the data, then I > restore them to server Y. The versions on my laptop and both servers > are likely to be different. My laptop is likely to have some > development version, and the servers may be in 7.3 or 7.4. > What problems have you encountered other than dollar quoting? In the most general case, ISTM you would need to teach pg_dump how to degrade gracefully, and a m:1 sources to targets relationship suddenly becomes m:m. I'm not convinced it is worth the trouble. cheers andrew
Dear Andrew, > > I transfer some data from one server to the other. On such occasion, I > > use my laptop to connect with server X, I download the data, then I > > restore them to server Y. The versions on my laptop and both servers > > are likely to be different. My laptop is likely to have some > > development version, and the servers may be in 7.3 or 7.4. > > What problems have you encountered other than dollar quoting? If you're interested to know, there were some complaints about the new "default_with_oids" setting, as far as I can remember. > In the most general case, ISTM you would need to teach pg_dump how to > degrade gracefully, and a m:1 sources to targets relationship suddenly > becomes m:m. I'm not convinced it is worth the trouble. I don't know what is worth the trouble. I'm just reporting a "feature" that make the tool not work properly for me. I've fixed part of the problem and submitted a patch, and I just noted in the mail that there was another small problem that I did not fix. I agree that fixing this would mean redesigning the tool in depth. That's why I did not fixed it in the patch I submitted. My opinion would be that pg_dump should just dump the data in some raw format, and that all syntax re-generation issues should be dealt with by pg_restore (INSERT vs COPY, quoting, and so on). But the tool was not resigned this way in the beginning. -- Fabien Coelho - coelho@cri.ensmp.fr
Fabien COELHO wrote: > > Dear patchers, > > please find a small patch submission so that "pg_restore" ignores some sql > errors. > > The implementation seems quite reasonnable to me, but pg-gods may have a > different opinion. Two fields are added to the ArchiveHandler to trigger > the behavior. A count summary of ignored sql errors is printed at the end > of the restoration. > > I did not fixed the "set session auth" attempt as option "-O" can already > be used to avoid it. > > It validates for me, but it seems that pg_dump/pg_restore is just *NOT* > tested at all in the validation:-(. So at least I tested it. > > My tests suggest that a feature of pg_restore is that it is only expected > to work with its own server. Indeed, it generates some new syntax, such as > $$ quoting, which is not compatible with older servers. This fact does not > seem to appear in the documentation. I looked over the patch and it seems to continue on pg_restore errors by default. That isn't good. By default, any error should make it exit loudly. I can see value in having a flag that allows pg_restore to continue on errors, particularly because only pg_restore can read binary dumps. Do folks get pg_restor failures frequently? I sure hope not. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I looked over the patch and it seems to continue on pg_restore errors by
> default. That isn't good. By default, any error should make it exit
> loudly.
I'm not sure of that. pg_dump is really designed and tested for the
case of text dump to a psql script, and if there is an error in the psql
script, psql will print it out and continue. In the cases I have seen
where pg_restore operates differently from that paradigm, the pg_restore
behavior of curling up and dying has *never* been the right thing.
I haven't looked at the details of Fabien's patch, but I'm in agreement
with the overall goal.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > I looked over the patch and it seems to continue on pg_restore errors by > > default. That isn't good. By default, any error should make it exit > > loudly. > > I'm not sure of that. pg_dump is really designed and tested for the > case of text dump to a psql script, and if there is an error in the psql > script, psql will print it out and continue. In the cases I have seen > where pg_restore operates differently from that paradigm, the pg_restore > behavior of curling up and dying has *never* been the right thing. > > I haven't looked at the details of Fabien's patch, but I'm in agreement > with the overall goal. Oh, OK, so make it behave like pg_dump's text output piped into psql. The patch is quite clean and just changes die to warn for pg_restore, and reports the number of errors on completion. The only thing that it needs is to change pg_restore so it returns a non-zero error if there were any warnings printed. I can make that change easily. I will add it to the patch queue. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
> > > I looked over the patch and it seems to continue on pg_restore errors by > > > default. That isn't good. By default, any error should make it exit > > > loudly. > > > > I'm not sure of that. pg_dump is really designed and tested for the > > case of text dump to a psql script, and if there is an error in the psql > > [...] > > Oh, OK, so make it behave like pg_dump's text output piped into psql. It is really easy to add an option to allow user change the 'ignore' behavior, and make pg_restore exit loudly if it is desired. Maybe it should be proposed just for backwards compatibility? -- Fabien Coelho - coelho@cri.ensmp.fr
Fabien COELHO wrote: > > > > > I looked over the patch and it seems to continue on pg_restore errors by > > > > default. That isn't good. By default, any error should make it exit > > > > loudly. > > > > > > I'm not sure of that. pg_dump is really designed and tested for the > > > case of text dump to a psql script, and if there is an error in the psql > > > [...] > > > > Oh, OK, so make it behave like pg_dump's text output piped into psql. > > It is really easy to add an option to allow user change the 'ignore' > behavior, and make pg_restore exit loudly if it is desired. > > Maybe it should be proposed just for backwards compatibility? Let's see if anyone asks for it. Unless they do, we will just enable it by default. As long as we exit with a non-zero status on error, just like psql, I think we are OK. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073