Re: Non-text mode for pg_dumpall - Mailing list pgsql-hackers

On 2025-03-30 Su 12:50 PM, Andrew Dunstan wrote:
>
> On 2025-03-29 Sa 1:17 AM, Mahendra Singh Thalor wrote:
>> On Sat, 29 Mar 2025 at 03:50, Andrew Dunstan <andrew@dunslane.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@dunslane.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@alvh.no-ip.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@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.
>
>
>



I have reworked the documentation some. See attached.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com