Re: pgsql: Non text modes for pg_dumpall, correspondingly change pg_restore - Mailing list pgsql-hackers

On Wed, 16 Apr 2025 at 01:52, Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
> On 2025-Apr-15, Mahendra Singh Thalor wrote:
>
> > I took this patch and did some testing. Patch looks good to me.
>
> Hello, thank you very much for looking.
>
> > I was not able to use "git am" in my setup due to CR's in patch but no
> > warning in the patch when I used "patch -p".
>
> Hmm, don't blame me; rather I think your email rig is broken and
> produces CRs for no reason (pretty normal on Windows I think).  Here's
> what hexdump shows for the attached file direct from the email:

Sorry for the noise. My mistake, I will fix my git setup.

Actually "git am" was working but "git apply" was failing. I think my
git setup is missing something.

I am downloading on a Mac and I am doing testing in a VM with Centos.

Below are the outputs of git commands.

[mst@localhost postgres]$ git apply
0001-remove-unnecessary-oid_string-list-stuff.patch
error: patch failed: src/bin/pg_dump/pg_restore.c:67
error: src/bin/pg_dump/pg_restore.c: patch does not apply
error: patch failed: src/fe_utils/simple_list.c:192
error: src/fe_utils/simple_list.c: patch does not apply
error: patch failed: src/include/fe_utils/simple_list.h:55
error: src/include/fe_utils/simple_list.h: patch does not apply
error: patch failed: src/tools/pgindent/typedefs.list:615
error: src/tools/pgindent/typedefs.list: patch does not apply
[mst@localhost postgres]$

[mst@localhost postgres]$ git am
0001-remove-unnecessary-oid_string-list-stuff.patch
Applying: remove unnecessary oid_string list stuff
[mst@localhost postgres]$

[mst@localhost postgres]$ git reset --hard HEAD~1
HEAD is now at 3b35f9a4c5e Fix an incorrect check in get_memoize_path
[mst@localhost postgres]$
[mst@localhost postgres]$ patch -p 1 <
0001-remove-unnecessary-oid_string-list-stuff.patch
(Stripping trailing CRs from patch; use --binary to disable.)
patching file src/bin/pg_dump/pg_restore.c
(Stripping trailing CRs from patch; use --binary to disable.)
patching file src/fe_utils/simple_list.c
(Stripping trailing CRs from patch; use --binary to disable.)
patching file src/include/fe_utils/simple_list.h
(Stripping trailing CRs from patch; use --binary to disable.)
patching file src/tools/pgindent/typedefs.list

>
> 000002a0  2d 2d 2d 20 61 2f 73 72  63 2f 62 69 6e 2f 70 67  |--- a/src/bin/pg|
> 000002b0  5f 64 75 6d 70 2f 70 67  5f 72 65 73 74 6f 72 65  |_dump/pg_restore|
> 000002c0  2e 63 0a 2b 2b 2b 20 62  2f 73 72 63 2f 62 69 6e  |.c.+++ b/src/bin|
> 000002d0  2f 70 67 5f 64 75 6d 70  2f 70 67 5f 72 65 73 74  |/pg_dump/pg_rest|
> 000002e0  6f 72 65 2e 63 0a 40 40  20 2d 36 37 2c 31 30 20  |ore.c.@@ -67,10 |
> 000002f0  2b 36 37 2c 32 30 20 40  40 20 73 74 61 74 69 63  |+67,20 @@ static|
> 00000300  20 69 6e 74 09 70 72 6f  63 65 73 73 5f 67 6c 6f  | int.process_glo|
> 00000310  62 61 6c 5f 73 71 6c 5f  63 6f 6d 6d 61 6e 64 73  |bal_sql_commands|
> 00000320  28 50 47 63 6f 6e 6e 20  2a 63 6f 6e 6e 2c 20 63  |(PGconn *conn, c|
> 00000330  6f 6e 73 74 20 63 68 61  72 20 2a 64 75 6d 70 64  |onst char *dumpd|
> 00000340  69 72 70 61 74 68 2c 0a  20 09 09 09 09 09 09 09  |irpath,. .......|
> 00000350  09 09 09 63 6f 6e 73 74  20 63 68 61 72 20 2a 6f  |...const char *o|
> 00000360  75 74 66 69 6c 65 29 3b  0a 20 73 74 61 74 69 63  |utfile);. static|
>
> In bytes 340ff you can see that the comma (2c) is followed by 0a ("line
> feed" or newline) and then a bunch of tabs (09).  There's no carriage
> return (0d) anywhere.
>
> Maybe you would be able to convince git not to complain by downloading
> the file from the email[1] directly onto it somehow, without letting
> Windows mess things up.  Maybe something like this on a command line
>   curl "https://www.postgresql.org/message-id/attachment/175873/0001-remove-unnecessary-oid_string-list-stuff.patch"
|git am - 
> assuming you can get curl to run on a command line at all.
>
> [1] https://postgr.es/m/202504141220.343fmoxfsbj4@alvherre.pgsql
>
> > *One review comment:*
> > We are using FLEXIBLE_ARRAY_MEMBER for dbname. Can we use NAMEDATALEN? In
> > code, many places we are using this hard coded value of 64 size for names.
>
> Well, yes we can, but then we'd be allocating a bunch of null bytes for
> no reason.  The current allocation strategy, which comes from the
> original commit not my code, is to allocate a struct with the OID and
> just enough bytes to store the database name, which is known.  I don't
> see any reason to modify this tbh.  The reason to use NAMEDATALEN in
> various places is to have enough room to store whatever name the user
> specifies, without having to know the length before the allocation
> occurs.

Thanks for feedback. Yes, FLEXIBLE_ARRAY_MEMBER is added in the original commit.

In other code parts also, we can use FLEXIBLE_ARRAY_MEMBER instead
NAMEDATALEN. I will try if this is possible to do in other code parts.

>
> FLEXIBLE_ARRAY_MEMBER is there to document exactly what's happening
> here, and used thoroughly across the codebase.
>
> --
> Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
> "E pur si muove" (Galileo Galilei)

-
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com