Thread: Non-text mode for pg_dumpall

Tom and Nathan opined recently that providing for non-text mode for 
pg_dumpall would be a Good Thing (TM). Not having it has been a 
long-standing complaint, so I've decided to give it a go.

I think we would need to restrict it to directory mode, at least to 
begin with. I would have a toc.dat with a different magic block (say 
"PGGLO" instead of "PGDMP") containing the global entries (roles, 
tablespaces, databases). Then for each database there would be a 
subdirectory (named for its toc entry) with a standard directory mode 
dump for that database. These could be generated in parallel (possibly 
by pg_dumpall calling pg_dump for each database). pg_restore on 
detecting a global type toc.data would restore the globals and then each 
of the databases (again possibly in parallel).

I'm sure there are many wrinkles I haven't thought of, but I don't see 
any insurmountable obstacles, just a significant amount of code.

Barring the unforeseen my main is to have a preliminary patch by the 
September CF.

Following that I would turn my attention to using it in pg_upgrade.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Mon, Jun 10, 2024 at 08:58:49AM -0400, Andrew Dunstan wrote:
> Tom and Nathan opined recently that providing for non-text mode for
> pg_dumpall would be a Good Thing (TM). Not having it has been a
> long-standing complaint, so I've decided to give it a go.

Thank you!

> I think we would need to restrict it to directory mode, at least to begin
> with. I would have a toc.dat with a different magic block (say "PGGLO"
> instead of "PGDMP") containing the global entries (roles, tablespaces,
> databases). Then for each database there would be a subdirectory (named for
> its toc entry) with a standard directory mode dump for that database. These
> could be generated in parallel (possibly by pg_dumpall calling pg_dump for
> each database). pg_restore on detecting a global type toc.data would restore
> the globals and then each of the databases (again possibly in parallel).

I'm curious why we couldn't also support the "custom" format.

> Following that I would turn my attention to using it in pg_upgrade.

+1

-- 
nathan

On 2024-06-10 Mo 10:14, Nathan Bossart wrote:
> On Mon, Jun 10, 2024 at 08:58:49AM -0400, Andrew Dunstan wrote:
>> Tom and Nathan opined recently that providing for non-text mode for
>> pg_dumpall would be a Good Thing (TM). Not having it has been a
>> long-standing complaint, so I've decided to give it a go.
> Thank you!
>
>> I think we would need to restrict it to directory mode, at least to begin
>> with. I would have a toc.dat with a different magic block (say "PGGLO"
>> instead of "PGDMP") containing the global entries (roles, tablespaces,
>> databases). Then for each database there would be a subdirectory (named for
>> its toc entry) with a standard directory mode dump for that database. These
>> could be generated in parallel (possibly by pg_dumpall calling pg_dump for
>> each database). pg_restore on detecting a global type toc.data would restore
>> the globals and then each of the databases (again possibly in parallel).
> I'm curious why we couldn't also support the "custom" format.


We could, but the housekeeping would be a bit harder. We'd need to keep 
pointers to the offsets of the per-database TOCs (I don't want to have a 
single per-cluster TOC). And we can't produce it in parallel, so I'd 
rather start with something we can produce in parallel.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Mon, Jun 10, 2024 at 10:51:42AM -0400, Andrew Dunstan wrote:
> On 2024-06-10 Mo 10:14, Nathan Bossart wrote:
>> I'm curious why we couldn't also support the "custom" format.
> 
> We could, but the housekeeping would be a bit harder. We'd need to keep
> pointers to the offsets of the per-database TOCs (I don't want to have a
> single per-cluster TOC). And we can't produce it in parallel, so I'd rather
> start with something we can produce in parallel.

Got it.

-- 
nathan

On Mon, Jun 10, 2024 at 04:52:06PM +0200, Magnus Hagander wrote:
> On Mon, Jun 10, 2024 at 4:14 PM Nathan Bossart <nathandbossart@gmail.com>
> wrote:
>> I'm curious why we couldn't also support the "custom" format.
> 
> Or maybe even a combo - a directory of custom format files? Plus that one
> special file being globals? I'd say that's what most use cases I've seen
> would prefer.

Is there a particular advantage to that approach as opposed to just using
"directory" mode for everything?  I know pg_upgrade uses "custom" mode for
each of the databases, so a combo approach would be a closer match to the
existing behavior, but that doesn't strike me as an especially strong
reason to keep doing it that way.

-- 
nathan

On Mon, Jun 10, 2024 at 05:45:19PM +0200, Magnus Hagander wrote:
> On Mon, Jun 10, 2024 at 5:03 PM Nathan Bossart <nathandbossart@gmail.com>
> wrote:
>> Is there a particular advantage to that approach as opposed to just using
>> "directory" mode for everything?  I know pg_upgrade uses "custom" mode for
>> each of the databases, so a combo approach would be a closer match to the
>> existing behavior, but that doesn't strike me as an especially strong
>> reason to keep doing it that way.
> 
> A gazillion files to deal with? Much easier to work with individual custom
> files if you're moving databases around and things like that.
> Much easier to monitor eg sizes/dates if you're using it for backups.
> 
> It's not things that are make-it-or-break-it or anything, but there are
> some smaller things that definitely can be useful.

Makes sense, thanks for elaborating.

-- 
nathan

Magnus Hagander <magnus@hagander.net> writes:
> On Mon, Jun 10, 2024 at 5:03 PM Nathan Bossart <nathandbossart@gmail.com>
> wrote:
>> Is there a particular advantage to that approach as opposed to just using
>> "directory" mode for everything?

> A gazillion files to deal with? Much easier to work with individual custom
> files if you're moving databases around and things like that.
> Much easier to monitor eg sizes/dates if you're using it for backups.

You can always tar up the directory tree after-the-fact if you want
one file.  Sure, that step's not parallelized, but I think we'd need
some non-parallelized copying to create such a file anyway.

            regards, tom lane

On 2024-06-10 Mo 12:21, Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> On Mon, Jun 10, 2024 at 5:03 PM Nathan Bossart <nathandbossart@gmail.com>
>> wrote:
>>> Is there a particular advantage to that approach as opposed to just using
>>> "directory" mode for everything?
>> A gazillion files to deal with? Much easier to work with individual custom
>> files if you're moving databases around and things like that.
>> Much easier to monitor eg sizes/dates if you're using it for backups.
> You can always tar up the directory tree after-the-fact if you want
> one file.  Sure, that step's not parallelized, but I think we'd need
> some non-parallelized copying to create such a file anyway.
>
>             


Yeah.

I think I can probably allow for Magnus' suggestion fairly easily, but 
if I have to choose I'm going to go for the format that can be produced 
with the maximum parallelism.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Thu, Jan 02, 2025 at 02:05:13AM +0530, Mahendra Singh Thalor wrote:
> Here, I am attaching an updated patch. I fixed some bugs of v01 patch and
> did some code cleanup also.

Thank you for picking this up!  I started to review it, but the
documentation changes didn't build, and a few tests in check-world are
failing.  Would you mind resolving those issues?  Also, if you haven't
already, please add an entry to the next commitfest [0] to ensure that 1)
this feature is tracked and 2) the automated tests will run.

+    if (dbfile)
+    {
+        printfPQExpBuffer(&cmd, "\"%s\" %s %s", pg_dump_bin,
+                          dbfile, create_opts);
+        appendPQExpBufferStr(&cmd, " -F d ");
+    }

Have you given any thought to allowing a directory of custom format files,
as discussed upthread [1]?  Perhaps that is better handled as a follow-up
patch, but it'd be good to understand the plan, anyway.

[0] https://commitfest.postgresql.org
[1] https://postgr.es/m/CABUevExoQ26jo%2BaQ9QZq%2BUMA1aD6gfpm9xBnh_t5e0DhaCeRYA%40mail.gmail.com

-- 
nathan

> hi.
> After some tests and thinking about your reply, I admit that using
> expand_dbname_patterns
> in pg_restore will not work.
> We need to do pattern matching against the map.dat file.
> Please check the attached v12 series based on your
> v11_pg_dumpall-with-directory-tar-custom-format-21-jan.patch
>
> v12-0001 cosmetic change.
> v12-0002 implement pg_resore --exclude-database=PATTERN.
> main gist of implementation:
> for each database name in map.dat file,
> check if this database name pattern matches with PATTERN or not.
> pattern matching is using processSQLNamePattern.
>
> your substring will not work.
> some of the test cases.
> $BIN10/pg_restore --exclude-database=* -Cd template1 --verbose dir10 >
> dir_format 2>&1

Hi,
As per discussion with Robert Haas and Dilip Kumar, we thought that we
can't assume that
there will be a db connection every time while doing pg_restore but in
attached patch, we are
assuming that we have a db connection.
In my previous updates, I already mentioned this problem. I think, we
should not use connection
for --exclude-database, rather we should use direct functions to
validate patterns or we should
restrict as NAME only.

On Sun, 26 Jan 2025 at 20:17, jian he <jian.universality@gmail.com> wrote:
>
> hi.
> attached patching trying to refactor ReadOneStatement
> for properly handling the single and double quotes.
> the commit message also has some tests on it.
>
> it is based on your
> v11_pg_dumpall-with-directory-tar-custom-format-21-jan.patch.

I think, instead of char, if we read line by line, then we don't need
that much code and need not to worry about double quotes.
In the next version, I will merge some patches and will change it to
read line by line.

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

On Tue, 28 Jan 2025 at 10:19, Srinath Reddy <srinath2133@gmail.com> wrote:
>
>
> Hi mahendra,
>
> I have reviewed the code in the v11 patch and it looks good to me.
>
> But in common_dumpall_restore.c there's  parseDumpFormat which is common between pg_dumpall and pg_restore ,as per
thediscussion in [1] thread i don't think we should create a common api ,as discussed in the thread there might chances
inthe future we might decide that some format is obsolete and desupport it in pg_dumpall ,while support in pg_restore
forcompatibility reasons. 
>

Oaky. Thanks for review. I will make changes as per discussion in
another thread.


On Tue, 28 Jan 2025 at 11:52, Srinath Reddy <srinath2133@gmail.com> wrote:
>
> make check-world fails,i think we don't need $port and $filename instead we can use something like 'xxx'.so fixed it
inthe below patch. 

In offline discussion, Andew already reported this test case. I will
fix this in the next version.


>
> Regards,
> Srinath Reddy Sadipiralla,
> EDB: http://www.enterprisedb.com
>


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

hi.
more small issues.

+ count_db++; /* Increment db couter. */
+ dboidprecell = dboid_cell;
+ }
+
typo, "couter" should be "counter".

