Date: Thu, 3 Apr 2025 21:19:51 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Jeff Davis <pgsql@j-davis.com>
Cc: Corey Huinker <corey.huinker@gmail.com>, Robert Treat <rob@xzilla.net>,
	Robert Haas <robertmhaas@gmail.com>,
	Andres Freund <andres@anarazel.de>, Tom Lane <tgl@sss.pgh.pa.us>,
	Michael Paquier <michael@paquier.xyz>,
	jian he <jian.universality@gmail.com>,
	Bruce Momjian <bruce@momjian.us>,
	Matthias van de Meent <boekewurm+postgres@gmail.com>,
	Magnus Hagander <magnus@hagander.net>,
	Stephen Frost <sfrost@snowman.net>,
	Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>,
	Peter Smith <smithpb2250@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	alvherre@alvh.no-ip.org
Subject: Re: Statistics Import and Export
Message-ID: <Z-9Bx3ml2i7OfHiN@nathan>
References: 
 <CADkLM=dG+8jW4UAwP69o7Q92FOXpoN8Pb9kD_Fck1_BBn+gwgQ@mail.gmail.com>
 <CADkLM=domd5+CvjKMHGbOfvSuY6J8G-x+9M2D6Ss2HamYefE9w@mail.gmail.com>
 <Z-tQa5zsVkcCyYin@nathan>
 <Z-wubg4vnItKnOCj@nathan>
 <Z-wz_sZdceGJ-fuI@nathan>
 <88af35fe0ff24cc4e0700b841aa60a0865f11648.camel@j-davis.com>
 <Z-ytSGUZAQNIM9GJ@nathan>
 <ec43b87247fd8700c6fcf4fe0b68cdb2fafecf2d.camel@j-davis.com>
 <Z-3x2AnPCP331JA3@nathan>
 <8a6905c658ed52418c6f413393390dc43d63a995.camel@j-davis.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="KIyZLC1cNgqfVArR"
Content-Disposition: inline
In-Reply-To: <8a6905c658ed52418c6f413393390dc43d63a995.camel@j-davis.com>
Archived-At: <https://www.postgresql.org/message-id/Z-9Bx3ml2i7OfHiN%40nathan>
Precedence: bulk


--KIyZLC1cNgqfVArR
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Thanks for reviewing.

On Thu, Apr 03, 2025 at 03:23:40PM -0700, Jeff Davis wrote:
> This simplifies commit a0a4601765. I'd break out that simplification as
> a separate commit to make it easier to understand what happened. 

Done.

> In patch 0003, there are quite a few static function-scoped variables,
> which is not a style that I'm used to. One idea is to bundle them into
> a struct representing the cache state (including enough information to
> fetch the next batch), and have a single static variable that points to
> that.

As discussed off-list, I didn't take this suggestion for now.  Corey did
this originally, and I converted it to static function-scoped variables 1)
to reduce patch size and 2) because I noticed that each of the state
variables were only needed in one function.  I agree that a struct might be
slightly more readable, but we can always change this in the future if
desired.

> Also in 0003, the "next_te" variable is a bit confusing, because it's
> actually the last TocEntry, until it's advanced to point to the current
> one.

I've renamed it to expected_te.

> Other than that, looks good to me.

Great.  I'm planning to commit the attached patch set tomorrow morning.

For the record, I spent most of today trying very hard to fix the layering
violations in 0002.  While I was successful, the result was awkward,
complicated, and nigh unreadable.  This is now the second time I've
attempted to fix this and have felt the result was worse than where I
started.  So, I added extremely descriptive comments instead.  I'm hoping
that it will be possible to clean this up with some additional work in v19.
I have a few ideas, but if anyone has suggestions, I'm all ears.

-- 
nathan

--KIyZLC1cNgqfVArR
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename="v12n6-0001-Skip-second-WriteToc-call-for-custom-format-du.patch"