Обсуждение: Fallback table AM for relkinds without storage

Hi all,

In recent history, we have had two bugs causing a crash of the backend
because of the default behavior of rd_tableam to be NULL for a
relcache entry for relkinds that have no storage:
1) Sequential attempt for a view:
https://www.postgresql.org/message-id/16856-0363e05c6e1612fd@postgresql.org
2) currtid() and currtid2():
https://postgr.es/m/CAJGNTeO93u-5APMga6WH41eTZ3Uee9f3s8dCpA-GSSqNs1b=Ug@mail.gmail.com

Any hole in the code that allows a relation without storage to attempt
to access a table AM is able to take the server down.  Of course, any
code doing that would be wrong, but it seems to me that we had better
put in place better defenses so as any mistake does not result in a
server going down.  Looking at the code, we would need to do a couple
of things, mainly:
- Create a new table AM for relations without storage to plug into.
The idea would be a simple wrapper for all the AM functions that
triggers a elog(ERROR) for each one of them.  If possible, provide
some details based on the arguments given by the caller of the
function.  Here are some ideas of names: no_storage_table_am,
no_storage_am, error_table_am, error_am, fallback_am (this one sounds
wrong).  This requires an extra row in pg_am.
- Tweak the area around RelationInitTableAccessMethod(), with rd_am so
as rd_amhandler is never NULL.

Putting sanity checks within all the table_* functions of tableam.h
would not be a good idea, as nothing prevents the call of what's
stored in rel->rd_tableam.

Thoughts?
--
Michael

On Tue, Feb 09, 2021 at 04:27:34PM +0900, Michael Paquier wrote:
> Putting sanity checks within all the table_* functions of tableam.h
> would not be a good idea, as nothing prevents the call of what's
> stored in rel->rd_tableam.

I have been playing with this idea, and finished with the attached,
which is not the sexiest patch around.  The table AM used as fallback
for tables without storage is called no_storage (this could be called
virtual_am?).  Reverting e786be5 or dd705a0 leads to an error coming
from no_storage instead of a crash.

One thing to note is that this simplifies a bit slot_callbacks as
views, foreign tables and partitioned tables can grab their slot type
directly from this new table AM.
--
Michael