why -Fdance archive format option works with ./pg_restore but not with ./pg_dump?
Hi,
in pg_restore archive format option is parsed using only the first
character of the value but in pg_dump the archive format option's value is
compared with whole string using pg_strcasecmp is there any specific reason?
Regards,
Srinath Reddy Sadipiralla
EnterpriseDB: http://www.enterprisedb.com
On 2025-01-23 Th 3:18 AM, Srinath Reddy wrote:
Hi,
in pg_restore archive format option is parsed using only the first
character of the value but in pg_dump the archive format option's
value is compared with whole string using pg_strcasecmp is there any
specific reason?
`git blame` tells me that this switch statement goes back to 2001.
Seeking a reason at this distance is unlikely to be productive. Probably
we were not nearly as careful then as we are now about such things. Feel
free to submit a patch tightening the test.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On Fri, Jan 24, 2025 at 1:42 AM Andrew Dunstan <andrew@dunslane.net> wrote:
`git blame` tells me that this switch statement goes back to 2001.
Seeking a reason at this distance is unlikely to be productive. Probably
we were not nearly as careful then as we are now about such things. Feel
free to submit a patch tightening the test.
Hi Andrew,
thanks for the reply,here's the patch for the same.
Regards,
Srinath Reddy Sadipiralla
EnterpriseDB: http://www.enterprisedb.com
Attachments:
0001-Compare-whole-string-value-using-pg_strcasecmp-pg_restore.patchapplication/octet-stream; name=0001-Compare-whole-string-value-using-pg_strcasecmp-pg_restore.patchDownload
From c2b9332040b5d04b461864d324aa49fe3668935d Mon Sep 17 00:00:00 2001
From: Srinath Reddy Sadipiralla <srinath2133@gmail.com>
Date: Fri, 24 Jan 2025 13:20:36 +0530
Subject: [PATCH] Compare whole string value using pg_strcasecmp instead of
comparing first char of the string for the --format option in pg_restore
binary.
---
src/bin/pg_dump/Makefile | 3 +-
src/bin/pg_dump/common_dump_restore_utils.c | 44 +++++++++++++++++++++
src/bin/pg_dump/common_dump_restore_utils.h | 20 ++++++++++
src/bin/pg_dump/pg_dump.c | 29 +++++---------
src/bin/pg_dump/pg_restore.c | 28 +++----------
5 files changed, 80 insertions(+), 44 deletions(-)
create mode 100644 src/bin/pg_dump/common_dump_restore_utils.c
create mode 100644 src/bin/pg_dump/common_dump_restore_utils.h
diff --git a/src/bin/pg_dump/Makefile b/src/bin/pg_dump/Makefile
index 233ad15ca7..08d6181b6a 100644
--- a/src/bin/pg_dump/Makefile
+++ b/src/bin/pg_dump/Makefile
@@ -40,7 +40,8 @@ OBJS = \
pg_backup_directory.o \
pg_backup_null.o \
pg_backup_tar.o \
- pg_backup_utils.o
+ pg_backup_utils.o \
+ common_dump_restore_utils.o
all: pg_dump pg_restore pg_dumpall
diff --git a/src/bin/pg_dump/common_dump_restore_utils.c b/src/bin/pg_dump/common_dump_restore_utils.c
new file mode 100644
index 0000000000..4faa352703
--- /dev/null
+++ b/src/bin/pg_dump/common_dump_restore_utils.c
@@ -0,0 +1,44 @@
+/*-------------------------------------------------------------------------
+ *
+ * common_dump_restore_utils.c
+ * Common routines between pg_dump 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_dump_restore_utils.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#include "postgres_fe.h"
+#include "pg_backup_utils.h"
+#include "common_dump_restore_utils.h"
+
+ArchiveFormat
+parseArchiveFormat(const char *format)
+{
+ ArchiveFormat archiveFormat;
+
+ if (pg_strcasecmp(format, "c") == 0)
+ archiveFormat = archCustom;
+ else if (pg_strcasecmp(format, "custom") == 0)
+ archiveFormat = archCustom;
+ else if (pg_strcasecmp(format, "d") == 0)
+ archiveFormat = archDirectory;
+ else if (pg_strcasecmp(format, "directory") == 0)
+ archiveFormat = archDirectory;
+ else if (pg_strcasecmp(format, "p") == 0)
+ archiveFormat = archNull;
+ else if (pg_strcasecmp(format, "plain") == 0)
+ archiveFormat = archNull;
+ else if (pg_strcasecmp(format, "t") == 0)
+ archiveFormat = archTar;
+ else if (pg_strcasecmp(format, "tar") == 0)
+ archiveFormat = archTar;
+ else
+ archiveFormat = archUnknown;
+ return archiveFormat;
+}
\ No newline at end of file
diff --git a/src/bin/pg_dump/common_dump_restore_utils.h b/src/bin/pg_dump/common_dump_restore_utils.h
new file mode 100644
index 0000000000..90f180d8b6
--- /dev/null
+++ b/src/bin/pg_dump/common_dump_restore_utils.h
@@ -0,0 +1,20 @@
+/*-------------------------------------------------------------------------
+ *
+ * common_dumpall_restore.h
+ * Common header file for pg_dump 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_dump_restore_utils.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef COMMON_DUMP_RESTORE_H
+#define COMMON_DUMP_RESTORE_H
+
+#include "pg_backup.h"
+
+extern ArchiveFormat parseArchiveFormat(const char *format);
+#endif /* COMMON_DUMP_RESTORE_H */
\ No newline at end of file
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index af857f00c7..587dc8f2d9 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -68,6 +68,7 @@
#include "pg_backup_utils.h"
#include "pg_dump.h"
#include "storage/block.h"
+#include "common_dump_restore_utils.h"
typedef struct
{
@@ -232,7 +233,7 @@ static void help(const char *progname);
static void setup_connection(Archive *AH,
const char *dumpencoding, const char *dumpsnapshot,
char *use_role);
-static ArchiveFormat parseArchiveFormat(const char *format, ArchiveMode *mode);
+static ArchiveFormat parseArchiveFormatWithMode(const char *format, ArchiveMode *mode);
static void expand_schema_name_patterns(Archive *fout,
SimpleStringList *patterns,
SimpleOidList *oids,
@@ -810,7 +811,7 @@ main(int argc, char **argv)
pg_fatal("option --on-conflict-do-nothing requires option --inserts, --rows-per-insert, or --column-inserts");
/* Identify archive format to emit */
- archiveFormat = parseArchiveFormat(format, &archiveMode);
+ archiveFormat = parseArchiveFormatWithMode(format, &archiveMode);
/* archiveFormat specific setup */
if (archiveFormat == archNull)
@@ -1455,7 +1456,7 @@ get_synchronized_snapshot(Archive *fout)
}
static ArchiveFormat
-parseArchiveFormat(const char *format, ArchiveMode *mode)
+parseArchiveFormatWithMode(const char *format, ArchiveMode *mode)
{
ArchiveFormat archiveFormat;
@@ -1467,24 +1468,12 @@ parseArchiveFormat(const char *format, ArchiveMode *mode)
archiveFormat = archNull;
*mode = archModeAppend;
}
- else if (pg_strcasecmp(format, "c") == 0)
- archiveFormat = archCustom;
- else if (pg_strcasecmp(format, "custom") == 0)
- archiveFormat = archCustom;
- else if (pg_strcasecmp(format, "d") == 0)
- archiveFormat = archDirectory;
- else if (pg_strcasecmp(format, "directory") == 0)
- archiveFormat = archDirectory;
- else if (pg_strcasecmp(format, "p") == 0)
- archiveFormat = archNull;
- else if (pg_strcasecmp(format, "plain") == 0)
- archiveFormat = archNull;
- else if (pg_strcasecmp(format, "t") == 0)
- archiveFormat = archTar;
- else if (pg_strcasecmp(format, "tar") == 0)
- archiveFormat = archTar;
else
- pg_fatal("invalid output format \"%s\" specified", format);
+ {
+ archiveFormat = parseArchiveFormat(format);
+ if(archiveFormat == archUnknown)
+ pg_fatal("invalid output format \"%s\" specified", format);
+ }
return archiveFormat;
}
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 88ae39d938..9e7fba4b86 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -50,6 +50,7 @@
#include "getopt_long.h"
#include "parallel.h"
#include "pg_backup_utils.h"
+#include "common_dump_restore_utils.h"
static void usage(const char *progname);
static void read_restore_filters(const char *filename, RestoreOptions *opts);
@@ -381,29 +382,10 @@ main(int argc, char **argv)
opts->if_exists = if_exists;
opts->strict_names = strict_names;
- if (opts->formatName)
- {
- switch (opts->formatName[0])
- {
- case 'c':
- case 'C':
- opts->format = archCustom;
- break;
-
- case 'd':
- case 'D':
- opts->format = archDirectory;
- break;
-
- case 't':
- case 'T':
- opts->format = archTar;
- break;
-
- default:
- pg_fatal("unrecognized archive format \"%s\"; please specify \"c\", \"d\", or \"t\"",
- opts->formatName);
- }
+ if (opts->formatName){
+ opts->format = parseArchiveFormat(opts->formatName);
+ if(opts->format == archUnknown)
+ pg_fatal("unrecognized archive format \"%s\"; please specify \"c\", \"d\", \"p\", or \"t\"",opts->formatName);
}
AH = OpenArchive(inputFileSpec, opts->format);
--
2.43.0
On 2025-01-24 Fr 3:04 AM, Srinath Reddy wrote:
On Fri, Jan 24, 2025 at 1:42 AM Andrew Dunstan <andrew@dunslane.net>
wrote:`git blame` tells me that this switch statement goes back to 2001.
Seeking a reason at this distance is unlikely to be productive.
Probably
we were not nearly as careful then as we are now about such
things. Feel
free to submit a patch tightening the test.Hi Andrew,
thanks for the reply,here's the patch for the same.
I don't think we need a new file for this. pg_backup_utils.c is already
there for routines common to pg_restore and pg_dump.
cheers
andrew
--
Andrew Dunstan
EDB:https://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes:
I don't think we need a new file for this. pg_backup_utils.c is already
there for routines common to pg_restore and pg_dump.
I'm not even on board with having a new function, because I doubt
we should try to share this code in the first place. Who's to
say that pg_dump and pg_restore must support exactly the same list
of formats? For example, in the future we might decide that some
format is obsolete and desupport it in pg_dump, while continuing
to allow it for awhile in pg_restore for compatibility reasons.
A closer-to-home possibility is that the work to allow non-text
output from pg_dumpall will result in a format that pg_restore
can read but pg_dump (by itself) doesn't write.
So I'd just scrap pg_restore's parsing logic for this and replace it
in-place. To the extent that that's copying and pasting stuff, fine.
It's not like there's no other duplicativeness in their switch-parsing
logic.
regards, tom lane
On 2025-01-24 Fr 10:24 AM, Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
I don't think we need a new file for this. pg_backup_utils.c is already
there for routines common to pg_restore and pg_dump.I'm not even on board with having a new function, because I doubt
we should try to share this code in the first place. Who's to
say that pg_dump and pg_restore must support exactly the same list
of formats? For example, in the future we might decide that some
format is obsolete and desupport it in pg_dump, while continuing
to allow it for awhile in pg_restore for compatibility reasons.
A closer-to-home possibility is that the work to allow non-text
output from pg_dumpall will result in a format that pg_restore
can read but pg_dump (by itself) doesn't write.So I'd just scrap pg_restore's parsing logic for this and replace it
in-place. To the extent that that's copying and pasting stuff, fine.
It's not like there's no other duplicativeness in their switch-parsing
logic.
Fair point. Agreed.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On Fri, Jan 24, 2025 at 8:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
I don't think we need a new file for this. pg_backup_utils.c is already
there for routines common to pg_restore and pg_dump.I'm not even on board with having a new function, because I doubt
we should try to share this code in the first place. Who's to
say that pg_dump and pg_restore must support exactly the same list
of formats? For example, in the future we might decide that some
format is obsolete and desupport it in pg_dump, while continuing
to allow it for awhile in pg_restore for compatibility reasons.
A closer-to-home possibility is that the work to allow non-text
output from pg_dumpall will result in a format that pg_restore
can read but pg_dump (by itself) doesn't write.So I'd just scrap pg_restore's parsing logic for this and replace it
in-place. To the extent that that's copying and pasting stuff, fine.
It's not like there's no other duplicativeness in their switch-parsing
logic.regards, tom lane
Agreed and made the patch as suggested .
Regards,
Srinath Reddy Sadipiralla,
EDB:http://www.enterprisedb.com
Attachments:
0001-Parse-the-format-option-for-pg_restore-with-pg_strcasecmp.patchapplication/octet-stream; name=0001-Parse-the-format-option-for-pg_restore-with-pg_strcasecmp.patchDownload
From eb632649b49b8c48593294875d823c0077d06d78 Mon Sep 17 00:00:00 2001
From: Srinath Reddy Sadipiralla <srinath2133@gmail.com>
Date: Sat, 25 Jan 2025 17:52:50 +0530
Subject: [PATCH 1/1] Parse the --format option for pg_restore with
pg_strcasecmp instead of first char of the formatName.
---
src/bin/pg_dump/pg_restore.c | 34 ++++++++++++++--------------------
1 file changed, 14 insertions(+), 20 deletions(-)
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 88ae39d938..79f046163c 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -383,27 +383,21 @@ main(int argc, char **argv)
if (opts->formatName)
{
- switch (opts->formatName[0])
- {
- case 'c':
- case 'C':
- opts->format = archCustom;
- break;
-
- case 'd':
- case 'D':
- opts->format = archDirectory;
- break;
-
- case 't':
- case 'T':
- opts->format = archTar;
- break;
-
- default:
- pg_fatal("unrecognized archive format \"%s\"; please specify \"c\", \"d\", or \"t\"",
+ if (pg_strcasecmp(opts->formatName, "c") == 0)
+ opts->format = archCustom;
+ else if (pg_strcasecmp(opts->formatName, "custom") == 0)
+ opts->format = archCustom;
+ else if (pg_strcasecmp(opts->formatName, "d") == 0)
+ opts->format = archDirectory;
+ else if (pg_strcasecmp(opts->formatName, "directory") == 0)
+ opts->format = archDirectory;
+ else if (pg_strcasecmp(opts->formatName, "t") == 0)
+ opts->format = archTar;
+ else if (pg_strcasecmp(opts->formatName, "tar") == 0)
+ opts->format = archTar;
+ else
+ pg_fatal("unrecognized archive format \"%s\"; please specify \"c\", \"d\", or \"t\"",
opts->formatName);
- }
}
AH = OpenArchive(inputFileSpec, opts->format);
--
2.43.0
./pg_dump postgres -Fp -f textdump
./pg_restore -Fp -d postgres textdump
pg_restore: error: unrecognized archive format "p"; please specify "c",
"d", or "t"
./pg_restore -d postgres textdump
pg_restore: error: input file appears to be a text format dump. Please use
psql.
wouldn't it be nice if we get the error message as same as when we didn't
gave format option and pg_restore finds the format
using _discoverArchiveFormat there it suggests to try psql ,instead of
saying "p/plain text" format is a invalid format,as we are doing currently
in pg.
thoughts?
Regards,
Srinath Reddy Sadipiralla,
EDB:http://www.enterprisedb.com
On 2025-01-25 Sa 8:12 AM, Srinath Reddy wrote:
./pg_dump postgres -Fp -f textdump
./pg_restore -Fp -d postgres textdump
pg_restore: error: unrecognized archive format "p"; please specify
"c", "d", or "t"./pg_restore -d postgres textdump
pg_restore: error: input file appears to be a text format dump. Please
use psql.wouldn't it be nice if we get the error message as same as when we
didn't gave format option and pg_restore finds the format
using _discoverArchiveFormat there it suggests to try psql ,instead of
saying "p/plain text" format is a invalid format,as we are doing
currently in pg.thoughts?
Probably not exactly the same, because in one case you're explicitly
saying it's a text dump. But maybe something closer would work, like
"text format not supported. Please use psql."
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Srinath Reddy <srinath2133@gmail.com> writes:
wouldn't it be nice if we get the error message as same as when we didn't
gave format option and pg_restore finds the format
using _discoverArchiveFormat there it suggests to try psql ,instead of
saying "p/plain text" format is a invalid format,as we are doing currently
in pg.
Good idea. I pushed your patch with that addition.
regards, tom lane
Thanks Tom and Andrew.
Regards,
Srinath Reddy Sadipiralla,
EDB: http://www.enterprisedb.com
On Sat, Jan 25, 2025 at 9:55 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Show quoted text
Srinath Reddy <srinath2133@gmail.com> writes:
wouldn't it be nice if we get the error message as same as when we didn't
gave format option and pg_restore finds the format
using _discoverArchiveFormat there it suggests to try psql ,instead of
saying "p/plain text" format is a invalid format,as we are doingcurrently
in pg.
Good idea. I pushed your patch with that addition.
regards, tom lane