+
+/*
+ * 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".


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""".

or split it into two functions.


+ 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)

hi.

 <refnamediv>
  <refname>pg_restore</refname>
  <refpurpose>
   restore a <productname>PostgreSQL</productname> database from an
   archive file created by <application>pg_dump</application>
   or restore multiple <productname>PostgreSQL</productname> database from an
   archive directory created by <application>pg_dumpall</application>
  </refpurpose>
 </refnamediv>

i think it's way too verbose. we can change it to:
<refpurpose>
   restore <productname>PostgreSQL</productname> database from an
   archive file created by <application>pg_dump</application> or
<application>pg_dumpall</application>
  </refpurpose>


  <para>
   <application>pg_restore</application> is a utility for restoring a
   <productname>PostgreSQL</productname> database from an archive
   created by <xref linkend="app-pgdump"/> in one of the non-plain-text
   formats.
we can change it to
  <para>
   <application>pg_restore</application> is a utility for restoring
   <productname>PostgreSQL</productname> databases from an archive
   created by <xref linkend="app-pgdump"/> or <xref
linkend="app-pgdumpall"/> in one of the non-plain-text
   formats.


similarly, pg_dumpall first 3 sentences in the description section
needs to change.


in pg_restore.sgml <option>--create</option section,
maybe we can explicitly mention that restoring multiple databases,
<option>--create</option> is required.
like: "This option is required when restoring multiple databases."


restoreAllDatabases
+ if (!conn)
+ pg_log_info("there is no database connection so consider pattern as
simple name for --exclude-database");
filter_dbnames_for_restore
+ if (!conn)
+ pg_log_info("considering PATTERN as NAME for --exclude-database
option as no db connection while doing pg_restore.");

these two log messages sent out the same information.
maybe we can remove the first one, and change the second to
    if (!conn && db_exclude_patterns.head != NULL)
        pg_log_info("considering PATTERN as NAME for
--exclude-database option as no db connection while doing
pg_restore.");


as mentioned in the previous thread, there is no need to change PrintTOCSummary.


another minor issue about comments.
I guess we can tolerate this minor issue.
$BIN10/pg_restore --format=tar --create --file=1.sql
--exclude-database=src10 --verbose tar10 > dir_format 2>&1
1.sql file will copy tar10/global.dat as is. but we already excluded
src10. but 1.sql will still have comments as
--
-- Database "src10" dump
--


$BIN10/pg_dumpall --format=custom --file=x1.dump --globals-only
$BIN10/pg_dumpall --format=custom --file=x2.dump

Currently x1.dump/global.dat is differ from x2.dump/global.dat
if we dump multiple databases using pg_dumpall we have
"
--
-- Databases
--
--
-- Database "template1" dump
--
--
-- Database "src10" dump
--
--
-- Database "x" dump
--
"
maybe there are not need, since we already have map.dat file


I am not sure if the following is as expected or not.
$BIN10/pg_dumpall --format=custom --file=x1.dump --globals-only
$BIN10/pg_restore --create --file=3.sql --globals-only x1.dump --verbose
$BIN10/pg_restore --create --file=3.sql x1.dump --verbose

the first pg_restore command  will copy x1.dump/global.dat as is to 3.sql,
the second pg_restore will not copy anything to 3.sql.
but the second command implies copying global dumps to 3.sql?

On Tue, Feb 18, 2025 at 2:10 PM jian he <jian.universality@gmail.com> wrote:
>
> hi.

hi. more cosmetic minor issues.

+static int
+get_dbname_oid_list_from_mfile(const char *dumpdirpath,
SimpleDatabaseOidList *dbname_oid_list)
...
+ /*
+ * 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.
+ */
i think the above comment in get_dbname_oid_list_from_mfile is not necessary.
we already have comments in filter_dbnames_for_restore.

in get_dbname_oid_list_from_mfile:
```
    pfile = fopen(map_file_path, PG_BINARY_R);
    if (pfile == NULL)
        pg_fatal("could not open map.dat file: \"%s\"", map_file_path);
```
file does not exist, we use pg_fatal, so if the directory does not
exist, we should also use pg_fatal.
so
    if (!IsFileExistsInDirectory(pg_strdup(dumpdirpath), "map.dat"))
    {
        pg_log_info("databases restoring is skipped as map.dat file is
not present in \"%s\"", dumpdirpath);
        return 0;
    }
can be
    if (!IsFileExistsInDirectory(pg_strdup(dumpdirpath), "map.dat"))
        pg_fatal("map.dat file: \"%s\"/map.dat does not exists", dumpdirpath);



+ /* 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);
i think the comments should be
+ /* Report error and exit if the file has any corrupted data. */


