MIME-Version: 1.0
References: <2656000.KRxA6XjA2N@peanuts2> <20230918215247.GA2661288@nathanxps13>
 <20230918215442.GB2661288@nathanxps13> <2575263.Lt9SDvczpP@peanuts2>
In-Reply-To: <2575263.Lt9SDvczpP@peanuts2>
From: vignesh C <vignesh21@gmail.com>
Date: Tue, 3 Oct 2023 15:17:57 +0530
Message-ID: <CALDaNm0eaZ8Ao28QHdFTm+wRK3V5oGuti20u89+cmmkX_g1pBA@mail.gmail.com>
Subject: Re: Improvements in pg_dump/pg_restore toc format and performances
To: Pierre Ducroquet <p.psql@pinaraf.info>
Cc: Nathan Bossart <nathandbossart@gmail.com>, pgsql-hackers@lists.postgresql.org
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CALDaNm0eaZ8Ao28QHdFTm%2BwRK3V5oGuti20u89%2BcmmkX_g1pBA%40mail.gmail.com>
Precedence: bulk

On Tue, 19 Sept 2023 at 15:46, Pierre Ducroquet <p.psql@pinaraf.info> wrote:
>
> On Monday, September 18, 2023 11:54:42 PM CEST Nathan Bossart wrote:
> > On Mon, Sep 18, 2023 at 02:52:47PM -0700, Nathan Bossart wrote:
> > > On Thu, Jul 27, 2023 at 10:51:11AM +0200, Pierre Ducroquet wrote:
> > >> I ended up writing several patches that shaved some time for pg_restore
> > >> -l,
> > >> and reduced the toc.dat size.
> > >
> > > I've only just started taking a look at these patches, and I intend to do
> > > a
> > > more thorough review in the hopefully-not-too-distant future.
> >
> > Since cfbot is failing on some pg_upgrade and pg_dump tests, I've set this
> > to waiting-on-author.
>
> Attached updated patches fix this regression, I'm sorry I missed that.

Few comments:
1) These printf statements are not required:
+       /* write the list of am */
+       printf("%d tableams to save\n", tableam_count);
+       WriteInt(AH, tableam_count);
+       for (i = 0 ; i < tableam_count ; i++)
+       {
+               printf("%d is %s\n", i, tableams[i]);
+               WriteStr(AH, tableams[i]);
+       }
+
+       /* write the list of namespaces */
+       printf("%d namespaces to save\n", namespace_count);
+       WriteInt(AH, namespace_count);
+       for (i = 0 ; i < namespace_count ; i++)
+       {
+               printf("%d is %s\n", i, namespaces[i]);
+               WriteStr(AH, namespaces[i]);
+       }
+
+       /* write the list of owners */
+       printf("%d owners to save\n", owner_count);

2) We generally use pg_malloc in client tools, we should change palloc
to pg_malloc:
+       /* prepare dynamic arrays */
+       tableams = palloc(sizeof(char*) * 1);
+       tableams[0] = NULL;
+       tableam_count = 0;
+       namespaces = palloc(sizeof(char*) * 1);
+       namespaces[0] = NULL;
+       namespace_count = 0;
+       owners = palloc(sizeof(char*) * 1);
+       owners[0] = NULL;
+       owner_count = 0;
+       tablespaces = palloc(sizeof(char*) * 1);

3) This similar code is repeated few times, will it be possible to do
it in a common function:
+                       if (namespace_count < 128)
+                       {
+                               te->namespace_idx = AH->ReadBytePtr(AH);
+                               invalid_entry = 255;
+                       }
+                       else
+                       {
+                               te->namespace_idx = ReadInt(AH);
+                               invalid_entry = -1;
+                       }
+                       if (te->namespace_idx == invalid_entry)
+                               te->namespace = "";
+                       else
+                               te->namespace = namespaces[te->namespace_idx];

4) Can the initialization of tableam_count, namespace_count,
owner_count and tablespace_count be done at declaration and these
initialization code can be removed:
+       /* prepare dynamic arrays */
+       tableams = palloc(sizeof(char*) * 1);
+       tableams[0] = NULL;
+       tableam_count = 0;
+       namespaces = palloc(sizeof(char*) * 1);
+       namespaces[0] = NULL;
+       namespace_count = 0;
+       owners = palloc(sizeof(char*) * 1);
+       owners[0] = NULL;
+       owner_count = 0;
+       tablespaces = palloc(sizeof(char*) * 1);
+       tablespaces[0] = NULL;
+       tablespace_count = 0;

4) There are some whitespace issues in the patch:
Applying: move static strings to arrays at beginning
.git/rebase-apply/patch:24: trailing whitespace.
.git/rebase-apply/patch:128: trailing whitespace.
.git/rebase-apply/patch:226: trailing whitespace.
.git/rebase-apply/patch:232: trailing whitespace.
warning: 4 lines add whitespace errors.

Regards,
Vignesh