why -Fdance archive format option works with ./pg_restore but not with ./pg_dump?

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

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

./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: