Re: Non-text mode for pg_dumpall

On 2025-03-29 Sa 1:17 AM, Mahendra Singh Thalor wrote:
> On Sat, 29 Mar 2025 at 03:50, Andrew Dunstan <andrew(at)dunslane(dot)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(at)dunslane(dot)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(at)alvh(dot)no-ip(dot)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(at)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