Re: Non-text mode for pg_dumpall

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.


cheers


andrew

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