Re: Non-text mode for pg_dumpall

Thanks Jian for review, testing and delta patches.

On Wed, 29 Jan 2025 at 15:09, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> hi.
>
> we need to escape the semicolon within the single quotes or double quotes.
> I think my patch in [1] is correct.
>
> we can have "ERROR: role "z" already exists
> but
> error message like
> pg_restore: error: could not execute query: "ERROR: unterminated
> quoted string at or near "';
> should not be accepted in execute_global_sql_commands, ReadOneStatement,
PQexec
>
> attached is the all the corner test case i come up with against
> ReadOneStatement.
> your v13 will generate errors like "ERROR: unterminated quoted string
> at or near ..."',
> which is not good, i think.
>
> [1]
https://www.postgresql.org/message-id/CACJufxEQUcjBocKJQ0Amf3AfiS9wFB7zYSHrj1qqD_oWeaJoGQ%40mail.gmail.com

Yes, you are right. We can't read line by line. We should read char by char
and we need some extra handling for double quote names.
I have merged your delta patch into this and now I am doing some more
testing for corner cases of this type of names.
*Ex*: add some comments in names etc or multiple semicolons or other
special characters in name.

On Fri, 31 Jan 2025 at 09:23, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> hi.
>
> -extern void RestoreArchive(Archive *AHX);
> +extern void RestoreArchive(Archive *AHX, bool append_data);
> Can we spare some words to explain the purpose of append_data.

Fixed. I added some comments on the top of the RestoreArchive function.

>
> in get_dbname_oid_list_from_mfile
> pg_log_info("map.dat file is not present in dump of
> pg_dumpall, so nothing to restore.");
> maybe we can change it to
> pg_log_info("databases restoring is skipped as map.dat file is
> not present in \"%s\"", dumpdirpath);

Fixed.

> we can aslo add Assert(dumpdirpath != NULL)

No, we don't need it as we are already checking inputfileSpec!= NULL.

>
> pg_log_info("found dbname as : \"%s\" and db_oid:%u in map.dat file
> while restoring", dbname, db_oid);
> also need to change. maybe
> pg_log_info("found database \"%s\" (OID: %u) in map.dat file while
> restoring.", dbname, db_oid);

Fixed.

>
> I also did some minor refactoring, please check attached.

Thanks. I merged it.

>
>
> doc/src/sgml/ref/pg_restore.sgml
> <refnamediv>
> <refname>pg_restore</refname>
>
> <refpurpose>
> restore a <productname>PostgreSQL</productname> database from an
> archive file created by <application>pg_dump</application>
> </refpurpose>
> </refnamediv>
> need to change, since now we can restore multiple databases.

Agreed. I added some comments.

>
> doc/src/sgml/ref/pg_dumpall.sgml
> <refnamediv>
> <refname>pg_dumpall</refname>
> <refpurpose>extract a <productname>PostgreSQL</productname> database
> cluster into a script file</refpurpose>
> </refnamediv>
> also need change.
On Sat, 1 Feb 2025 at 21:36, Srinath Reddy <srinath2133(at)gmail(dot)com> wrote:
>
> Hi,
> i think we have to change the pg_dumpall "--help" message similar to
pg_dump's specifying that now pg_dumpall dumps cluster into to other
non-text formats.
> Need similar "--help" message change in pg_restore to specify that now
pg_restore supports restoring whole cluster from archive created from
pg_dumpall.

As Jian suggested, we need to change docs so I did the same changes into
doc and --help also.

On Fri, 31 Jan 2025 at 14:22, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> hi.
> more small issues.
>
> + count_db++; /* Increment db couter. */
> + dboidprecell = dboid_cell;
> + }
> +
> typo, "couter" should be "counter".

Fixed.

>
> +
> +/*
> + * get_dbname_oid_list_from_mfile
> + *
> + * Open map.dat file and read line by line and then prepare a list of
database
> + * names and correspoding db_oid.
> + *
> typo, "correspoding" should be "corresponding".

Fixed.

>
>
> execute_global_sql_commands comments didn't mention ``IF (outfile) ``
> branch related code.
> We can add some comments saying that
> ""IF opts->filename is not specified, then copy the content of
> global.dat to opts->filename""".

We already have some comments on the top of the execute_global_sql_commands
function.

>
> or split it into two functions.

Done. I added a new function for outfile.

>
>
> + while((fgets(line, MAXPGPATH, pfile)) != NULL)
> + {
> + Oid db_oid;
> + char db_oid_str[MAXPGPATH + 1];
> + char dbname[MAXPGPATH + 1];
> +
> + /* Extract dboid. */
> + sscanf(line, "%u" , &db_oid);
> + sscanf(line, "%s" , db_oid_str);
> +
> + /* Now copy dbname. */
> + strcpy(dbname, line + strlen(db_oid_str) + 1);
> +
> + /* Remove \n from dbanme. */
> + dbname[strlen(dbname) - 1] = '\0';
> +
> + pg_log_info("found dbname as : \"%s\" and db_oid:%u in map.dat file
> while restoring", dbname, db_oid);
> +
> + /* Report error if file has any corrupted data. */
> + if (!OidIsValid(db_oid) || strlen(dbname) == 0)
> + pg_fatal("invalid entry in map.dat file at line : %d", count + 1);
> +
> + /*
> + * XXX : before adding dbname into list, we can verify that this db
> + * needs to skipped for restore or not but as of now, we are making
> + * a list of all the databases.
> + */
> + simple_db_oid_list_append(dbname_oid_list, db_oid, dbname);
> + count++;
> + }
>
>
> db_oid first should be set to 0, dbname first character first should be
set to 0
> (char dbname[0] = '\0') before sscanf call.
> so if sscanf fail, the db_oid and dbname value is not undermined)

Okay. Fixed.

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

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