Re: Temporary tables versus wraparound... again

On Sat, 5 Nov 2022 at 11:34, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Greg Stark <stark(at)mit(dot)edu> writes:
> > Simple Rebase
>
> I took a little bit of a look through these.
>
> * I find 0001 a bit scary, specifically that it's decided it's
> okay to apply extract_autovac_opts, pgstat_fetch_stat_tabentry_ext,
> and especially relation_needs_vacanalyze to another session's
> temp table. How safe is that really?

I can look a bit more closely but none of them are doing any thing
with the table itself, just the catalog entries which afaik have
always been fair game for other sessions. So I'm not really clear what
kind of unsafeness you're asking about.

> * Don't see much point in renaming checkTempNamespaceStatus.
> That doesn't make it not an ABI break. If we want to back-patch
> this we'll have to do something different than what you did here.

Well it's an ABI break but at least it's an ABI break that gives a
build-time error or shared library loading error rather than one that
just crashes or writes to random memory at runtime.

> * In 0002, I don't especially approve of what you've done with
> the relminmxid calculation --- that seems to move it out of
> "pure bug fix" territory and into "hmm, I wonder if this
> creates new bugs" territory.

Hm. Ok, I can separate that into a separate patch. I admit I have a
lot of trouble remembering how multixactids work.

> Also, skipping that update
> for non-temp tables immediately falsifies ResetVacStats'
> claimed charter of "resetting to the same values used when
> creating tables". Surely GetOldestMultiXactId isn't *that*
> expensive, especially compared to the costs of file truncation.
> I think you should just do GetOldestMultiXactId straight up,
> and maybe submit a separate performance-improvement patch
> to make it do the other thing (in both places) for temp tables.

Hm. the feedback I got earlier was that it was quite expensive. That
said, I think the concern was about the temp tables where the truncate
was happening on every transaction commit when they're used. For
regular truncates I'm not sure how important optimizing it is.

> * I wonder if this is the best place for ResetVacStats --- it
> doesn't seem to be very close to the code it needs to stay in
> sync with. If there's no better place, perhaps adding cross-
> reference comments in both directions would be advisable.

I'll look at that. I think relfrozenxid and relfrozenmxid are touched
in a lot of places so it may be tilting at windmills to try to
centralize the code working with them at this point...

> * 0003 says it's running temp.sql by itself to avoid interference
> from other sessions, but sadly that cannot avoid interference
> from background autovacuum/autoanalyze. I seriously doubt this
> patch would survive contact with the buildfarm. Do we actually
> need a new test case? It's not like the code won't get exercised
> without it --- we have plenty of temp table truncations, surely.

No I don't think we do. I kept it in a separate commit so it could be
dropped when committing.

But just having truncate working isn't really good enough either. An
early version of the patch had a bug that meant it didn't run at all
so truncate worked fine but relfrozenxid never got reset.

In thinking about whether we could have a basic test that temp tables
are getting reset at all it occurs to me that there's still a gap
here:

You can have a session attached to a temp namespace that never
actually uses the temp tables. That would prevent autovacuum from
dropping them and still never reset their vacuum stats. :( Offhand I
think PreCommit_on_commit_actions() could occasionally truncate all ON
COMMIT TRUNCATE tables even if they haven't been touched in this
transaction.

--
greg