Re: Reject access to unlogged tables in contrib modules

Hi Jim,

I tested v3 on a primary/standby pair built on top of eaa5961efc6.
All five patches apply cleanly and build without warnings.
On the standby, every new check I tried reports the new error for all the
functions you reported.
get_raw_page(..., 'init', 0) on an unlogged index still works, as intended.

A few comments:
1) In 0001, the detoast guard checks the relation passed by the caller,
but detoasting follows va_toastrelid from the pointer itself, so the two
can disagree. In addition to Kirill's u_toast/u_snapshot, I
created on the primary:

CREATE TABLE logged_text (b text);
CREATE TABLE l_toast (b text);
ALTER TABLE l_toast ALTER COLUMN b SET STORAGE EXTERNAL;
INSERT INTO l_toast VALUES (repeat('abc', 3000));
CREATE TABLE l_snapshot AS SELECT get_raw_page('l_toast', 0) AS raw;

Then on the standby:

-- page from an unlogged table, permanent table passed in
SELECT octet_length(t_attrs[1]) FROM u_snapshot,
LATERAL heap_page_item_attrs(raw, 'logged_text'::regclass, true);
ERROR: could not open file "base/5/16468": No such file or directory

-- page from a permanent table, unlogged table passed in
SELECT octet_length(t_attrs[1]) FROM l_snapshot,
LATERAL heap_page_item_attrs(raw, 'u_toast'::regclass, true);
ERROR: cannot access temporary or unlogged relations during recovery

The second value is readable on the standby. Checking the TOAST relation
from the pointer fixes both, and still catches Kirill's case:

if (VARATT_IS_EXTERNAL_ONDISK(tupdata + off) &&
RecoveryInProgress())
{
toast_external_data toast_ext_data;

toast_external_info_get((varlena *) (tupdata + off),
&toast_ext_data);
/* If the relation is gone, let detoasting complain. */
if (SearchSysCacheExists1(RELOID,

ObjectIdGetDatum(toast_ext_data.toastrelid)) &&
get_rel_persistence(toast_ext_data.toastrelid) !=
RELPERSISTENCE_PERMANENT)
ereport(ERROR, ...);
}

toast_external_info_get() handles both the OID and OID8 pointer formats.
The existence check matters for a saved page whose TOAST
table has since been dropped, without it get_rel_persistence() fails with
"cache lookup failed for relation", while with it the error stays
"could not open relation with OID", as in v3.

2) pg_freespacemap has the same problem as pg_visibility_map*. On the
standby, pg_freespace('ut', 0) returns 0 while the primary returns 3968.
For another session's temp table, it returns 0 if the table has
no FSM fork yet, but fails with "cannot access temporary tables of other
sessions" once it has one. Attached is 0006, which adds both
checks to pg_freespace(), as 0004 and 0005 do for pg_visibility; the
module's regression test passes with it.

3) Regarding Michael's point about duplication, pg_visibility now has a
single check, but pgstattuple still carries six copies and pageinspect
three (plus the variants in get_raw_page and the detoast path), and core
has three more of the same shape (plancat.c, copyto.c,
tid.c). Would a small core helper make sense here, something like
CheckRelationReadableDuringRecovery(Relation), so all of these share one
place and one message?

A minor comment, pg_prewarm on an unlogged table on a standby reports
'fork "main" does not exist for this relation', which is not wrong but
could use the same check and message as the other modules.

Regards,
Surya Poondla