Re: [GENERAL] pgstattuple triggered checkpoint failure and database outage?

I wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>> (We already have rel->rd_istemp, but it's not what we need here.)

> Yeah. I was considering converting that into a three-state flag, but
> it might be simpler to remove it altogether and look to the new pg_class
> field; only after we've gone down the path into localbuf.c would we
> check relnamespace == our temp namespace before permitting a read or write.

I looked at the uses of this flag a bit further. It seems that
rd_istemp is being used with three subtly different meanings:

* do we need to WAL-log operations on this relation
* do we need to fsync this relation during checkpoint
* do we use shared or local buffers for this relation

I thought briefly about trying to make these shades of meaning explicit,
eg by introducing macros like RelationUsesWal(rel). It doesn't seem
quite worth the trouble though. There would be a lot of places to
change, and if we wanted to take it completely seriously, we'd have to
pass more than one argument to, for example, smgrtruncate() (when it
passes isTemp to DropRelFileNodeBuffers, the bufmgr is wanting to know
about the buffer classification; but mdtruncate wants to know about
fsync behavior).

For the buffer-classification meaning we want to consider local and
nonlocal temp tables the same, so that control passes down to localbuf.c
where we can put the error test. For the other two meanings it probably
doesn't matter, since in principle we shouldn't get as far as making
such decisions for a nonlocal temp table --- but if we did, we'd want
to treat local and nonlocal temp tables alike, ie, no WAL or fsync.

In short, there are only a *very* small number of places, codewise,
where we want to distinguish local from nonlocal temp tables. The test
to be added to localbuf.c is one, and RELATION_IS_LOCAL() is another
(it must not succeed for nonlocals), and right now I don't see any
others.

So my inclination is to leave rd_istemp as a boolean field of
RelationData so as to minimize textual code changes, but redefine it as
a copy of pg_class.relistemp (so that it will now be true for both local
and nonlocal temp tables).

I'm also thinking of adding a bool field rd_islocaltemp to be true only
for local temp tables. This isn't really necessary in terms of code
cleanliness, but it will cache the result of isTempOrToastNamespace so
that we don't have to repeat that call every time through localbuf.c.
(Although it's not *that* expensive, so maybe this is overkill...)

Thoughts, better ideas?

regards, tom lane