Horton

Thread

Commits

  1. Tighten pg_restore's recognition of its -F (format) option values.

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

    Srinath Reddy Sadipiralla <srinath2133@gmail.com> — 2025-01-23T08:18:56Z

    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

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

    Andrew Dunstan <andrew@dunslane.net> — 2025-01-23T20:12:22Z

    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

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

    Srinath Reddy Sadipiralla <srinath2133@gmail.com> — 2025-01-24T08:04:19Z

    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

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

    Andrew Dunstan <andrew@dunslane.net> — 2025-01-24T14:14:59Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-24T15:24:06Z

    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

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

    Andrew Dunstan <andrew@dunslane.net> — 2025-01-24T19:47:01Z

    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

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

    Srinath Reddy Sadipiralla <srinath2133@gmail.com> — 2025-01-25T12:42:44Z

    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

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

    Srinath Reddy Sadipiralla <srinath2133@gmail.com> — 2025-01-25T13:12:06Z

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

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

    Andrew Dunstan <andrew@dunslane.net> — 2025-01-25T16:18:35Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-25T16:25:01Z

    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

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

    Srinath Reddy Sadipiralla <srinath2133@gmail.com> — 2025-01-25T16:51:42Z

    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:

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