Re: Non-text mode for pg_dumpall

On 2025-07-21 Mo 8:53 PM, Noah Misch wrote:
> On Mon, Jul 21, 2025 at 04:41:03PM -0400, Andrew Dunstan wrote:
>> On 2025-07-17 Th 6:18 AM, Mahendra Singh Thalor wrote
>>>>>>>> --- a/src/bin/pg_dump/pg_restore.c
>>>>>>>> +++ b/src/bin/pg_dump/pg_restore.c
>>>>>>>> +/*
>>>>>>>> + * read_one_statement
>>>>>>>> + *
>>>>>>>> + * This will start reading from passed file pointer using fgetc and read till
>>>>>>>> + * semicolon(sql statement terminator for global.dat file)
>>>>>>>> + *
>>>>>>>> + * EOF is returned if end-of-file input is seen; time to shut down.
>>>>>>> What makes it okay to use this particular subset of SQL lexing?
>>>>>> To support complex syntax, we used this code from another file.
>>>>> I'm hearing that you copied this code from somewhere. Running
>>>>> "git grep 'time to shut down'" suggests you copied it from
>>>>> InteractiveBackend(). Is that right? I do see other similarities between
>>>>> read_one_statement() and InteractiveBackend().
>>>>>
>>>>> Copying InteractiveBackend() provides negligible assurance that this is the
>>>>> right subset of SQL lexing. Only single-user mode uses InteractiveBackend().
>>>>> Single-user mode survives mostly as a last resort for recovering from having
>>>>> reached xidStopLimit, is rarely used, and only superusers write queries to it.
>>>> Yes, we copied this from InteractiveBackend to read statements from
>>>> global.dat file.
>> Maybe we should ensure that identifiers with CR or LF are turned into
>> Unicode quoted identifiers, so each SQL statement would always only occupy
>> one line.
> Interesting. That might work.
>
>> Or just reject role and tablespace names with CR or LF altogether,
>> just as we do for database names.
> There are other ways to get multi-line statements. Non-exhaustive list:
>
> - pg_db_role_setting.setconfig
> - pg_shdescription.description
> - pg_shseclabel.label
> - pg_tablespace.spcoptions (if we add a text option in the future)
>
> I think this decision about lexing also ties to other unfinished open item
> work of aligning "pg_dumpall -Fd;pg_restore [options]" behavior with "pg_dump
> -Fd;pg_restore [options]". "pg_restore --no-privileges" should not restore
> pg_tablespace.spcacl, and "pg_restore --no-comments" should not emit COMMENT
> statements.
>
> I suspect this is going to end with a structured dump like we use on the
> pg_dump (per-database) side. It's not an accident that v17 pg_restore doesn't
> lex text files to do its job. pg_dumpall deals with a more-limited set of
> statements than pg_dump deals with, but they're not _that much_ more limited.
> I won't veto a lexing-based approach if it gets the behaviors right, but I
> don't have high hopes for it getting the behaviors right and staying that way.

Yeah, that was my original idea. But maybe instead of extending the
archive mechanism, we could do something more lightweight, e.g. output
the statements as a JSON array.

cheers

andrew

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