Re: Reject access to unlogged tables in contrib modules

Hi Surya

On 04/11/2026 00:37, surya poondla wrote:
> 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.

Thank you very much for the thorough review!

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

Nice catch.
v4 now fixes this gap.

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

Yeah, this module also has the same issue.
v4 has your 0006.

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

I'm not so sure about this one. The idea itself is quite reasonable, but
I suspect it would add more complexity to core for very little gain.
We'd also need to change copyto.c, plancat.c, and tid.c, which share the
same message. If we agree that it should be done nevertheless, I'd say
it belongs to another refactoring patch. WDYT?

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

Since the message is not wrong, I'd vote to keep it as is. This patch
set keeps growing and I'd not like to take too much time from the
committers for yet another cosmetic change.

PFA v4. 0002-0005 are unchanged.

Thank you!

Best, Jim