Re: Non-text mode for pg_dumpall

Thanks Jian for the detailed review and testing.

On Mon, 20 Jan 2025 at 21:32, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> hi.
> some minor issues come to my mind when I look at it again.
>
> looking at set_null_conf,
> i think "if (archDumpFormat != archNull)" can be:
>
> if (archDumpFormat != archNull)
> {
> OPF = fopen(toc_path, "w");
> if (!OPF)
> pg_fatal("could not open global.dat file: \"%s\" for writing: %m",
> toc_path);
> }
>
> some places we use ``fopen(filename, PG_BINARY_W)``,
> some places we use ``fopen(filename, "w");``
> kind of inconsistent...

Fixed. We should use PG_BINARY_W/PG_BINARY_R.

>
>
> + printf(_(" -F, --format=c|d|t|p output file format
> (custom, directory, tar,\n"
> + " plain text (default))\n"));
> this indentation level is not right?
> if we look closely at the surrounding output of `pg_dumpall --help`.

Fixed.

>
>
> pg_dump.sgml --create option description:
> This option is ignored when emitting an archive (non-text) output file. For the
> archive formats, you can specify the option when you call pg_restore.
>
> in runPgDump, we have:
> /*
> * If this is non-plain format dump, then append file name and dump
> * format to the pg_dump command to get archive dump.
> */
> if (archDumpFormat != archNull)
> {
> printfPQExpBuffer(&cmd, "\"%s\" -f %s %s", pg_dump_bin,
> dbfile, create_opts);
> ...
> }
>
> so in here, create_opts is not necessary per pg_dump.sgml above description.
> we can simplify it as:
>
> if (archDumpFormat != archNull)
> {
> printfPQExpBuffer(&cmd, "\"%s\" --file=%s", pg_dump_bin, dbfile);
> }
> ?

We are already using the same code without this patch also. I haven't
tested this without create_opts. I think, if your theory is right,
then
you can submit a patch for this change in another thread.

On Tue, 21 Jan 2025 at 09:37, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> hi.
>
> $BIN10/pg_restore --globals-only --verbose --file=test.sql x.dump
> it will create a "test.sql" file, but it should create file test.sql
> (no double quotes).

Fixed.

>
> ------<>>>>------
> if (archDumpFormat != archNull &&
> (!filename || strcmp(filename, "") == 0))
> {
> pg_log_error("options -F/--format=d|c|t requires option
> -f/--filename with non-empty string");
> ...
> }
> here, it should be
> pg_log_error("options -F/--format=d|c|t requires option -f/--file with
> non-empty string");

Fixed.

> ------<>>>>------
> the following pg_dumpall, pg_restore not working.
> $BIN10/pg_dumpall --format=custom --file=x1.dump --globals-only
> $BIN10/pg_restore --file=3.sql x1.dump
>
> ERROR: pg_restore: error: directory "x1.dump" does not appear to be a
> valid archive ("toc.dat" does not exist)

Fixed.

>
> these two also not working:
> $BIN10/pg_dumpall --format=custom --file=x1.dump --verbose --globals-only
> $BIN10/pg_restore --file=3.sql --format=custom x1.dump

Fixed.

>
> error message:
> pg_restore: error: could not read from input file: Is a directory

Fixed.

> ------<>>>>------
> IsFileExistsInDirectory function is the same as _fileExistsInDirectory.
> Can we make _fileExistsInDirectory extern function?

No, we can't make it as we are using this function in different-2 modules.

>
> + /* If global.dat and map.dat are exist, then proces them. */
> + if (IsFileExistsInDirectory(pg_strdup(inputFileSpec), "global.dat")
> + && IsFileExistsInDirectory(pg_strdup(inputFileSpec),
> "map.dat"))
> + {
> comment typo, "proces" should "process".

Fixed.

> here, we don't need pg_strdup?

In most places, we are dumping strings so I kept the same here also.

> ------<>>>>------
> # pg_restore tests
> +command_fails_like(
> + [
> + 'pg_restore', '-p', $port, '-f', $plainfile,
> + "--exclude-database=grabadge",
> + '--globals-only'
> + ],
> + qr/\Qg_restore: error: option --exclude-database cannot be used
> together with -g\/--globals-only\E/,
> + 'pg_restore: option --exclude-database cannot be used together
> with -g/--globals-only');
>
> We can put the above test on src/bin/pg_dump/t/001_basic.pl,
> since validating these conflict options don't need a cluster to be set up.

Done.

>
>
> typedef struct SimpleDatabaseOidListCell
> and
> typedef struct SimpleDatabaseOidList
> need also put into src/tools/pgindent/typedefs.list

Fixed.

On Tue, 21 Jan 2025 at 15:00, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> hi.
>
> + printfPQExpBuffer(query,
> + "SELECT substring ( "
> + " '%s' , "
> + " '%s' ) ", str, ptrn);
> + result = executeQuery(conn, query->data);
> + if (PQresultStatus(result) == PGRES_TUPLES_OK)
> + {
> + if (PQntuples(result) == 1)
> + {
> + const char *outstr;
> +
> + outstr = PQgetvalue(result, 0, 0);
> i think here you should use PQgetisnull(result, 0, 0)

Fixed.

>
> example: pg_dumpall and pg_restore:
> $BIN10/pg_dumpall --verbose --format=custom --file=x12.dump
> $BIN10/pg_restore --verbose --dbname=src10 x12.dump
>
> some log message for the above command:
> pg_restore: found dbname as : "template1" and db_oid:1 in map.dat file
> while restoring
> pg_restore: found dbname as : "s1" and db_oid:17960 in map.dat file
> while restoring
> pg_restore: found dbname as : "src10" and db_oid:5 in map.dat file
> while restoring
> pg_restore: found total 3 database names in map.dat file
> pg_restore: needs to restore 3 databases out of 3 databases
> pg_restore: restoring dump of pg_dumpall without -C option, there
> might be multiple databases in directory.
> pg_restore: restoring database "template1"
> pg_restore: connecting to database for restore
> pg_restore: implied data-only restore
> pg_restore: restoring database "s1"
> pg_restore: connecting to database for restore
> pg_restore: processing data for table "public.t"
> pg_restore: while PROCESSING TOC:
> pg_restore: from TOC entry 3376; 0 17961 TABLE DATA t jian
> pg_restore: error: could not execute query: ERROR: relation
> "public.t" does not exist
> Command was: COPY public.t (a) FROM stdin;
>
>
> 1. message: "pg_restore: implied data-only restore"
> Normally pg_dump and pg_restore will dump the schema and the data,
> then when we are connecting to the same database with pg_restore,
> there will be lots of schema elements already exists ERROR.
> but the above command case, pg_restore only restores the content/data
> not schema, that's why there is very little error happening.
> so here pg_restore not restore schema seems not ok?
>
>
> 2. pg_dumpall with non-text mode, we don't have \connect command in
> file global.dat or map.dat
> I have database "s1" with table "public.t".
> if I create a table src10.public.t (database.schema.table) with column a.
> then pg_restore will restore content of s1.public.t (database s1) to
> src10.public.t (database src10).
>
> in ConnectDatabase(Archive *AHX,
> const ConnParams *cparams,
> bool isReconnect)
> i added
> if (cparams->dbname)
> fprintf(stderr, "pg_backup_db.c:%d %s called connecting to %s
> now\n", __LINE__, __func__, cparams->dbname);
> to confirm that we are connecting the same database "src10", while
> dumping all the contents in x12.dump.

I will do some more study for this and will update. As of now, I added
the "--create" option in the dump.

Here, I am attaching an updated patch for review and testing.

--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com