Re: A few patches to clarify snapshot management

On 20/08/2025 03:37, Noah Misch wrote:
> On Tue, Aug 19, 2025 at 11:45:01PM +0300, Heikki Linnakangas wrote:
>> How about the attached, then? It reverts the GetTransactionSnapshot()
>> change. But to still catch at least some of the invalid uses of the historic
>> snapshot, it adds checks to heap_beginscan() and index_beginscan(), to
>> complain if they are called on a non-catalog relation with a historic
>> snapshot.
>
>> @@ -1143,6 +1143,15 @@ heap_beginscan(Relation relation, Snapshot snapshot,
>> if (!(snapshot && IsMVCCSnapshot(snapshot)))
>> scan->rs_base.rs_flags &= ~SO_ALLOW_PAGEMODE;
>>
>> + /* Check that a historic snapshot is not used for non-catalog tables */
>> + if (snapshot &&
>> + IsHistoricMVCCSnapshot(snapshot) &&
>> + !RelationIsAccessibleInLogicalDecoding(relation))
>> + {
>> + elog(ERROR, "cannot query non-catalog table \"%s\" during logical decoding",
>> + RelationGetRelationName(relation));
>> + }
>> +
>
> I feel post-beta3 is late for debut of restrictions like this. How about a
> pure revert, then add those restrictions in v19? Should be s/elog/ereport/,
> also.

Ok, fair. I committed the revert to v18, and the revert + additional
checks to master.

- Heikki