public inbox for [email protected]
help / color / mirror / Atom feedFrom: Mahendra Singh Thalor <[email protected]>
To: Andrew Dunstan <[email protected]>
Cc: jian he <[email protected]>
Cc: Álvaro Herrera <[email protected]>
Cc: Srinath Reddy <[email protected]>
Cc: [email protected]
Subject: Re: Non-text mode for pg_dumpall
Date: Mon, 31 Mar 2025 15:04:38 +0530
Message-ID: <CAKYtNApiDSUvBT-abuFUdjuayrbG-tmrxxqn8WrwLKRvXV3J+w@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CAKYtNAr7ToCBSLO2Mm6NGFTZvbdTAFNgjv_YoRqJJghC3n1MiQ@mail.gmail.com>
<[email protected]>
<CACJufxG44zd+KV4B3_0d+Lf2fwnDDptX-DQRqn-o9ukmXEWeNw@mail.gmail.com>
<[email protected]>
<CAKYtNAoebXn-CC_cgv=-DxL1s7hPvx-1UW7CEfsiJZ8fbD6pjg@mail.gmail.com>
<[email protected]>
<[email protected]>
<CAKYtNAq8PeyK_bhqX7kR_WPp0x+Z3GAVPnqyBCxe5SVa1MRB7A@mail.gmail.com>
<[email protected]>
On Sun, 30 Mar 2025 at 22:20, Andrew Dunstan <[email protected]> wrote:
>
>
> On 2025-03-29 Sa 1:17 AM, Mahendra Singh Thalor wrote:
> > On Sat, 29 Mar 2025 at 03:50, Andrew Dunstan <[email protected]>
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 <[email protected]>
> >>>> wrote:
> >>>>> On 2025-03-12 We 3:03 AM, jian he wrote:
> >>>>>> On Wed, Mar 12, 2025 at 1:06 AM Álvaro Herrera
> >>>>>> <[email protected]> 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.
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.
>
> 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.
Thank you.
>
>
> cheers
>
>
> andrew
>
> --
> Andrew Dunstan
> EDB: https://www.enterprisedb.com
--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com
Attachments:
[application/octet-stream] delta_0002-pg_restore-skip-error-for-CRETE-ROLE-username.patch (3.7K, 3-delta_0002-pg_restore-skip-error-for-CRETE-ROLE-username.patch)
download | inline diff:
From f795accf5fede15476300a43ea27135708b5d1e1 Mon Sep 17 00:00:00 2001
From: Mahendra Singh Thalor <[email protected]>
Date: Mon, 31 Mar 2025 14:59:13 +0530
Subject: [PATCH] pg_restore: skip error for CRETE ROLE username
---
src/bin/pg_dump/pg_restore.c | 32 +++++++++++++++++++++++++++-----
1 file changed, 27 insertions(+), 5 deletions(-)
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 3d8be43241d..8ce5b790e51 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -64,7 +64,7 @@ static int read_one_statement(StringInfo inBuf, FILE *pfile);
static int restoreAllDatabases(PGconn *conn, const char *dumpdirpath,
SimpleStringList db_exclude_patterns, RestoreOptions *opts, int numWorkers);
static int process_global_sql_commands(PGconn *conn, const char *dumpdirpath,
- const char *outfile);
+ const char *outfile, const char *username);
static void copy_or_print_global_file(const char *outfile, FILE *pfile);
static int get_dbnames_list_to_restore(PGconn *conn,
SimpleOidStringList * dbname_oid_list,
@@ -543,7 +543,7 @@ main(int argc, char **argv)
* commands.
*/
n_errors = process_global_sql_commands(conn, inputFileSpec,
- opts->filename);
+ opts->filename, opts->cparams.username);
if (conn)
PQfinish(conn);
@@ -1123,7 +1123,7 @@ restoreAllDatabases(PGconn *conn, const char *dumpdirpath,
* file.
*/
if (dbname_oid_list.head == NULL)
- return process_global_sql_commands(conn, dumpdirpath, opts->filename);
+ return process_global_sql_commands(conn, dumpdirpath, opts->filename, opts->cparams.username);
pg_log_info("found total %d database names in map.dat file", num_total_db);
@@ -1153,7 +1153,7 @@ restoreAllDatabases(PGconn *conn, const char *dumpdirpath,
db_exclude_patterns);
/* Open global.dat file and execute/append all the global sql commands. */
- n_errors_total = process_global_sql_commands(conn, dumpdirpath, opts->filename);
+ n_errors_total = process_global_sql_commands(conn, dumpdirpath, opts->filename, opts->cparams.username);
/* Close the db connection as we are done with globals and patterns. */
if (conn)
@@ -1280,11 +1280,13 @@ restoreAllDatabases(PGconn *conn, const char *dumpdirpath,
* returns the number of errors while processing global.dat
*/
static int
-process_global_sql_commands(PGconn *conn, const char *dumpdirpath, const char *outfile)
+process_global_sql_commands(PGconn *conn, const char *dumpdirpath,
+ const char *outfile, const char *username)
{
char global_file_path[MAXPGPATH];
PGresult *result;
StringInfoData sqlstatement;
+ StringInfoData rolesqlstatement;
FILE *pfile;
int n_errors = 0;
@@ -1308,10 +1310,30 @@ process_global_sql_commands(PGconn *conn, const char *dumpdirpath, const char *o
/* Init sqlstatement to append commands. */
initStringInfo(&sqlstatement);
+ initStringInfo(&rolesqlstatement);
/* Process file till EOF and execute sql statements. */
while (read_one_statement(&sqlstatement, pfile) != EOF)
{
+ if (username)
+ {
+ appendStringInfoString(&rolesqlstatement, "\n\n--\n-- Roles\n--\n\nCREATE ROLE ");
+ appendStringInfoString(&rolesqlstatement, username);
+ appendStringInfoString(&rolesqlstatement, ";");
+ }
+
+ /*
+ * If this command is for "CREATE ROLE username", then skip this as
+ * current user is already created.
+ */
+ if (username && strcmp(sqlstatement.data, rolesqlstatement.data) == 0)
+ {
+ resetStringInfo(&rolesqlstatement);
+ continue;
+ }
+
+ resetStringInfo(&rolesqlstatement);
+
pg_log_info("executing query: %s", sqlstatement.data);
result = PQexec(conn, sqlstatement.data);
--
2.39.3
[application/octet-stream] delta-0001-pg_restore-skip-error-if-db-already-created.patch (2.3K, 4-delta-0001-pg_restore-skip-error-if-db-already-created.patch)
download | inline diff:
From eb1bd481d4216bb27db227410cc48edc4bf2634e Mon Sep 17 00:00:00 2001
From: Mahendra Singh Thalor <[email protected]>
Date: Mon, 31 Mar 2025 14:01:41 +0530
Subject: [PATCH] pg_restore: if database is already created, then set createdb
as 0
If database is already created, then set createdb as 0 so that user
will not get errors.
Also reset some flags for each database.
as dumpData, dumpSchema, dumpStatistics
---
src/bin/pg_dump/pg_restore.c | 38 ++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index ce70b7e12b2..3d8be43241d 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -1107,6 +1107,14 @@ restoreAllDatabases(PGconn *conn, const char *dumpdirpath,
int num_total_db;
int n_errors_total;
int count = 0;
+ char *connected_db = NULL;
+ bool dumpData = opts->dumpData;
+ bool dumpSchema = opts->dumpSchema;
+ bool dumpStatistics = opts->dumpSchema;
+
+ /* Save db name. */
+ if (opts->cparams.dbname)
+ connected_db = pg_strdup(opts->cparams.dbname);
num_total_db = get_dbname_oid_list_from_mfile(dumpdirpath, &dbname_oid_list);
@@ -1209,9 +1217,39 @@ restoreAllDatabases(PGconn *conn, const char *dumpdirpath,
pg_log_info("restoring database \"%s\"", db_cell->str);
+ /* If database is already created, then don't set createDB flag. */
+ if (opts->cparams.dbname)
+ {
+ conn = ConnectDatabase(db_cell->str, NULL, opts->cparams.pghost,
+ opts->cparams.pgport, opts->cparams.username, TRI_DEFAULT,
+ false, progname, NULL, NULL, NULL, NULL);
+
+ if (conn)
+ {
+ opts->createDB = 0;
+ PQfinish(conn);
+
+ /* Use already created database for connection. */
+ if (opts->cparams.dbname)
+ opts->cparams.dbname = pg_strdup(db_cell->str);
+ }
+ }
+
/* Restore single database. */
n_errors = restoreOneDatabase(subdirpath, opts, numWorkers, true, count);
+ /* Set opts->createDB flag. */
+ if (opts->createDB == 0)
+ {
+ opts->createDB = 1;
+ opts->cparams.dbname = pg_strdup(connected_db);
+ }
+
+ /* Reset flags for next database. */
+ opts->dumpData = dumpData;
+ opts->dumpSchema = dumpSchema;
+ opts->dumpStatistics = dumpStatistics;
+
/* Print a summary of ignored errors during single database restore. */
if (n_errors)
{
--
2.39.3
view thread (99+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Non-text mode for pg_dumpall
In-Reply-To: <CAKYtNApiDSUvBT-abuFUdjuayrbG-tmrxxqn8WrwLKRvXV3J+w@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox