Date: Wed, 20 Sep 2023 13:29:22 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Pierre Ducroquet <p.psql@pinaraf.info>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Improvements in pg_dump/pg_restore toc format and performances
Message-ID: <20230920202922.GA3054233@nathanxps13>
References: <2656000.KRxA6XjA2N@peanuts2>
 <20230918215247.GA2661288@nathanxps13>
 <20230918215442.GB2661288@nathanxps13>
 <2575263.Lt9SDvczpP@peanuts2>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2575263.Lt9SDvczpP@peanuts2>
Archived-At: <https://www.postgresql.org/message-id/20230920202922.GA3054233%40nathanxps13>
Precedence: bulk

On Tue, Sep 19, 2023 at 12:15:55PM +0200, Pierre Ducroquet wrote:
> Attached updated patches fix this regression, I'm sorry I missed that.

Thanks for the new patches.  0001 and 0002 look reasonable to me.  This is
a nitpick, but we might want to use atooid() in 0002, which is just
shorthand for the strtoul() call you are using.

> -		WriteStr(AH, NULL);		/* Terminate List */
> +		WriteInt(AH, -1);		/* Terminate List */

I think we need to be cautious about using WriteInt and ReadInt here.  OIDs
are unsigned, so we probably want to use InvalidOid (0) instead.

> +				if (AH->version >= K_VERS_1_16)
>  				{
> -					depSize *= 2;
> -					deps = (DumpId *) pg_realloc(deps, sizeof(DumpId) * depSize);
> +					depId = ReadInt(AH);
> +					if (depId == -1)
> +						break;		/* end of list */
> +					if (depIdx >= depSize)
> +					{
> +						depSize *= 2;
> +						deps = (DumpId *) pg_realloc(deps, sizeof(DumpId) * depSize);
> +					}
> +					deps[depIdx] = depId;
> +				}
> +				else
> +				{
> +					tmp = ReadStr(AH);
> +					if (!tmp)
> +						break;		/* end of list */
> +					if (depIdx >= depSize)
> +					{
> +						depSize *= 2;
> +						deps = (DumpId *) pg_realloc(deps, sizeof(DumpId) * depSize);
> +					}
> +					deps[depIdx] = strtoul(tmp, NULL, 10);
> +					free(tmp);
>  				}

I would suggest making the resizing logic common.

	if (AH->version >= K_VERS_1_16)
	{
		depId = ReadInt(AH);
		if (depId == InvalidOid)
			break;		/* end of list */
	}
	else
	{
		tmp = ReadStr(AH);
		if (!tmp)
			break;		/* end of list */
		depId = strtoul(tmp, NULL, 10);
		free(tmp);
	}

	if (depIdx >= depSize)
	{
		depSize *= 2;
		deps = (DumpId *) pg_realloc(deps, sizeof(DumpId) * depSize);
	}
	deps[depIdx] = depId;

Also, can we make depId more locally scoped?

I have yet to look into 0004 still.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com