Date: Tue, 1 Apr 2025 22:21:44 -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-ytSGUZAQNIM9GJ@nathan>
References: 
 <CADkLM=f5Dy3RtGOq=2RH2z4xLXNTwSYCAu-d5ggzQFrb8nCsyg@mail.gmail.com>
 <CADkLM=ftC94muGKH+z1irdBUXO2+tmBMLdqDAAxcct=H+LE1Eg@mail.gmail.com>
 <CADkLM=desCuf3dVHasADvdUVRmb-5gO0mhMO5u9nzgv6i7U86Q@mail.gmail.com>
 <f248676c32cbb3a4dd0da13bc84af43760239bf2.camel@j-davis.com>
 <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>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="nfxxzXxr01r2a0nb"
Content-Disposition: inline
In-Reply-To: <88af35fe0ff24cc4e0700b841aa60a0865f11648.camel@j-davis.com>
Archived-At: <https://www.postgresql.org/message-id/Z-ytSGUZAQNIM9GJ%40nathan>
Precedence: bulk


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

On Tue, Apr 01, 2025 at 03:05:59PM -0700, Jeff Davis wrote:
> To restate the problem: one of the problems being solved here is that
> the existing code for custom-format dumps calls WriteToc twice. That
> was not a big problem before this patch, when the contents of the
> entries was easily accessible in memory. But the point of 0002 is to
> avoid keeping all of the stats in memory at once, because that causes
> bloat; and instead to query it on demand.
> 
> In theory, we could fix the pre-existing code by making the second pass
> able to jump over the other contents of the entry and just update the
> data offsets. But that seems invasive, at least to do it properly.
> 
> 0001 sidesteps the problem by skipping the second pass if data's not
> being dumped (because there are no offsets that need updating). The
> worst case is when there are a lot of objects with a small amount of
> data. But that's a worst case for stats in general, so I don't think
> that needs to be solved here.
> 
> Issuing the stats queries twice is not great, though. If there's any
> non-deterministic output in the query, that could lead to strangeness.
> How bad can that be? If the results change in some way that looks
> benign, but changes the length of the definition string, can it lead to
> corruption of a ToC entry? I'm not saying there's a problem, but trying
> to understand the risk of future problems.

It certainly feels risky.  I was able to avoid executing the queries twice
in all cases by saving the definition length in the TOC entry and skipping
that many bytes the second time round.  That's simple enough, but it relies
on various assumptions such as fseeko() being available (IIUC the file will
only be open for writing so we cannot fall back on fread()) and WriteStr()
returning an accurate value (which I'm skeptical of because some formats
compress this data).  But AFAICT custom format is the only format that does
a second WriteToc() pass at the moment, and it only does so when fseeko()
is usable.  Plus, custom format doesn't appear to compress anything written
via WriteStr().

We might be able to improve this by inventing a new callback that fails for
all formats except for custom with feesko() available.  That would at least
ensure hard failures if these assumptions change.  That problably wouldn't
be terribly invasive.  I'm curious what you think.

> For 0003, it makes an assumption about the way the scan happens in
> WriteToc(). Can you add some additional sanity checks to verify that
> something doesn't happen in a different order than we expect?

Hm.  One thing we could do is to send the TocEntry to the callback and
verify that matches the one we were expecting to see next (as set by a
previous call).  Does that sound like a strong enough check?  FWIW the
pg_dump tests failed miserably until Corey and I got this part right, so
our usual tests should also offer some assurance.

> Also, why do we need the clause "WHERE s.tablename = ANY($2)"? Isn't
> that already implied by "JOIN unnest($1, $2) ... s.tablename =
> u.tablename"?

Good question.  Corey, do you recall why this was needed?

-- 
nathan

--nfxxzXxr01r2a0nb
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename="v12n4-0001-Skip-second-WriteToc-for-custom-format-dumps-w.patch"