Re: BUG #7792: pg_dump does not treat -c flag correctly when using tar format

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> On Mon, Jan 7, 2013 at 1:32 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> emesika(at)redhat(dot)com writes:
>>> * restore.sql includes DROP statements for each object even tough -c flag
>>> was not given

>> I believe this is intentional - at least, pg_backup_tar.c goes out of
>> its way to make it happen. (The forcible setting of ropt->dropSchema
>> in _CloseArchive is the cause, and it's hard to see why that would be
>> there unless the author intended this effect.) Perhaps we should remove
>> that, but it would be an incompatible change. Arguing for or against
>> it really requires a model of what people would be doing with the
>> restore.sql script. I'm not entirely convinced that it should be
>> considered equivalent to what you'd get from a plain dump run.

> tar archives are more like custom archives ,arne't they? The idea
> being that since you use pg_restore to restore them, you should use -c
> on the pg_restore commandline, and not on the pg_dump one? And if we
> didn't include it in the dump, that wouldn't even be possible.

No, pg_restore depends only on what is in the TOC data structure.
The restore.sql script is just an auxiliary file that's there if you
extract the contents of the tar file --- pg_restore doesn't use it.

On reviewing the commit history, I find that pg_backup_tar.c was created
in commit c3e18804ff14f690a6d8c31b452476d0f8fcec28, and in that version
the code looked like

ropt = NewRestoreOptions();
ropt->dropSchema = 1;
ropt->compression = 0;

RestoreArchive((Archive*)AH, ropt);

where the recursive call to RestoreArchive is what scans the TOC and
prints the contents of restore.sql. I am not sure that in 2000 the
function even had the opportunity to pass down the outer restore options
to the recursive call, but it certainly didn't try. Somewhere along the
line we added a link to RestoreOptions to the ArchiveHandle data
structure, and since last year (commit
4317e0246c645f60c39e6572644cff1cb03b4c65) the code looks like

ropt = NewRestoreOptions();
+ memcpy(ropt, AH->ropt, sizeof(RestoreOptions));
ropt->dropSchema = 1;
ropt->compression = 0;
ropt->superuser = NULL;
ropt->suppressDumpWarnings = true;

+ savRopt = AH->ropt;
+ AH->ropt = ropt;
+
savVerbose = AH->public.verbose;
AH->public.verbose = 0;

- RestoreArchive((Archive *) AH, ropt);
+ RestoreArchive((Archive *) AH);

+ AH->ropt = savRopt;
AH->public.verbose = savVerbose;

So at this point passing down options is the norm and forcibly setting
dropSchema is the exception. (The forced clearing of superuser seems a
bit debatable now too.) Maybe we should change it, but it would
definitely be a user-visible behavioral change.

regards, tom lane