+/*
+ * filter_dbnames_for_restore
+ *
+ * This will remove names from all dblist those can
+ * be constructed from database_exclude_pattern list.
+ *
+ * returns number of dbnames those will be restored.
+ */
+static int
+filter_dbnames_for_restore(PGconn *conn,
+   SimpleDatabaseOidList *dbname_oid_list,
there is no "database_exclude_pattern" list, so the above comments are
slightly wrong.


+/*
+ * ReadOneStatement
+ *
+ * This will start reading from passed file pointer using fgetc and read till
+ * semicolon(sql statement terminator for global.sql file)
+ *
+ * EOF is returned if end-of-file input is seen; time to shut down.
+ */
here, "global sql" should change to "gloal.dat".


  /* sync the resulting file, errors are not fatal */
- if (dosync)
+ if (dosync && (archDumpFormat == archNull))
  (void) fsync_fname(filename, false);
does this mean pg_dumpall --no-sync option only works for plain format.
if so, we need to update the pg_dumpall --no-sync section.

On Tue, 18 Feb 2025 at 10:00, Srinath Reddy <srinath2133@gmail.com> wrote:
>
> Hi,
> i think during restore we should not force user to use -C during cases like
> ./pg_restore pdd -g -f -
> ./pg_restore pdd -a -f -
> ./pg_restore pdd -s -f -
> because its not good to use -C to create database every time when we are using these options individually.
> latest patch throws following error for all the above cases

-g => we can allow this case without the -C option.
-a and -s => user should use this option with a single database (i
mean user should use a particular dump file to restore, not full dump
directory of all the databases.)

As pg_dumpall dumps all the databases in create mode, we should either
use --create option in our code or we should give an error. I think,
error is a good option if the user is using a dump of pg_dumpall.
If the user wants to use all the options, then the user should use a
single database dump path.
If we allow users without the --create option, then pg_restore will
create all the tables under a single database even if those tables are
in different databases.

I will fix the -g option(1st test case) in the next patch.

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

hi.

Currently, pg_retore says
--exit-on-error
Exit if an error is encountered while sending SQL commands to the
database. The default is to continue and to display a count of errors
at the end of the restoration.
Do we need to apply this to restore executing global commands (create
role, create tablespace)?
If not then we need to put some words in pg_restoe --exit-on-error
option saying that while restoring global objects --exit-on-error
option is ignored.



IMHO, in pg_restore.sgml, we need words explicitly saying that
when restoring multiple databases, all the specified options will
apply to each individual database.

I tested the following options for restoring multiple databases. The
results look good to me.
--index=index
--table=table
--schema-only
--transaction-size
--no-comments
some part of (--filter=filename)
--exclude-schema=schema

attach is a minor cosmetic change.

On Tue, 18 Feb 2025 at 10:00, Srinath Reddy <srinath2133@gmail.com> wrote:
>
> Hi,
> i think during restore we should not force user to use -C during cases like
> ./pg_restore pdd -g -f -
> ./pg_restore pdd -a -f -
> ./pg_restore pdd -s -f -
> because its not good to use -C to create database every time when we are using these options individually.
> latest patch throws following error for all the above cases

Fixed. (./pg_restore pdd -g -f -)

Thanks Jian and Srinath for the review and testing.

On Tue, 18 Feb 2025 at 11:41, jian he <jian.universality@gmail.com> wrote:
>
> hi.
>
>  <refnamediv>
>   <refname>pg_restore</refname>
>   <refpurpose>
>    restore a <productname>PostgreSQL</productname> database from an
>    archive file created by <application>pg_dump</application>
>    or restore multiple <productname>PostgreSQL</productname> database from an
>    archive directory created by <application>pg_dumpall</application>
>   </refpurpose>
>  </refnamediv>
>
> i think it's way too verbose. we can change it to:
> <refpurpose>
>    restore <productname>PostgreSQL</productname> database from an
>    archive file created by <application>pg_dump</application> or
> <application>pg_dumpall</application>
>   </refpurpose>

Fixed.

>
>
>   <para>
>    <application>pg_restore</application> is a utility for restoring a
>    <productname>PostgreSQL</productname> database from an archive
>    created by <xref linkend="app-pgdump"/> in one of the non-plain-text
>    formats.
> we can change it to
>   <para>
>    <application>pg_restore</application> is a utility for restoring
>    <productname>PostgreSQL</productname> databases from an archive
>    created by <xref linkend="app-pgdump"/> or <xref
> linkend="app-pgdumpall"/> in one of the non-plain-text
>    formats.

Fixed.

>
>
> similarly, pg_dumpall first 3 sentences in the description section
> needs to change.
>

I think we can keep them for pg_dumpall.

>
> in pg_restore.sgml <option>--create</option section,
> maybe we can explicitly mention that restoring multiple databases,
> <option>--create</option> is required.
> like: "This option is required when restoring multiple databases."

Fixed.

>
>
> restoreAllDatabases
> + if (!conn)
> + pg_log_info("there is no database connection so consider pattern as
> simple name for --exclude-database");
> filter_dbnames_for_restore
> + if (!conn)
> + pg_log_info("considering PATTERN as NAME for --exclude-database
> option as no db connection while doing pg_restore.");
>
> these two log messages sent out the same information.
> maybe we can remove the first one, and change the second to
>     if (!conn && db_exclude_patterns.head != NULL)
>         pg_log_info("considering PATTERN as NAME for
> --exclude-database option as no db connection while doing
> pg_restore.");

Fixed.

>
>
> as mentioned in the previous thread, there is no need to change PrintTOCSummary.

Yes, I removed it.

>
>
> another minor issue about comments.
> I guess we can tolerate this minor issue.
> $BIN10/pg_restore --format=tar --create --file=1.sql
> --exclude-database=src10 --verbose tar10 > dir_format 2>&1
> 1.sql file will copy tar10/global.dat as is. but we already excluded
> src10. but 1.sql will still have comments as
> --
> -- Database "src10" dump
> --

Fixed.

>
> $BIN10/pg_dumpall --format=custom --file=x1.dump --globals-only
> $BIN10/pg_dumpall --format=custom --file=x2.dump
>
> Currently x1.dump/global.dat is differ from x2.dump/global.dat
> if we dump multiple databases using pg_dumpall we have
> "
> --
> -- Databases
> --
> --
> -- Database "template1" dump
> --
> --
> -- Database "src10" dump
> --
> --
> -- Database "x" dump
> --
> "
> maybe there are not need, since we already have map.dat file

Okay. Fixed.

>
>
> I am not sure if the following is as expected or not.
> $BIN10/pg_dumpall --format=custom --file=x1.dump --globals-only
> $BIN10/pg_restore --create --file=3.sql --globals-only x1.dump --verbose
> $BIN10/pg_restore --create --file=3.sql x1.dump --verbose
>
> the first pg_restore command  will copy x1.dump/global.dat as is to 3.sql,
> the second pg_restore will not copy anything to 3.sql.
> but the second command implies copying global dumps to 3.sql?

We should copy global.dat. I fixed this in the v18 patch.

On Tue, 18 Feb 2025 at 14:02, jian he <jian.universality@gmail.com> wrote:
>
> On Tue, Feb 18, 2025 at 2:10 PM jian he <jian.universality@gmail.com> wrote:
> >
> > hi.
>
> hi. more cosmetic minor issues.
>
> +static int
> +get_dbname_oid_list_from_mfile(const char *dumpdirpath,
> SimpleDatabaseOidList *dbname_oid_list)
> ...
> + /*
> + * 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.
> + */
> i think the above comment in get_dbname_oid_list_from_mfile is not necessary.
> we already have comments in filter_dbnames_for_restore.

As of now, I am keeping this comment as this will be helpful while
implementing parallel pg_restore.

>
> in get_dbname_oid_list_from_mfile:
> ```
>     pfile = fopen(map_file_path, PG_BINARY_R);
>     if (pfile == NULL)
>         pg_fatal("could not open map.dat file: \"%s\"", map_file_path);
> ```
> file does not exist, we use pg_fatal, so if the directory does not
> exist, we should also use pg_fatal.
> so
>     if (!IsFileExistsInDirectory(pg_strdup(dumpdirpath), "map.dat"))
>     {
>         pg_log_info("databases restoring is skipped as map.dat file is
> not present in \"%s\"", dumpdirpath);
>         return 0;
>     }
> can be
>     if (!IsFileExistsInDirectory(pg_strdup(dumpdirpath), "map.dat"))
>         pg_fatal("map.dat file: \"%s\"/map.dat does not exists", dumpdirpath);

No, we can't add FATAL here as in case  of global-only dump, we will
not have a map.dat file.

>
>
> + /* 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);
> i think the comments should be
> + /* Report error and exit if the file has any corrupted data. */

Fixed.

>
>
> +/*
> + * filter_dbnames_for_restore
> + *
> + * This will remove names from all dblist those can
> + * be constructed from database_exclude_pattern list.
> + *
> + * returns number of dbnames those will be restored.
> + */
> +static int
> +filter_dbnames_for_restore(PGconn *conn,
> +   SimpleDatabaseOidList *dbname_oid_list,
> there is no "database_exclude_pattern" list, so the above comments are
> slightly wrong.

Fixed.

>
>
> +/*
> + * ReadOneStatement
> + *
> + * This will start reading from passed file pointer using fgetc and read till
> + * semicolon(sql statement terminator for global.sql file)
> + *
> + * EOF is returned if end-of-file input is seen; time to shut down.
> + */
> here, "global sql" should change to "gloal.dat".

Fixed.

>
>
>   /* sync the resulting file, errors are not fatal */
> - if (dosync)
> + if (dosync && (archDumpFormat == archNull))
>   (void) fsync_fname(filename, false);
> does this mean pg_dumpall --no-sync option only works for plain format.
> if so, we need to update the pg_dumpall --no-sync section.
As of now, we are using this option with plain format as we dump
server commands in different db file. We can test this more.

On Wed, 19 Feb 2025 at 17:08, jian he <jian.universality@gmail.com> wrote:
>
> hi.
>
> Currently, pg_retore says
> --exit-on-error
> Exit if an error is encountered while sending SQL commands to the
> database. The default is to continue and to display a count of errors
> at the end of the restoration.
> Do we need to apply this to restore executing global commands (create
> role, create tablespace)?
> If not then we need to put some words in pg_restoe --exit-on-error
> option saying that while restoring global objects --exit-on-error
> option is ignored.

I think this is the same for all pg_restore commands. Still if we want
to add some docs, we can put.

>
>
>
> IMHO, in pg_restore.sgml, we need words explicitly saying that
> when restoring multiple databases, all the specified options will
> apply to each individual database.

We can skip this extra info. I will try in the next version if we can
add something in doc.

>
> I tested the following options for restoring multiple databases. The
> results look good to me.
> --index=index
> --table=table
> --schema-only
> --transaction-size
> --no-comments
> some part of (--filter=filename)
> --exclude-schema=schema

Thank you for detailed testing.

>
> attach is a minor cosmetic change.
Okay.

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

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

Hello,

I think the business with an evergrowing on_exit list needs a different
solution than a gigantic array of entries.  Maybe it would make sense to
restructure that code so that there's a single on_exit item, but there
exists a list of per-database entries to clean up which are all done in
one call of the function.  Then you don't need to change the hardcoded
MAX_ON_EXIT_NICELY array size there.

I think it would be better to have a preparatory 0001 patch that just
moves the code to the new files, without touching anything else, and
then the new feature is introduced as a separate 0002 commit.

You still have a bunch of XXX items here and there which look to me like
they need to be handled before this patch can be considered final, plus
the TODOs in the commit message.  Please pgindent.

Thanks

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Porque francamente, si para saber manejarse a uno mismo hubiera que
rendir examen... ¿Quién es el machito que tendría carnet?"  (Mafalda)

Thanks Álvaro for feedback.

On Thu, 20 Feb 2025 at 02:39, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Hello,
>
> I think the business with an evergrowing on_exit list needs a different
> solution than a gigantic array of entries.  Maybe it would make sense to
> restructure that code so that there's a single on_exit item, but there
> exists a list of per-database entries to clean up which are all done in
> one call of the function.  Then you don't need to change the hardcoded
> MAX_ON_EXIT_NICELY array size there.
>

In the latest patch, I added one new function to clean
index(on_exit_nicely_index) with each database restore.

> I think it would be better to have a preparatory 0001 patch that just
> moves the code to the new files, without touching anything else, and
> then the new feature is introduced as a separate 0002 commit.

Fixed.

>
> You still have a bunch of XXX items here and there which look to me like
> they need to be handled before this patch can be considered final, plus

Fixed.

> the TODOs in the commit message.  Please pgindent.
I am facing some errors in pgindent. I will run pgindent in the next version.

Here, I am attaching updated patches for review and testing.

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

hi.
about 0001

/*
 * connectDatabase
 *
 * Make a database connection with the given parameters.  An
 * interactive password prompt is automatically issued if required.
 *
 * If fail_on_error is false, we return NULL without printing any message
 * on failure, but preserve any prompted password for the next try.
 *
 * On success, the global variable 'connstr' is set to a connection string
 * containing the options used.
 */
PGconn *
connectDatabase(const char *dbname, const char *connection_string,
                const char *pghost, const char *pgport, const char *pguser,
                trivalue prompt_password, bool fail_on_error, const
char *progname,
                const char **connstr, int *server_version)
do the comments need to change? since no
global variable 'connstr' in common_dumpall_restore.c
maybe we need some words to explain server_version, (i don't have a
huge opinion though).


/*-------------------------------------------------------------------------
 *
 * common_dumpall_restore.c
 *
 * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group
 * Portions Copyright (c) 1994, Regents of the University of California
 *
 * This is a common file for pg_dumpall and pg_restore.
 * src/bin/pg_dump/common_dumpall_restore.c
 *
 *-------------------------------------------------------------------------
 */

may change to

/*-------------------------------------------------------------------------
 *
 * common_dumpall_restore.c
 *     This is a common file for pg_dumpall and pg_restore.
 *
 * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group
 * Portions Copyright (c) 1994, Regents of the University of California
 *
 * IDENTIFICATION
 *    src/bin/pg_dump/common_dumpall_restore.c
 *
 *-------------------------------------------------------------------------
 */
so the style aligns with most other files.
(we can apply the same logic to src/bin/pg_dump/common_dumpall_restore.h)


in src/bin/pg_dump/pg_dumpall.c
#include "common_dumpall_restore.h"
imply include "pg_backup.h".
so in src/bin/pg_dump/pg_dumpall.c, we don't need include "pg_backup.h"


attached are minor cosmetic changes for v19.

hi.

v20-0001
in src/bin/pg_dump/pg_dumpall.c, we have:

static const char *connstr = "";
            case 'd':
                connstr = pg_strdup(optarg);
                break;

i am not sure you can declare it as "const" for connstr.
since connstr value can be changed.
``#include "pg_backup.h"`` can be removed from src/bin/pg_dump/pg_dumpall.c
Other than that, v20_0001 looks good to me.


v20_0002
const char *formatName = "p";
formatName should not be declared as "const", since its value can be changed.


+ /* Create a subdirectory with 'databases' name under main directory. */
+ if (mkdir(db_subdir, 0755) != 0)
+ pg_fatal("could not create subdirectory \"%s\": %m", db_subdir);
can change to
        if (mkdir(db_subdir, pg_dir_create_mode) != 0)
            pg_fatal("could not create subdirectory \"%s\": %m", db_subdir);
then in src/bin/pg_dump/pg_dumpall.c need add ``#include "common/file_perm.h"``

similarly
+ else if (mkdir(dirname, 0700) < 0)
+ pg_fatal("could not create directory \"%s\": %m", dirname);
can change to
``
else if (mkdir(dirname, pg_dir_create_mode) != 0)
    pg_fatal("could not create directory \"%s\": %m", dirname);
``


+
+ if (!conn)
+ pg_log_info("considering PATTERN as NAME for --exclude-database
option as no db connection while doing pg_restore.");
"db connection" maybe "database connection" or "connection"


+ /*
+ * We need to reset on_exit_nicely_index with each database so that
we can restore
+ * multiple databases by archive.  See EXIT_NICELY macro for more details.
+ */
+ if (dboid_cell != NULL)
+ reset_exit_nicely_list(n_errors ? 1 : 0);
i don't fully understand this part, anyway, i think EXIT_NICELY, you mean
MAX_ON_EXIT_NICELY?


just found out, parseArchiveFormat looks familiar with parseDumpFormat.


for all the options in pg_restore.
--list option is not applicable to multiple databases, therefore
option --use-list=list-file also not applicable,
in the doc we should mention it.


global.dat comments should not mention "cluster", "global objects"
would be more appropriate.
global.dat comments should not mention "--\n-- Database \"%s\" dump\n--\n\n"
the attached minor patch fixes this issue.

hi.
some documentation issue:

doc/src/sgml/ref/pg_dumpall.sgml
       <variablelist>
        <varlistentry>
         <term><literal>d</literal></term>
         <term><literal>directory</literal></term>
         <listitem>
          <para>
           Output a directory-format archive suitable for input into
pg_restore. Under dboid
           subdirectory, this will create a directory with one file
for each table and large
           object being dumped, plus a so-called Table of Contents
file describing the dumped
           objects in a machine-readable format that pg_restore can
read. A directory format
           archive can be manipulated with standard Unix tools; for
example, files in an
           uncompressed archive can be compressed with the gzip, lz4,
or zstd tools. This
           format is compressed by default using gzip and also
supports parallel dumps.
          </para>
         </listitem>
        </varlistentry>
with the v20 implementation,
"""
For example, files in an
           uncompressed archive can be compressed with the gzip, lz4,
or zstd tools. This
           format is compressed by default using gzip and also
supports parallel dumps.
""
Is this part is wrong?
I think, currently, by default the pg_dumpall directory will use gzip
compress level=-1 to do the compression.
and pg_dumpall format==directory does not support parallel dumps.


-------------------
by default, this is plain format. If non-plain mode is passed, then global.dat
(global sql commands) and map.dat(dboid and dbnames list of all the databases)
files will be created. Apart from these files, one subdirectory with databases
name will be created. Under this databases subdirectory, there will be files
with dboid name for each database and if --format is directory, then toc.dat and
other dump files will be under dboid subdirectory.
-------------------
I think the above message changes to the below, the message is more clear, IMHO.

By default, this uses plain format. If a non-plain mode is specified, two files
will be created: **global.dat** (containing SQL commands for global objects) and
**map.dat** (listing database OIDs and names for all databases). Additionally, a
subdirectory named after each database OID will be created.
If the --format option is set to **directory**, then **toc.dat** and
other dump files
will be stored within the corresponding database Oid subdirectory.


---------------------
doc/src/sgml/ref/pg_restore.sgml
<term><option>--exclude-database=<replaceable
class="parameter">pattern</replaceable></option></term>
we can add:

        When emitting a script, this option is not supported for
wild-card matching,
        the excluded database must exactly match the literal
<replaceable class="parameter">pattern</replaceable> string.

A database name containing a newline breaks things for this patch:

CREATE DATABASE "foo
bar";


$ pg_dumpall -Fc --file test
shell command argument contains a newline or carriage return: " dbname='foo
bar'"

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

Disclaimer: I didn't review these patches fully.

On 2025-Mar-05, Mahendra Singh Thalor wrote:

> On Wed, 5 Mar 2025 at 01:02, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> > A database name containing a newline breaks things for this patch:
> >
> > CREATE DATABASE "foo
> > bar";

> I also reported this issue on 29-01-2025. This breaks even without this
> patch also.

Okay, we should probably fix that, but I think the new map.dat file your
patch adds is going to make the problem worse, because it doesn't look
like you handled that case in any particular way that would make it not
fail.  I think it would be good to avoid digging us up even deeper in
that hole.  More generally, the pg_upgrade tests contain some code to
try database names with almost all possible ascii characters (see
generate_db in pg_upgrade/t/002_pg_upgrade.pl); it would be good to
ensure that this new functionality also works correctly for that --
perhaps add an equivalent test to the pg_dumpall test suite.

Looking at 0001:

I'm not sure that the whole common_dumpall_restore.c thing is properly
structured.  First, the file name shouldn't presume which programs
exactly are going to use the funcionality there.  Second, it looks like
there's another PQconnectdbParams() in pg_backup_db.c and I don't
understand what the reason is for that one to be separate.  In my mind,
there should be a file maybe called connection.c or connectdb.c or
whatever that's in charge of establishing connection for all the
src/bin/pg_dump programs, for cleanliness sake.  (This is probably also
the place where to put an on_exit callback that cleans up any leftover
connections.)

Looking at 0002 I see it mentions looking at the EXIT_NICELY macro for
documentation.  No such macro exists.  But also I think the addition
(and use) of reset_exit_nicely_list() is not a good idea.  It seems to
assume that the only entries in that list are ones that can be cleared
and reinstated whenever.  This makes too much of an assumption about how
the program works.  It may work today, but it'll get in the way of any
other patch that wants to set up some different on-exit clean up.  In
other words, we shouldn't reset the on_exit list at all.  Also, this is
just a weird addition:

#define exit_nicely(code) exit(code)

You added "A" as an option to the getopt_long() call in pg_restore, but
no handling for it is added.

I think the --globals-only option to pg_restore should be a separate
commit.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

Thanks Alvaro for feedback and review.

On Wed, 5 Mar 2025 at 20:42, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Disclaimer: I didn't review these patches fully.
>
> On 2025-Mar-05, Mahendra Singh Thalor wrote:
>
> > On Wed, 5 Mar 2025 at 01:02, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > > A database name containing a newline breaks things for this patch:
> > >
> > > CREATE DATABASE "foo
> > > bar";
>
> > I also reported this issue on 29-01-2025. This breaks even without this
> > patch also.
>
> Okay, we should probably fix that, but I think the new map.dat file your
> patch adds is going to make the problem worse, because it doesn't look
> like you handled that case in any particular way that would make it not
> fail.  I think it would be good to avoid digging us up even deeper in
> that hole.  More generally, the pg_upgrade tests contain some code to
> try database names with almost all possible ascii characters (see
> generate_db in pg_upgrade/t/002_pg_upgrade.pl); it would be good to
> ensure that this new functionality also works correctly for that --
> perhaps add an equivalent test to the pg_dumpall test suite.

In the attached patch, I tried to solve the problem of the map.dat
file. I will do more analysis based on dbnames in 002_pg_upgrade.pl
file.

>
> Looking at 0001:
>
> I'm not sure that the whole common_dumpall_restore.c thing is properly
> structured.  First, the file name shouldn't presume which programs
> exactly are going to use the funcionality there.  Second, it looks like
> there's another PQconnectdbParams() in pg_backup_db.c and I don't
> understand what the reason is for that one to be separate.  In my mind,
> there should be a file maybe called connection.c or connectdb.c or
> whatever that's in charge of establishing connection for all the
> src/bin/pg_dump programs, for cleanliness sake.  (This is probably also
> the place where to put an on_exit callback that cleans up any leftover
> connections.)

Okay. I will do these changes.

>
> Looking at 0002 I see it mentions looking at the EXIT_NICELY macro for
> documentation.  No such macro exists.  But also I think the addition
> (and use) of reset_exit_nicely_list() is not a good idea.  It seems to
> assume that the only entries in that list are ones that can be cleared
> and reinstated whenever.  This makes too much of an assumption about how
> the program works.  It may work today, but it'll get in the way of any
> other patch that wants to set up some different on-exit clean up.  In
> other words, we shouldn't reset the on_exit list at all.  Also, this is
> just a weird addition:

I will do more study for this case and will update here.

>
> #define exit_nicely(code) exit(code)

Okay. I will fix this.

>
> You added "A" as an option to the getopt_long() call in pg_restore, but
> no handling for it is added.

Fixed.

>
> I think the --globals-only option to pg_restore should be a separate
> commit.

Okay.

>
> --
> Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

Here, I am attaching updated patches for review and testing.

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

On Thu, Mar 6, 2025 at 12:49 AM Mahendra Singh Thalor
<mahi6run@gmail.com> wrote:
>
> Thanks Alvaro for feedback and review.
>
> On Wed, 5 Mar 2025 at 20:42, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > Disclaimer: I didn't review these patches fully.
> >
> > On 2025-Mar-05, Mahendra Singh Thalor wrote:
> >
> > > On Wed, 5 Mar 2025 at 01:02, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > >
> > > > A database name containing a newline breaks things for this patch:
> > > >
> > > > CREATE DATABASE "foo
> > > > bar";
> >
> > > I also reported this issue on 29-01-2025. This breaks even without this
> > > patch also.
> >
> > Okay, we should probably fix that, but I think the new map.dat file your
> > patch adds is going to make the problem worse, because it doesn't look
> > like you handled that case in any particular way that would make it not
> > fail.  I think it would be good to avoid digging us up even deeper in
> > that hole.  More generally, the pg_upgrade tests contain some code to
> > try database names with almost all possible ascii characters (see
> > generate_db in pg_upgrade/t/002_pg_upgrade.pl); it would be good to
> > ensure that this new functionality also works correctly for that --
> > perhaps add an equivalent test to the pg_dumpall test suite.
>
> In the attached patch, I tried to solve the problem of the map.dat
> file. I will do more analysis based on dbnames in 002_pg_upgrade.pl
> file.
>

hi.

/*
 * Append the given string to the shell command being built in the buffer,
 * with shell-style quoting as needed to create exactly one argument.
 *
 * Forbid LF or CR characters, which have scant practical use beyond designing
 * security breaches.  The Windows command shell is unusable as a conduit for
 * arguments containing LF or CR characters.  A future major release should
 * reject those characters in CREATE ROLE and CREATE DATABASE, because use
 * there eventually leads to errors here.
 *
 * appendShellString() simply prints an error and dies if LF or CR appears.
 * appendShellStringNoError() omits those characters from the result, and
 * returns false if there were any.
 */
void
appendShellString(PQExpBuffer buf, const char *str)

per above comments,
we need to disallow LF/CR in database name and role name when issuing
shell command.

rolename LF/CR issue already being handled in
src/bin/pg_dump/pg_dumpall.c: while(getopt_long) code:
            case 3:
                use_role = pg_strdup(optarg);
                appendPQExpBufferStr(pgdumpopts, " --role ");
                appendShellString(pgdumpopts, use_role);

we can fail earlier also for database names in dumpDatabases, right
after executeQuery.
Please check attached, which is based on *v20*.


in V21, src/bin/pg_dump/pg_dumpall.c:
+#include "common_dumpall_restore.h"
happened within v21-0001 and v21-0002, it is being included twice.

Thanks Alvaro and Jian for the review and feedback.

On Wed, 5 Mar 2025 at 20:42, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Disclaimer: I didn't review these patches fully.
>
> On 2025-Mar-05, Mahendra Singh Thalor wrote:
>
> > On Wed, 5 Mar 2025 at 01:02, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > > A database name containing a newline breaks things for this patch:
> > >
> > > CREATE DATABASE "foo
> > > bar";
>
> > I also reported this issue on 29-01-2025. This breaks even without this
> > patch also.
>
> Okay, we should probably fix that, but I think the new map.dat file your
> patch adds is going to make the problem worse, because it doesn't look
> like you handled that case in any particular way that would make it not
> fail.  I think it would be good to avoid digging us up even deeper in
> that hole.  More generally, the pg_upgrade tests contain some code to
> try database names with almost all possible ascii characters (see
> generate_db in pg_upgrade/t/002_pg_upgrade.pl); it would be good to
> ensure that this new functionality also works correctly for that --
> perhaps add an equivalent test to the pg_dumpall test suite.

As Jian also pointed out, we should not allow \n\r in dbnames. I am
keeping dbanames as single line names only.

I am doing testing using the pg_upgrade/t/002_pg_upgrade.pl file to
check different-2 dbnames.

>
> Looking at 0001:
>
> I'm not sure that the whole common_dumpall_restore.c thing is properly
> structured.  First, the file name shouldn't presume which programs
> exactly are going to use the funcionality there.  Second, it looks like
> there's another PQconnectdbParams() in pg_backup_db.c and I don't
> understand what the reason is for that one to be separate.  In my mind,
> there should be a file maybe called connection.c or connectdb.c or
> whatever that's in charge of establishing connection for all the
> src/bin/pg_dump programs, for cleanliness sake.  (This is probably also
> the place where to put an on_exit callback that cleans up any leftover
> connections.)

I did some more refactoring and made a connectdb.c file.

> Looking at 0002 I see it mentions looking at the EXIT_NICELY macro for
> documentation.  No such macro exists.  But also I think the addition
> (and use) of reset_exit_nicely_list() is not a good idea.  It seems to
> assume that the only entries in that list are ones that can be cleared
> and reinstated whenever.  This makes too much of an assumption about how
> the program works.  It may work today, but it'll get in the way of any
> other patch that wants to set up some different on-exit clean up.  In
> other words, we shouldn't reset the on_exit list at all.  Also, this is
> just a weird addition:

Based on some discussions, I added handling for cleanup. for 1st
database, I am saving index of array and then I am using same index
for rest of the databases as we are closing archive file in
CloseArchive so we can use same index for next database.

>
> #define exit_nicely(code) exit(code)

Fixed.

>
> You added "A" as an option to the getopt_long() call in pg_restore, but
> no handling for it is added.
Fixed.

>
> I think the --globals-only option to pg_restore should be a separate
> commit.
I will make this in the next version.

Here, I am attaching updated patches for review and testing.


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

On 2025-Mar-11, Mahendra Singh Thalor wrote:

> On Wed, 5 Mar 2025 at 20:42, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> > Okay, we should probably fix that, but I think the new map.dat file your
> > patch adds is going to make the problem worse, because it doesn't look
> > like you handled that case in any particular way that would make it not
> > fail.
> 
> As Jian also pointed out, we should not allow \n\r in dbnames. I am
> keeping dbanames as single line names only.

Ehm, did you get consensus on adding such a restriction?

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

Hello,

On 2025-Mar-11, Mahendra Singh Thalor wrote:

> In map.dat file, I tried to fix this issue by adding number of characters
> in dbname but as per code comments, as of now, we are not supporting \n\r
> in dbnames so i removed handling.
> I will do some more study to fix this issue.

Yeah, I think this is saying that you should not consider the contents
of map.dat as a shell string.  After all, you're not going to _execute_
that file via the shell.

Maybe for map.dat you need to escape such characters somehow, so that
they don't appear as literal newlines/carriage returns.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

Álvaro Herrera <alvherre@alvh.no-ip.org> writes:

> Hello,
>
> On 2025-Mar-11, Mahendra Singh Thalor wrote:
>
>> In map.dat file, I tried to fix this issue by adding number of characters
>> in dbname but as per code comments, as of now, we are not supporting \n\r
>> in dbnames so i removed handling.
>> I will do some more study to fix this issue.
>
> Yeah, I think this is saying that you should not consider the contents
> of map.dat as a shell string.  After all, you're not going to _execute_
> that file via the shell.
>
> Maybe for map.dat you need to escape such characters somehow, so that
> they don't appear as literal newlines/carriage returns.

I haven't looked at the code for this, but why are we inventing an
ad-hoc file format?  Why not use JSON, like we do for backup manifests?
Then storing arbitrary database names won't be a problem.

- ilmari

On 2025-03-11 Tu 1:52 PM, Dagfinn Ilmari Mannsåker wrote:
> Álvaro Herrera <alvherre@alvh.no-ip.org> writes:
>
>> Hello,
>>
>> On 2025-Mar-11, Mahendra Singh Thalor wrote:
>>
>>> In map.dat file, I tried to fix this issue by adding number of characters
>>> in dbname but as per code comments, as of now, we are not supporting \n\r
>>> in dbnames so i removed handling.
>>> I will do some more study to fix this issue.
>> Yeah, I think this is saying that you should not consider the contents
>> of map.dat as a shell string.  After all, you're not going to _execute_
>> that file via the shell.
>>
>> Maybe for map.dat you need to escape such characters somehow, so that
>> they don't appear as literal newlines/carriage returns.
> I haven't looked at the code for this, but why are we inventing an
> ad-hoc file format?  Why not use JSON, like we do for backup manifests?
> Then storing arbitrary database names won't be a problem.
>

I'm not sure everyone thinks that was a good idea for backup manifests 
(in fact I know some don't), and it seems somewhat like overkill for a 
simple map of oids to database names.


cheers


andrew



>
>
--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 2025-Mar-11, Andrew Dunstan wrote:

> I'm not sure everyone thinks that was a good idea for backup manifests (in
> fact I know some don't), and it seems somewhat like overkill for a simple
> map of oids to database names.

If such a simple system can be made to work for all possible valid
database names, then I agree with you.  But if it forces us to restrict
database names to not contain newlines or other funny chars that are so
far unrestricted, then I would take the other position.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"La victoria es para quien se atreve a estar solo"

On 2025-03-11 Tu 5:03 PM, Álvaro Herrera wrote:
> On 2025-Mar-11, Andrew Dunstan wrote:
>
>> I'm not sure everyone thinks that was a good idea for backup manifests (in
>> fact I know some don't), and it seems somewhat like overkill for a simple
>> map of oids to database names.
> If such a simple system can be made to work for all possible valid
> database names, then I agree with you.  But if it forces us to restrict
> database names to not contain newlines or other funny chars that are so
> far unrestricted, then I would take the other position.
>

Well, JSON is supposed to be UTF8. What should we do about database 
names that are not UTF8?

It's kinda tempting to say we should have the file consist of lines like:

     oid base64_encoded_name escaped_human_readable name


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Wed, Mar 12, 2025 at 1:06 AM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Hello,
>
> On 2025-Mar-11, Mahendra Singh Thalor wrote:
>
> > In map.dat file, I tried to fix this issue by adding number of characters
> > in dbname but as per code comments, as of now, we are not supporting \n\r
> > in dbnames so i removed handling.
> > I will do some more study to fix this issue.
>
> Yeah, I think this is saying that you should not consider the contents
> of map.dat as a shell string.  After all, you're not going to _execute_
> that file via the shell.
>
> Maybe for map.dat you need to escape such characters somehow, so that
> they don't appear as literal newlines/carriage returns.
>

I am confused.
currently pg_dumpall plain format will abort when encountering dbname
containing newline.
the left dumped plain file does not contain all the cluster databases data.


if pg_dumpall non-text format aborts earlier,
it's aligned with pg_dumpall plain format?
it's also an improvement since aborts earlier, nothing will be dumped?


am i missing something?

On Tue, 2025-03-11 at 19:14 -0400, Isaac Morland wrote:
> On Tue, 11 Mar 2025 at 18:37, Andrew Dunstan <andrew@dunslane.net> wrote:
>
> > Well, JSON is supposed to be UTF8. What should we do about database
> > names that are not UTF8?
>
> How can you have a database name that isn't encodeable in UTF-8? At this point
> I'm pretty sure Unicode has subsumed essentially every character ever mentioned
> in a standards document.

There is a difference between "encodable" and "encoded".  You'd have to figure
out the actual encoding of the database name and convert that to UTF-8.

Yours,
Laurenz Albe

On 2025-03-12 We 3:03 AM, jian he wrote:
> On Wed, Mar 12, 2025 at 1:06 AM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>> Hello,
>>
>> On 2025-Mar-11, Mahendra Singh Thalor wrote:
>>
>>> In map.dat file, I tried to fix this issue by adding number of characters
>>> in dbname but as per code comments, as of now, we are not supporting \n\r
>>> in dbnames so i removed handling.
>>> I will do some more study to fix this issue.
>> Yeah, I think this is saying that you should not consider the contents
>> of map.dat as a shell string.  After all, you're not going to _execute_
>> that file via the shell.
>>
>> Maybe for map.dat you need to escape such characters somehow, so that
>> they don't appear as literal newlines/carriage returns.
>>
> I am confused.
> currently pg_dumpall plain format will abort when encountering dbname
> containing newline.
> the left dumped plain file does not contain all the cluster databases data.
>
>
> if pg_dumpall non-text format aborts earlier,
> it's aligned with pg_dumpall plain format?
> it's also an improvement since aborts earlier, nothing will be dumped?
>
>
> am i missing something?
>
>

I think we should fix that.

But for the current proposal, Álvaro and I were talking this morning, 
and we thought the simplest thing here would be to have the one line 
format and escape NL/CRs in the database name.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Wed, 12 Mar 2025 at 21:18, Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 2025-03-12 We 3:03 AM, jian he wrote:
> > On Wed, Mar 12, 2025 at 1:06 AM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >> Hello,
> >>
> >> On 2025-Mar-11, Mahendra Singh Thalor wrote:
> >>
> >>> In map.dat file, I tried to fix this issue by adding number of characters
> >>> in dbname but as per code comments, as of now, we are not supporting \n\r
> >>> in dbnames so i removed handling.
> >>> I will do some more study to fix this issue.
> >> Yeah, I think this is saying that you should not consider the contents
> >> of map.dat as a shell string.  After all, you're not going to _execute_
> >> that file via the shell.
> >>
> >> Maybe for map.dat you need to escape such characters somehow, so that
> >> they don't appear as literal newlines/carriage returns.
> >>
> > I am confused.
> > currently pg_dumpall plain format will abort when encountering dbname
> > containing newline.
> > the left dumped plain file does not contain all the cluster databases data.
> >
> >
> > if pg_dumpall non-text format aborts earlier,
> > it's aligned with pg_dumpall plain format?
> > it's also an improvement since aborts earlier, nothing will be dumped?
> >
> >
> > am i missing something?
> >
> >
>
> I think we should fix that.
>
> But for the current proposal, Álvaro and I were talking this morning,
> and we thought the simplest thing here would be to have the one line
> format and escape NL/CRs in the database name.
>
>
> cheers
>
Okay. As per discussions, we will keep one line entry for each
database into map.file.

Thanks all for feedback and review.

Here, I am attaching updated patches for review and testing. These
patches can be applied on commit a6524105d20b.

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

On 2025-03-19 We 2:41 AM, Mahendra Singh Thalor wrote:
> On Wed, 12 Mar 2025 at 21:18, Andrew Dunstan <andrew@dunslane.net> wrote:
>>
>> On 2025-03-12 We 3:03 AM, jian he wrote:
>>> On Wed, Mar 12, 2025 at 1:06 AM Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>>>> Hello,
>>>>
>>>> On 2025-Mar-11, Mahendra Singh Thalor wrote:
>>>>
>>>>> In map.dat file, I tried to fix this issue by adding number of characters
>>>>> in dbname but as per code comments, as of now, we are not supporting \n\r
>>>>> in dbnames so i removed handling.
>>>>> I will do some more study to fix this issue.
>>>> Yeah, I think this is saying that you should not consider the contents
>>>> of map.dat as a shell string.  After all, you're not going to _execute_
>>>> that file via the shell.
>>>>
>>>> Maybe for map.dat you need to escape such characters somehow, so that
>>>> they don't appear as literal newlines/carriage returns.
>>>>
>>> I am confused.
>>> currently pg_dumpall plain format will abort when encountering dbname
>>> containing newline.
>>> the left dumped plain file does not contain all the cluster databases data.
>>>
>>>
>>> if pg_dumpall non-text format aborts earlier,
>>> it's aligned with pg_dumpall plain format?
>>> it's also an improvement since aborts earlier, nothing will be dumped?
>>>
>>>
>>> am i missing something?
>>>
>>>
>> I think we should fix that.
>>
>> But for the current proposal, Álvaro and I were talking this morning,
>> and we thought the simplest thing here would be to have the one line
>> format and escape NL/CRs in the database name.
>>
>>
>> cheers
>>
> Okay. As per discussions, we will keep one line entry for each
> database into map.file.
>
> Thanks all for feedback and review.
>
> Here, I am attaching updated patches for review and testing. These
> patches can be applied on commit a6524105d20b.



I'm working through this patch set with a view to committing it. 
Attached is some cleanup which is where I got to today, although there 
is more to do. One thing I am wondering is why not put the 
SimpleDatabaseOidList stuff in fe_utils/simle_list.{c,h} ? That's where 
all the similar stuff belongs, and it feels strange to have this inline 
in pg_restore.c. (I also don't like the name much - SimpleOidStringList 
or maybe SimpleOidPlusStringList might be better).


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 2025-03-27 Th 5:15 PM, Andrew Dunstan wrote:
>
> On 2025-03-19 We 2:41 AM, Mahendra Singh Thalor wrote:
>> On Wed, 12 Mar 2025 at 21:18, Andrew Dunstan <andrew@dunslane.net> 
>> wrote:
>>>
>>> On 2025-03-12 We 3:03 AM, jian he wrote:
>>>> On Wed, Mar 12, 2025 at 1:06 AM Álvaro Herrera 
>>>> <alvherre@alvh.no-ip.org> wrote:
>>>>> Hello,
>>>>>
>>>>> On 2025-Mar-11, Mahendra Singh Thalor wrote:
>>>>>
>>>>>> In map.dat file, I tried to fix this issue by adding number of 
>>>>>> characters
>>>>>> in dbname but as per code comments, as of now, we are not 
>>>>>> supporting \n\r
>>>>>> in dbnames so i removed handling.
>>>>>> I will do some more study to fix this issue.
>>>>> Yeah, I think this is saying that you should not consider the 
>>>>> contents
>>>>> of map.dat as a shell string.  After all, you're not going to 
>>>>> _execute_
>>>>> that file via the shell.
>>>>>
>>>>> Maybe for map.dat you need to escape such characters somehow, so that
>>>>> they don't appear as literal newlines/carriage returns.
>>>>>
>>>> I am confused.
>>>> currently pg_dumpall plain format will abort when encountering dbname
>>>> containing newline.
>>>> the left dumped plain file does not contain all the cluster 
>>>> databases data.
>>>>
>>>>
>>>> if pg_dumpall non-text format aborts earlier,
>>>> it's aligned with pg_dumpall plain format?
>>>> it's also an improvement since aborts earlier, nothing will be dumped?
>>>>
>>>>
>>>> am i missing something?
>>>>
>>>>
>>> I think we should fix that.
>>>
>>> But for the current proposal, Álvaro and I were talking this morning,
>>> and we thought the simplest thing here would be to have the one line
>>> format and escape NL/CRs in the database name.
>>>
>>>
>>> cheers
>>>
>> Okay. As per discussions, we will keep one line entry for each
>> database into map.file.
>>
>> Thanks all for feedback and review.
>>
>> Here, I am attaching updated patches for review and testing. These
>> patches can be applied on commit a6524105d20b.
>
>
>
> I'm working through this patch set with a view to committing it. 
> Attached is some cleanup which is where I got to today, although there 
> is more to do. One thing I am wondering is why not put the 
> SimpleDatabaseOidList stuff in fe_utils/simle_list.{c,h} ? That's 
> where all the similar stuff belongs, and it feels strange to have this 
> inline in pg_restore.c. (I also don't like the name much - 
> SimpleOidStringList or maybe SimpleOidPlusStringList might be better).
>
>
>


OK, I have done that, so here is the result. The first two are you 
original patches. patch 3 adds the new list type to fe-utils, and patch 
4 contains my cleanups and use of the new list type. Apart from some 
relatively minor cleanup, the one thing I would like to change is how 
dumps are named. If we are producing tar or custom format dumps, I think 
the file names should reflect that (oid.dmp and oid.tar rather than a 
bare oid as the filename), and pg_restore should look for those. I'm 
going to work on that tomorrow - I don't think it will be terribly 
difficult.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Sat, 29 Mar 2025 at 03:50, Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 2025-03-27 Th 5:15 PM, Andrew Dunstan wrote:
> >
> > On 2025-03-19 We 2:41 AM, Mahendra Singh Thalor wrote:
> >> On Wed, 12 Mar 2025 at 21:18, Andrew Dunstan <andrew@dunslane.net>
> >> wrote:
> >>>
> >>> On 2025-03-12 We 3:03 AM, jian he wrote:
> >>>> On Wed, Mar 12, 2025 at 1:06 AM Álvaro Herrera
> >>>> <alvherre@alvh.no-ip.org> wrote:
> >>>>> Hello,
> >>>>>
> >>>>> On 2025-Mar-11, Mahendra Singh Thalor wrote:
> >>>>>
> >>>>>> In map.dat file, I tried to fix this issue by adding number of
> >>>>>> characters
> >>>>>> in dbname but as per code comments, as of now, we are not
> >>>>>> supporting \n\r
> >>>>>> in dbnames so i removed handling.
> >>>>>> I will do some more study to fix this issue.
> >>>>> Yeah, I think this is saying that you should not consider the
> >>>>> contents
> >>>>> of map.dat as a shell string.  After all, you're not going to
> >>>>> _execute_
> >>>>> that file via the shell.
> >>>>>
> >>>>> Maybe for map.dat you need to escape such characters somehow, so that
> >>>>> they don't appear as literal newlines/carriage returns.
> >>>>>
> >>>> I am confused.
> >>>> currently pg_dumpall plain format will abort when encountering dbname
> >>>> containing newline.
> >>>> the left dumped plain file does not contain all the cluster
> >>>> databases data.
> >>>>
> >>>>
> >>>> if pg_dumpall non-text format aborts earlier,
> >>>> it's aligned with pg_dumpall plain format?
> >>>> it's also an improvement since aborts earlier, nothing will be dumped?
> >>>>
> >>>>
> >>>> am i missing something?
> >>>>
> >>>>
> >>> I think we should fix that.
> >>>
> >>> But for the current proposal, Álvaro and I were talking this morning,
> >>> and we thought the simplest thing here would be to have the one line
> >>> format and escape NL/CRs in the database name.
> >>>
> >>>
> >>> cheers
> >>>
> >> Okay. As per discussions, we will keep one line entry for each
> >> database into map.file.
> >>
> >> Thanks all for feedback and review.
> >>
> >> Here, I am attaching updated patches for review and testing. These
> >> patches can be applied on commit a6524105d20b.
> >
> >
> >
> > I'm working through this patch set with a view to committing it.
> > Attached is some cleanup which is where I got to today, although there
> > is more to do. One thing I am wondering is why not put the
> > SimpleDatabaseOidList stuff in fe_utils/simle_list.{c,h} ? That's
> > where all the similar stuff belongs, and it feels strange to have this
> > inline in pg_restore.c. (I also don't like the name much -
> > SimpleOidStringList or maybe SimpleOidPlusStringList might be better).
> >
> >
> >
>
>
> OK, I have done that, so here is the result. The first two are you
> original patches. patch 3 adds the new list type to fe-utils, and patch
> 4 contains my cleanups and use of the new list type. Apart from some
> relatively minor cleanup, the one thing I would like to change is how
> dumps are named. If we are producing tar or custom format dumps, I think
> the file names should reflect that (oid.dmp and oid.tar rather than a
> bare oid as the filename), and pg_restore should look for those. I'm
> going to work on that tomorrow - I don't think it will be terribly
> difficult.
>

Thanks Andrew.

Here, I am attaching a delta patch for oid.tar and oid.dmp format.

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

On 2025-03-29 Sa 1:17 AM, Mahendra Singh Thalor wrote:
> On Sat, 29 Mar 2025 at 03:50, Andrew Dunstan <andrew@dunslane.net> wrote:
>>
>> On 2025-03-27 Th 5:15 PM, Andrew Dunstan wrote:
>>> On 2025-03-19 We 2:41 AM, Mahendra Singh Thalor wrote:
>>>> On Wed, 12 Mar 2025 at 21:18, Andrew Dunstan <andrew@dunslane.net>
>>>> wrote:
>>>>> On 2025-03-12 We 3:03 AM, jian he wrote:
>>>>>> On Wed, Mar 12, 2025 at 1:06 AM Álvaro Herrera
>>>>>> <alvherre@alvh.no-ip.org> wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> On 2025-Mar-11, Mahendra Singh Thalor wrote:
>>>>>>>
>>>>>>>> In map.dat file, I tried to fix this issue by adding number of
>>>>>>>> characters
>>>>>>>> in dbname but as per code comments, as of now, we are not
>>>>>>>> supporting \n\r
>>>>>>>> in dbnames so i removed handling.
>>>>>>>> I will do some more study to fix this issue.
>>>>>>> Yeah, I think this is saying that you should not consider the
>>>>>>> contents
>>>>>>> of map.dat as a shell string.  After all, you're not going to
>>>>>>> _execute_
>>>>>>> that file via the shell.
>>>>>>>
>>>>>>> Maybe for map.dat you need to escape such characters somehow, so that
>>>>>>> they don't appear as literal newlines/carriage returns.
>>>>>>>
>>>>>> I am confused.
>>>>>> currently pg_dumpall plain format will abort when encountering dbname
>>>>>> containing newline.
>>>>>> the left dumped plain file does not contain all the cluster
>>>>>> databases data.
>>>>>>
>>>>>>
>>>>>> if pg_dumpall non-text format aborts earlier,
>>>>>> it's aligned with pg_dumpall plain format?
>>>>>> it's also an improvement since aborts earlier, nothing will be dumped?
>>>>>>
>>>>>>
>>>>>> am i missing something?
>>>>>>
>>>>>>
>>>>> I think we should fix that.
>>>>>
>>>>> But for the current proposal, Álvaro and I were talking this morning,
>>>>> and we thought the simplest thing here would be to have the one line
>>>>> format and escape NL/CRs in the database name.
>>>>>
>>>>>
>>>>> cheers
>>>>>
>>>> Okay. As per discussions, we will keep one line entry for each
>>>> database into map.file.
>>>>
>>>> Thanks all for feedback and review.
>>>>
>>>> Here, I am attaching updated patches for review and testing. These
>>>> patches can be applied on commit a6524105d20b.
>>>
>>>
>>> I'm working through this patch set with a view to committing it.
>>> Attached is some cleanup which is where I got to today, although there
>>> is more to do. One thing I am wondering is why not put the
>>> SimpleDatabaseOidList stuff in fe_utils/simle_list.{c,h} ? That's
>>> where all the similar stuff belongs, and it feels strange to have this
>>> inline in pg_restore.c. (I also don't like the name much -
>>> SimpleOidStringList or maybe SimpleOidPlusStringList might be better).
>>>
>>>
>>>
>>
>> OK, I have done that, so here is the result. The first two are you
>> original patches. patch 3 adds the new list type to fe-utils, and patch
>> 4 contains my cleanups and use of the new list type. Apart from some
>> relatively minor cleanup, the one thing I would like to change is how
>> dumps are named. If we are producing tar or custom format dumps, I think
>> the file names should reflect that (oid.dmp and oid.tar rather than a
>> bare oid as the filename), and pg_restore should look for those. I'm
>> going to work on that tomorrow - I don't think it will be terribly
>> difficult.
>>
> Thanks Andrew.
>
> Here, I am attaching a delta patch for oid.tar and oid.dmp format.
>


OK, looks good, I have incorporated that.

There are a couple of rough edges, though.

First, I see this:


andrew@ub22arm:inst $ bin/pg_restore -C -d postgres 
--exclude-database=regression_dummy_seclabel 
--exclude-database=regression_test_extensions 
--exclude-database=regression_test_pg_dump dest
pg_restore: error: could not execute query: "ERROR:  role "andrew" 
already exists
"
Command was: "

--
-- Roles
--

CREATE ROLE andrew;"
pg_restore: warning: errors ignored on global.dat file restore: 1
pg_restore: error: could not execute query: ERROR:  database "template1" 
already exists
Command was: CREATE DATABASE template1 WITH TEMPLATE = template0 
ENCODING = 'SQL_ASCII' LOCALE_PROVIDER = libc LOCALE = 'C';


pg_restore: warning: errors ignored on database "template1" restore: 1
pg_restore: error: could not execute query: ERROR:  database "postgres" 
already exists
Command was: CREATE DATABASE postgres WITH TEMPLATE = template0 ENCODING 
= 'SQL_ASCII' LOCALE_PROVIDER = libc LOCALE = 'C';


pg_restore: warning: errors ignored on database "postgres" restore: 1
pg_restore: warning: errors ignored on restore: 3



It seems pointless to be trying to create the rolw that we are connected 
as, and we also expect template1 and postgres to exist.

In a similar vein, I don't see why we are setting the --create flag in 
pg_dumpall for those databases. I'm attaching a patch that is designed 
to stop that, but it doesn't solve the above issues.

I also notice a bunch of these in globals.dat:


--
-- Databases
--

--
-- Database "template1" dump
--

--
-- Database "andrew" dump
--

--
-- Database "isolation_regression_brin" dump
--

--
-- Database "isolation_regression_delay_execution" dump
--

  ...


The patch also tries to fix this.

Lastly, this badly needs some TAP tests written.

I'm going to work on reviewing the documentation next.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 2025-03-30 Su 12:50 PM, Andrew Dunstan wrote:
>
> On 2025-03-29 Sa 1:17 AM, Mahendra Singh Thalor wrote:
>> On Sat, 29 Mar 2025 at 03:50, Andrew Dunstan <andrew@dunslane.net> 
>> wrote:
>>>
>>> On 2025-03-27 Th 5:15 PM, Andrew Dunstan wrote:
>>>> On 2025-03-19 We 2:41 AM, Mahendra Singh Thalor wrote:
>>>>> On Wed, 12 Mar 2025 at 21:18, Andrew Dunstan <andrew@dunslane.net>
>>>>> wrote:
>>>>>> On 2025-03-12 We 3:03 AM, jian he wrote:
>>>>>>> On Wed, Mar 12, 2025 at 1:06 AM Álvaro Herrera
>>>>>>> <alvherre@alvh.no-ip.org> wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> On 2025-Mar-11, Mahendra Singh Thalor wrote:
>>>>>>>>
>>>>>>>>> In map.dat file, I tried to fix this issue by adding number of
>>>>>>>>> characters
>>>>>>>>> in dbname but as per code comments, as of now, we are not
>>>>>>>>> supporting \n\r
>>>>>>>>> in dbnames so i removed handling.
>>>>>>>>> I will do some more study to fix this issue.
>>>>>>>> Yeah, I think this is saying that you should not consider the
>>>>>>>> contents
>>>>>>>> of map.dat as a shell string.  After all, you're not going to
>>>>>>>> _execute_
>>>>>>>> that file via the shell.
>>>>>>>>
>>>>>>>> Maybe for map.dat you need to escape such characters somehow, 
>>>>>>>> so that
>>>>>>>> they don't appear as literal newlines/carriage returns.
>>>>>>>>
>>>>>>> I am confused.
>>>>>>> currently pg_dumpall plain format will abort when encountering 
>>>>>>> dbname
>>>>>>> containing newline.
>>>>>>> the left dumped plain file does not contain all the cluster
>>>>>>> databases data.
>>>>>>>
>>>>>>>
>>>>>>> if pg_dumpall non-text format aborts earlier,
>>>>>>> it's aligned with pg_dumpall plain format?
>>>>>>> it's also an improvement since aborts earlier, nothing will be 
>>>>>>> dumped?
>>>>>>>
>>>>>>>
>>>>>>> am i missing something?
>>>>>>>
>>>>>>>
>>>>>> I think we should fix that.
>>>>>>
>>>>>> But for the current proposal, Álvaro and I were talking this 
>>>>>> morning,
>>>>>> and we thought the simplest thing here would be to have the one line
>>>>>> format and escape NL/CRs in the database name.
>>>>>>
>>>>>>
>>>>>> cheers
>>>>>>
>>>>> Okay. As per discussions, we will keep one line entry for each
>>>>> database into map.file.
>>>>>
>>>>> Thanks all for feedback and review.
>>>>>
>>>>> Here, I am attaching updated patches for review and testing. These
>>>>> patches can be applied on commit a6524105d20b.
>>>>
>>>>
>>>> I'm working through this patch set with a view to committing it.
>>>> Attached is some cleanup which is where I got to today, although there
>>>> is more to do. One thing I am wondering is why not put the
>>>> SimpleDatabaseOidList stuff in fe_utils/simle_list.{c,h} ? That's
>>>> where all the similar stuff belongs, and it feels strange to have this
>>>> inline in pg_restore.c. (I also don't like the name much -
>>>> SimpleOidStringList or maybe SimpleOidPlusStringList might be better).
>>>>
>>>>
>>>>
>>>
>>> OK, I have done that, so here is the result. The first two are you
>>> original patches. patch 3 adds the new list type to fe-utils, and patch
>>> 4 contains my cleanups and use of the new list type. Apart from some
>>> relatively minor cleanup, the one thing I would like to change is how
>>> dumps are named. If we are producing tar or custom format dumps, I 
>>> think
>>> the file names should reflect that (oid.dmp and oid.tar rather than a
>>> bare oid as the filename), and pg_restore should look for those. I'm
>>> going to work on that tomorrow - I don't think it will be terribly
>>> difficult.
>>>
>> Thanks Andrew.
>>
>> Here, I am attaching a delta patch for oid.tar and oid.dmp format.
>>
>
>
> OK, looks good, I have incorporated that.
>
> There are a couple of rough edges, though.
>
> First, I see this:
>
>
> andrew@ub22arm:inst $ bin/pg_restore -C -d postgres 
> --exclude-database=regression_dummy_seclabel 
> --exclude-database=regression_test_extensions 
> --exclude-database=regression_test_pg_dump dest
> pg_restore: error: could not execute query: "ERROR:  role "andrew" 
> already exists
> "
> Command was: "
>
> -- 
> -- Roles
> -- 
>
> CREATE ROLE andrew;"
> pg_restore: warning: errors ignored on global.dat file restore: 1
> pg_restore: error: could not execute query: ERROR:  database 
> "template1" already exists
> Command was: CREATE DATABASE template1 WITH TEMPLATE = template0 
> ENCODING = 'SQL_ASCII' LOCALE_PROVIDER = libc LOCALE = 'C';
>
>
> pg_restore: warning: errors ignored on database "template1" restore: 1
> pg_restore: error: could not execute query: ERROR:  database 
> "postgres" already exists
> Command was: CREATE DATABASE postgres WITH TEMPLATE = template0 
> ENCODING = 'SQL_ASCII' LOCALE_PROVIDER = libc LOCALE = 'C';
>
>
> pg_restore: warning: errors ignored on database "postgres" restore: 1
> pg_restore: warning: errors ignored on restore: 3
>
>
>
> It seems pointless to be trying to create the rolw that we are 
> connected as, and we also expect template1 and postgres to exist.
>
> In a similar vein, I don't see why we are setting the --create flag in 
> pg_dumpall for those databases. I'm attaching a patch that is designed 
> to stop that, but it doesn't solve the above issues.
>
> I also notice a bunch of these in globals.dat:
>
>
> -- 
> -- Databases
> -- 
>
> -- 
> -- Database "template1" dump
> -- 
>
> -- 
> -- Database "andrew" dump
> -- 
>
> -- 
> -- Database "isolation_regression_brin" dump
> -- 
>
> -- 
> -- Database "isolation_regression_delay_execution" dump
> -- 
>
>  ...
>
>
> The patch also tries to fix this.
>
> Lastly, this badly needs some TAP tests written.
>
> I'm going to work on reviewing the documentation next.
>
>
>



I have reworked the documentation some. See attached.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 2025-03-31 Mo 5:34 AM, Mahendra Singh Thalor wrote:
>
> >
> > There are a couple of rough edges, though.
> >
> > First, I see this:
> >
> >
> > andrew@ub22arm:inst $ bin/pg_restore -C -d postgres
> > --exclude-database=regression_dummy_seclabel
> > --exclude-database=regression_test_extensions
> > --exclude-database=regression_test_pg_dump dest
> > pg_restore: error: could not execute query: "ERROR:  role "andrew"
> > already exists
> > "
> > Command was: "
> >
> > --
> > -- Roles
> > --
> >
> > CREATE ROLE andrew;"
> > pg_restore: warning: errors ignored on global.dat file restore: 1
> > pg_restore: error: could not execute query: ERROR:  database "template1"
> > already exists
> > Command was: CREATE DATABASE template1 WITH TEMPLATE = template0
> > ENCODING = 'SQL_ASCII' LOCALE_PROVIDER = libc LOCALE = 'C';
> >
> >
> > pg_restore: warning: errors ignored on database "template1" restore: 1
> > pg_restore: error: could not execute query: ERROR:  database "postgres"
> > already exists
> > Command was: CREATE DATABASE postgres WITH TEMPLATE = template0 ENCODING
> > = 'SQL_ASCII' LOCALE_PROVIDER = libc LOCALE = 'C';
> >
> >
> > pg_restore: warning: errors ignored on database "postgres" restore: 1
> > pg_restore: warning: errors ignored on restore: 3
> >
> >
> >
> > It seems pointless to be trying to create the rolw that we are connected
> > as, and we also expect template1 and postgres to exist.
>
> Thanks Andrew for the updated patches.
>
> Here, I am attaching a delta patch which solves the errors for the 
> already created database and we need to reset some flags also. Please 
> have a look over this delta patch and merge it.
>
> If we want to skip errors for connected user (CREATE ROLE username), 
> then we need to handle it by comparing sql commands in 
> process_global_sql_commands function or we can compare errors after 
> executing it.
> delta_0002* patch is doing some handling but this is not a proper fix.
>
> I think we can merge delta_0001* and later, we can work on delta_0002.


Yes, delta 1 looks OK, except that the pstrdup() calls are probably 
unnecessary. Delta 2 needs some significant surgery at least. I think we 
can use it as at least a partial fix, to avoid trying to create the role 
we're running as (Should use PQuser() for that rather than cparams.user).

BTW, if you're sending delta patches, make sure they don't have .patch 
extensions. Otherwise, the CFBot gets upset. I usually just add .noci to 
the file names.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Mon, 31 Mar 2025 at 19:27, Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 2025-03-31 Mo 5:34 AM, Mahendra Singh Thalor wrote:
> >
> > >
> > > There are a couple of rough edges, though.
> > >
> > > First, I see this:
> > >
> > >
> > > andrew@ub22arm:inst $ bin/pg_restore -C -d postgres
> > > --exclude-database=regression_dummy_seclabel
> > > --exclude-database=regression_test_extensions
> > > --exclude-database=regression_test_pg_dump dest
> > > pg_restore: error: could not execute query: "ERROR:  role "andrew"
> > > already exists
> > > "
> > > Command was: "
> > >
> > > --
> > > -- Roles
> > > --
> > >
> > > CREATE ROLE andrew;"
> > > pg_restore: warning: errors ignored on global.dat file restore: 1
> > > pg_restore: error: could not execute query: ERROR:  database "template1"
> > > already exists
> > > Command was: CREATE DATABASE template1 WITH TEMPLATE = template0
> > > ENCODING = 'SQL_ASCII' LOCALE_PROVIDER = libc LOCALE = 'C';
> > >
> > >
> > > pg_restore: warning: errors ignored on database "template1" restore: 1
> > > pg_restore: error: could not execute query: ERROR:  database "postgres"
> > > already exists
> > > Command was: CREATE DATABASE postgres WITH TEMPLATE = template0 ENCODING
> > > = 'SQL_ASCII' LOCALE_PROVIDER = libc LOCALE = 'C';
> > >
> > >
> > > pg_restore: warning: errors ignored on database "postgres" restore: 1
> > > pg_restore: warning: errors ignored on restore: 3
> > >
> > >
> > >
> > > It seems pointless to be trying to create the rolw that we are connected
> > > as, and we also expect template1 and postgres to exist.
> >
> > Thanks Andrew for the updated patches.
> >
> > Here, I am attaching a delta patch which solves the errors for the
> > already created database and we need to reset some flags also. Please
> > have a look over this delta patch and merge it.
> >
> > If we want to skip errors for connected user (CREATE ROLE username),
> > then we need to handle it by comparing sql commands in
> > process_global_sql_commands function or we can compare errors after
> > executing it.
> > delta_0002* patch is doing some handling but this is not a proper fix.
> >
> > I think we can merge delta_0001* and later, we can work on delta_0002.
>
>
> Yes, delta 1 looks OK, except that the pstrdup() calls are probably
> unnecessary. Delta 2 needs some significant surgery at least. I think we
> can use it as at least a partial fix, to avoid trying to create the role
> we're running as (Should use PQuser() for that rather than cparams.user).

Thanks for the quick review.

I fixed the above comments and made 2 delta patches. Please have a
look over these.

> BTW, if you're sending delta patches, make sure they don't have .patch
> extensions. Otherwise, the CFBot gets upset. I usually just add .noci to
> the file names.

Sure. I will also use .noci. Thanks for feedback.

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

On 2025-03-31 Mo 12:16 PM, Mahendra Singh Thalor wrote:
> On Mon, 31 Mar 2025 at 19:27, Andrew Dunstan <andrew@dunslane.net> wrote:
>>
>> On 2025-03-31 Mo 5:34 AM, Mahendra Singh Thalor wrote:
>>>> There are a couple of rough edges, though.
>>>>
>>>> First, I see this:
>>>>
>>>>
>>>> andrew@ub22arm:inst $ bin/pg_restore -C -d postgres
>>>> --exclude-database=regression_dummy_seclabel
>>>> --exclude-database=regression_test_extensions
>>>> --exclude-database=regression_test_pg_dump dest
>>>> pg_restore: error: could not execute query: "ERROR:  role "andrew"
>>>> already exists
>>>> "
>>>> Command was: "
>>>>
>>>> --
>>>> -- Roles
>>>> --
>>>>
>>>> CREATE ROLE andrew;"
>>>> pg_restore: warning: errors ignored on global.dat file restore: 1
>>>> pg_restore: error: could not execute query: ERROR:  database "template1"
>>>> already exists
>>>> Command was: CREATE DATABASE template1 WITH TEMPLATE = template0
>>>> ENCODING = 'SQL_ASCII' LOCALE_PROVIDER = libc LOCALE = 'C';
>>>>
>>>>
>>>> pg_restore: warning: errors ignored on database "template1" restore: 1
>>>> pg_restore: error: could not execute query: ERROR:  database "postgres"
>>>> already exists
>>>> Command was: CREATE DATABASE postgres WITH TEMPLATE = template0 ENCODING
>>>> = 'SQL_ASCII' LOCALE_PROVIDER = libc LOCALE = 'C';
>>>>
>>>>
>>>> pg_restore: warning: errors ignored on database "postgres" restore: 1
>>>> pg_restore: warning: errors ignored on restore: 3
>>>>
>>>>
>>>>
>>>> It seems pointless to be trying to create the rolw that we are connected
>>>> as, and we also expect template1 and postgres to exist.
>>> Thanks Andrew for the updated patches.
>>>
>>> Here, I am attaching a delta patch which solves the errors for the
>>> already created database and we need to reset some flags also. Please
>>> have a look over this delta patch and merge it.
>>>
>>> If we want to skip errors for connected user (CREATE ROLE username),
>>> then we need to handle it by comparing sql commands in
>>> process_global_sql_commands function or we can compare errors after
>>> executing it.
>>> delta_0002* patch is doing some handling but this is not a proper fix.
>>>
>>> I think we can merge delta_0001* and later, we can work on delta_0002.
>>
>> Yes, delta 1 looks OK, except that the pstrdup() calls are probably
>> unnecessary. Delta 2 needs some significant surgery at least. I think we
>> can use it as at least a partial fix, to avoid trying to create the role
>> we're running as (Should use PQuser() for that rather than cparams.user).
> Thanks for the quick review.
>
> I fixed the above comments and made 2 delta patches. Please have a
> look over these.
>
>> BTW, if you're sending delta patches, make sure they don't have .patch
>> extensions. Otherwise, the CFBot gets upset. I usually just add .noci to
>> the file names.
> Sure. I will also use .noci. Thanks for feedback.


Thanks. Here are patches that contain (my version of) all the cleanups. 
With this I get a clean restore run in my test case with no error messages.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 2025-03-31 Mo 1:16 PM, Andrew Dunstan wrote:
>
>
>
> Thanks. Here are patches that contain (my version of) all the 
> cleanups. With this I get a clean restore run in my test case with no 
> error messages.
>
>
>

This time with patches


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi

FWIW I don't think the on_exit_nicely business is in final shape just
yet.  We're doing something super strange and novel about keeping track
of an array index, so that we can modify it later.  Or something like
that, I think?  That doesn't sound all that nice to me.  Elsewhere it
was suggested that we need some way to keep track of the list of things
that need cleanup (a list of connections IIRC?) -- perhaps in a
thread-local variable or a global or something -- and we install the
cleanup function once, and that reads from the variable.  The program
can add things to the list, or remove them, at will; and we don't need
to modify the cleanup function in any way.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

On Mon, 31 Mar 2025 at 23:43, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Hi
>
> FWIW I don't think the on_exit_nicely business is in final shape just
> yet.  We're doing something super strange and novel about keeping track
> of an array index, so that we can modify it later.  Or something like
> that, I think?  That doesn't sound all that nice to me.  Elsewhere it
> was suggested that we need some way to keep track of the list of things
> that need cleanup (a list of connections IIRC?) -- perhaps in a
> thread-local variable or a global or something -- and we install the
> cleanup function once, and that reads from the variable.  The program
> can add things to the list, or remove them, at will; and we don't need
> to modify the cleanup function in any way.
>
> --
> Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

Thanks Álvaro for the feedback.

I removed the old handling of on_exit_nicely_list from the last patch
set and added one simple function to just update the archive handle in
shutdown_info.  (shutdown_info.AHX = AHX;)

For first database, we will add entry into on_exit_nicely_list array
and for rest database, we will update only shutdown_info as we already
closed connection for previous database.With this fix, we will not
touch entry of on_exit_nicely_list for each database.

Here, I am attaching updated patches.

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

On 2025-04-01 Tu 1:59 AM, Mahendra Singh Thalor wrote:
> On Mon, 31 Mar 2025 at 23:43, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>> Hi
>>
>> FWIW I don't think the on_exit_nicely business is in final shape just
>> yet.  We're doing something super strange and novel about keeping track
>> of an array index, so that we can modify it later.  Or something like
>> that, I think?  That doesn't sound all that nice to me.  Elsewhere it
>> was suggested that we need some way to keep track of the list of things
>> that need cleanup (a list of connections IIRC?) -- perhaps in a
>> thread-local variable or a global or something -- and we install the
>> cleanup function once, and that reads from the variable.  The program
>> can add things to the list, or remove them, at will; and we don't need
>> to modify the cleanup function in any way.
>>
>> --
>> Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
> Thanks Álvaro for the feedback.
>
> I removed the old handling of on_exit_nicely_list from the last patch
> set and added one simple function to just update the archive handle in
> shutdown_info.  (shutdown_info.AHX = AHX;)
>
> For first database, we will add entry into on_exit_nicely_list array
> and for rest database, we will update only shutdown_info as we already
> closed connection for previous database.With this fix, we will not
> touch entry of on_exit_nicely_list for each database.
>
> Here, I am attaching updated patches.
>


OK, looks good. here's my latest. I'm currently working on tidying up 
docco and comments.


cheers


andrew




--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Fri, 4 Apr 2025 at 01:17, Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 2025-04-01 Tu 1:59 AM, Mahendra Singh Thalor wrote:
> > On Mon, 31 Mar 2025 at 23:43, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >> Hi
> >>
> >> FWIW I don't think the on_exit_nicely business is in final shape just
> >> yet.  We're doing something super strange and novel about keeping track
> >> of an array index, so that we can modify it later.  Or something like
> >> that, I think?  That doesn't sound all that nice to me.  Elsewhere it
> >> was suggested that we need some way to keep track of the list of things
> >> that need cleanup (a list of connections IIRC?) -- perhaps in a
> >> thread-local variable or a global or something -- and we install the
> >> cleanup function once, and that reads from the variable.  The program
> >> can add things to the list, or remove them, at will; and we don't need
> >> to modify the cleanup function in any way.
> >>
> >> --
> >> Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
> > Thanks Álvaro for the feedback.
> >
> > I removed the old handling of on_exit_nicely_list from the last patch
> > set and added one simple function to just update the archive handle in
> > shutdown_info.  (shutdown_info.AHX = AHX;)
> >
> > For first database, we will add entry into on_exit_nicely_list array
> > and for rest database, we will update only shutdown_info as we already
> > closed connection for previous database.With this fix, we will not
> > touch entry of on_exit_nicely_list for each database.
> >
> > Here, I am attaching updated patches.
> >
>
>
> OK, looks good. here's my latest. I'm currently working on tidying up
> docco and comments.
>
>
> cheers
>
>
> andrew
>
>
>
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com

Thanks Andrew for the updated patches.

Here, I am attaching a delta patch with some more TAP-test cases.

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

On Fri, 4 Apr 2025 at 13:52, Mahendra Singh Thalor <mahi6run@gmail.com> wrote:
>
> On Fri, 4 Apr 2025 at 01:17, Andrew Dunstan <andrew@dunslane.net> wrote:
> >
> >
> > On 2025-04-01 Tu 1:59 AM, Mahendra Singh Thalor wrote:
> > > On Mon, 31 Mar 2025 at 23:43, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > >> Hi
> > >>
> > >> FWIW I don't think the on_exit_nicely business is in final shape just
> > >> yet.  We're doing something super strange and novel about keeping track
> > >> of an array index, so that we can modify it later.  Or something like
> > >> that, I think?  That doesn't sound all that nice to me.  Elsewhere it
> > >> was suggested that we need some way to keep track of the list of things
> > >> that need cleanup (a list of connections IIRC?) -- perhaps in a
> > >> thread-local variable or a global or something -- and we install the
> > >> cleanup function once, and that reads from the variable.  The program
> > >> can add things to the list, or remove them, at will; and we don't need
> > >> to modify the cleanup function in any way.
> > >>
> > >> --
> > >> Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
> > > Thanks Álvaro for the feedback.
> > >
> > > I removed the old handling of on_exit_nicely_list from the last patch
> > > set and added one simple function to just update the archive handle in
> > > shutdown_info.  (shutdown_info.AHX = AHX;)
> > >
> > > For first database, we will add entry into on_exit_nicely_list array
> > > and for rest database, we will update only shutdown_info as we already
> > > closed connection for previous database.With this fix, we will not
> > > touch entry of on_exit_nicely_list for each database.
> > >
> > > Here, I am attaching updated patches.
> > >
> >
> >
> > OK, looks good. here's my latest. I'm currently working on tidying up
> > docco and comments.
> >
> >
> > cheers
> >
> >
> > andrew
> >
> >
> >
> >
> > --
> > Andrew Dunstan
> > EDB: https://www.enterprisedb.com
>
> Thanks Andrew for the updated patches.
>
> Here, I am attaching a delta patch with some more TAP-test cases.
>

Here, I am attaching an updated delta patch which has some more TAP
tests. Please include these tests also. This patch can be applied on
v20250403_0004* patch.


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

On Fri, 4 Apr 2025 at 13:52, Mahendra Singh Thalor <mahi6run@gmail.com> wrote:

On Fri, 4 Apr 2025 at 01:17, Andrew Dunstan <andrew@dunslane.net> wrote:

On 2025-04-01 Tu 1:59 AM, Mahendra Singh Thalor wrote:

On Mon, 31 Mar 2025 at 23:43, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:

Hi

FWIW I don't think the on_exit_nicely business is in final shape just
yet.  We're doing something super strange and novel about keeping track
of an array index, so that we can modify it later.  Or something like
that, I think?  That doesn't sound all that nice to me.  Elsewhere it
was suggested that we need some way to keep track of the list of things
that need cleanup (a list of connections IIRC?) -- perhaps in a
thread-local variable or a global or something -- and we install the
cleanup function once, and that reads from the variable.  The program
can add things to the list, or remove them, at will; and we don't need
to modify the cleanup function in any way.

--
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

Thanks Álvaro for the feedback.

I removed the old handling of on_exit_nicely_list from the last patch
set and added one simple function to just update the archive handle in
shutdown_info.  (shutdown_info.AHX = AHX;)

For first database, we will add entry into on_exit_nicely_list array
and for rest database, we will update only shutdown_info as we already
closed connection for previous database.With this fix, we will not
touch entry of on_exit_nicely_list for each database.

Here, I am attaching updated patches.

OK, looks good. here's my latest. I'm currently working on tidying up
docco and comments.


cheers


andrew




--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Thanks Andrew for the updated patches.

Here, I am attaching a delta patch with some more TAP-test cases.

Here, I am attaching an updated delta patch which has some more TAP
tests. Please include these tests also. This patch can be applied on
v20250403_0004* patch.

Thanks. I have pushed these now with a few further small tweaks.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Sat, 5 Apr 2025 at 01:41, Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 2025-04-04 Fr 5:12 AM, Mahendra Singh Thalor wrote:
>
> On Fri, 4 Apr 2025 at 13:52, Mahendra Singh Thalor <mahi6run@gmail.com> wrote:
>
> On Fri, 4 Apr 2025 at 01:17, Andrew Dunstan <andrew@dunslane.net> wrote:
>
> On 2025-04-01 Tu 1:59 AM, Mahendra Singh Thalor wrote:
>
> On Mon, 31 Mar 2025 at 23:43, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Hi
>
> FWIW I don't think the on_exit_nicely business is in final shape just
> yet.  We're doing something super strange and novel about keeping track
> of an array index, so that we can modify it later.  Or something like
> that, I think?  That doesn't sound all that nice to me.  Elsewhere it
> was suggested that we need some way to keep track of the list of things
> that need cleanup (a list of connections IIRC?) -- perhaps in a
> thread-local variable or a global or something -- and we install the
> cleanup function once, and that reads from the variable.  The program
> can add things to the list, or remove them, at will; and we don't need
> to modify the cleanup function in any way.
>
> --
> Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
>
> Thanks Álvaro for the feedback.
>
> I removed the old handling of on_exit_nicely_list from the last patch
> set and added one simple function to just update the archive handle in
> shutdown_info.  (shutdown_info.AHX = AHX;)
>
> For first database, we will add entry into on_exit_nicely_list array
> and for rest database, we will update only shutdown_info as we already
> closed connection for previous database.With this fix, we will not
> touch entry of on_exit_nicely_list for each database.
>
> Here, I am attaching updated patches.
>
>
> OK, looks good. here's my latest. I'm currently working on tidying up
> docco and comments.
>
>
> cheers
>
>
> andrew
>
>
>
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com
>
> Thanks Andrew for the updated patches.
>
> Here, I am attaching a delta patch with some more TAP-test cases.
>
> Here, I am attaching an updated delta patch which has some more TAP
> tests. Please include these tests also. This patch can be applied on
> v20250403_0004* patch.
>
>
>
> Thanks. I have pushed these now with a few further small tweaks.
>
>
> cheers
>
>
> andrew
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com

Thanks Andrew for committing this.

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

Thanks. I have pushed these now with a few further small tweaks.

Sorry if it is not the right place.

Coverity has another resource leak alert.

trivial patch attached.

Thanks for checking. Pushed.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Em qui., 10 de abr. de 2025 às 15:58, Andrew Dunstan <andrew@dunslane.net> escreveu:

On 2025-04-10 Th 2:38 PM, Ranier Vilela wrote:

Thanks. I have pushed these now with a few further small tweaks.

Sorry if it is not the right place.

Coverity has another resource leak alert.

trivial patch attached.

Thanks for checking. Pushed.

Andew, I think that the commit wasn't very correct.

Now the variable *q* is being destroyed inside the loop.

The patch was destroying the variable *q* (stringinfo),

after the loop while.

 

Yes, you're right. Must be blind. Fixed.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Sat, 5 Apr 2025 at 01:41, Andrew Dunstan <andrew@dunslane.net> wrote:
>
>
> On 2025-04-04 Fr 5:12 AM, Mahendra Singh Thalor wrote:
>
> On Fri, 4 Apr 2025 at 13:52, Mahendra Singh Thalor <mahi6run@gmail.com> wrote:
>
> On Fri, 4 Apr 2025 at 01:17, Andrew Dunstan <andrew@dunslane.net> wrote:
>
> On 2025-04-01 Tu 1:59 AM, Mahendra Singh Thalor wrote:
>
> On Mon, 31 Mar 2025 at 23:43, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Hi
>
> FWIW I don't think the on_exit_nicely business is in final shape just
> yet.  We're doing something super strange and novel about keeping track
> of an array index, so that we can modify it later.  Or something like
> that, I think?  That doesn't sound all that nice to me.  Elsewhere it
> was suggested that we need some way to keep track of the list of things
> that need cleanup (a list of connections IIRC?) -- perhaps in a
> thread-local variable or a global or something -- and we install the
> cleanup function once, and that reads from the variable.  The program
> can add things to the list, or remove them, at will; and we don't need
> to modify the cleanup function in any way.
>
> --
> Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
>
> Thanks Álvaro for the feedback.
>
> I removed the old handling of on_exit_nicely_list from the last patch
> set and added one simple function to just update the archive handle in
> shutdown_info.  (shutdown_info.AHX = AHX;)
>
> For first database, we will add entry into on_exit_nicely_list array
> and for rest database, we will update only shutdown_info as we already
> closed connection for previous database.With this fix, we will not
> touch entry of on_exit_nicely_list for each database.
>
> Here, I am attaching updated patches.
>
>
> OK, looks good. here's my latest. I'm currently working on tidying up
> docco and comments.
>
>
> cheers
>
>
> andrew
>
>
>
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com
>
> Thanks Andrew for the updated patches.
>
> Here, I am attaching a delta patch with some more TAP-test cases.
>
> Here, I am attaching an updated delta patch which has some more TAP
> tests. Please include these tests also. This patch can be applied on
> v20250403_0004* patch.
>
>
>
> Thanks. I have pushed these now with a few further small tweaks.
>
>
> cheers
>
>
> andrew
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com

Hi Andrew,
I did some refactoring to find out dump file extensions(.dmp/.tar etc)
in pg_restore. With the attached patch, we will not try to find out
file extension with each database, rather we will find out before the
loop.

Here, I am attaching a patch for the same. Please have a look over this.

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

On 2025-04-15 Tu 2:30 PM, Mahendra Singh Thalor wrote:
> Hi Andrew,
> I did some refactoring to find out dump file extensions(.dmp/.tar etc)
> in pg_restore. With the attached patch, we will not try to find out
> file extension with each database, rather we will find out before the
> loop.
>
> Here, I am attaching a patch for the same. Please have a look over this.



That doesn't look right at first glance. You shouldn't have to tell 
pg_restore what format to use, it should be able to intuit it from the 
dumps (and that's what the docs say it does).

The saving here would be hardly measurable anyway - you would be in 
effect saving one or two stat calls per database.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com