Subject: Re: Statistics Import and Export

From: Jeff Davis <[email-masked]>
Date: Tue, 01 Apr 2025 15:05:59 -0700

On Tue, 2025-04-01 at 13:44 -0500, Nathan Bossart wrote:
> Apologies for the noise.  I noticed one more way to simplify 0002. 
> As
> before, there should be no functional differences.

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.

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?

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"?

Regards,
	Jeff Davis

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 1 Apr 2025 22:21:44 -0500

> 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.

> 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.

> that already implied by "JOIN unnest($1, $2) ... s.tablename =

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

-- 
nathan

---

From: Jeff Davis <[email-masked]>
Date: Tue, 01 Apr 2025 22:44:19 -0700

On Tue, 2025-04-01 at 22:21 -0500, Nathan Bossart wrote:
> that many bytes the second time round.

That feels like a better approach.

>   That's simple enough, but it relies
> is usable.

Even with those assumptions, I think it's much better than querying
twice and assuming that the results are the same.

>   Plus, custom format doesn't appear to compress anything written
> via WriteStr().

If WriteStr() was doing compression, that would make the second
WriteToc() pass to update the data offsets scary even in the existing
code.

> be terribly invasive.  I'm curious what you think.

That sounds fine, I'd say do that if it feels reasonable, and if the
extra callbacks get too messy, we can just document the assumptions
instead.

> previous call).  Does that sound like a strong enough check?

Again, I'd just be practical here and do the check if it feels natural,
and if not, improve the comments so that someone modifying the code
would know where to look.

Regards,
	Jeff Davis

---

From: Andres Freund <[email-masked]>
Date: Wed, 2 Apr 2025 12:42:01 -0400

Hi,

https://commitfest.postgresql.org/patch/4538/ is still in "needs review", even
though the feature really has been committed.  Is that intention, e.g. to
track pending changes that we're planning to make?

Greetings,

Andres

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 2 Apr 2025 21:26:32 -0500

> extra callbacks get too messy, we can just document the assumptions

I did write a version with callbacks, but it felt a bit silly because it is
very obviously intended for this one case.  So, I removed them in the
attached patch set.

> would know where to look.

Okay, here is an updated patch set.  I did add some verification code,
which ended up being a really good idea because it revealed a couple of
cases we weren't handling:

* Besides custom format calling WriteToc() twice to update the data
  offsets, tar format calls WriteToc() followed by RestoreArchive() to
  write restore.sql.  I couldn't think of a great way to avoid executing
  the queries twice in this case, so I settled on allowing it for only that
  mode.  While we don't expect the second set of queries to result in
  different stats definitions, even if it did, the worst case is that the
  content of restore.sql (which isn't used by pg_restore) would be
  different.  I noticed some past discussion that seems to suggest that
  this format might be a candidate for deprecation [0], so I'm not sure
  it's worth doing anything fancier.

* Our batching code assumes that stats entries are dumped in TOC order,
  which unfortunately wasn't true for formats that use RestoreArchive() for
  dumping.  This is because RestoreArchive() does multiple passes through
  the TOC and selectively dumps certain entries each time.  This is
  particularly troublesome for index stats and a subset of matview stats;
  both are in SECTION_POST_DATA, but matview stats that depend on matview
  data are dumped in RESTORE_PASS_POST_ACL, while all other stats data is
  dumped in RESTORE_PASS_MAIN.  To deal with this, I propose moving all
  stats entries in SECTION_POST_DATA to RESTORE_PASS_POST_ACL, which
  ensures that we always dump stats in TOC order.  One convenient side
  effect of this change is that we can revert a decent chunk of commit
  a0a4601765.  It might be possible to do better via smarter lookahead code
  or a more sophisticated cache, but it's a bit late in the game for that.

[0] https://postgr.es/m/20180727015306.fzlo4inv5i3zqr2c%40alap3.anarazel.de

-- 
nathan