Re: A few patches to clarify snapshot management

On 10/08/2025 01:23, Noah Misch wrote:
> On Tue, Jan 07, 2025 at 11:55:00AM +0200, Heikki Linnakangas wrote:
>> On 07/01/2025 00:00, Andres Freund wrote:
>>> On 2024-12-20 19:31:01 +0200, Heikki Linnakangas wrote:
>>>> While playing around some more with this, I noticed that this code in
>>>> GetTransactionSnapshot() is never reached, and AFAICS has always been dead
>>>> code:
>>>>
>>>>> Snapshot
>>>>> GetTransactionSnapshot(void)
>>>>> {
>>>>> /*
>>>>> * Return historic snapshot if doing logical decoding. We'll never need a
>>>>> * non-historic transaction snapshot in this (sub-)transaction, so there's
>>>>> * no need to be careful to set one up for later calls to
>>>>> * GetTransactionSnapshot().
>>>>> */
>>>>> if (HistoricSnapshotActive())
>>>>> {
>>>>> Assert(!FirstSnapshotSet);
>>>>> return HistoricSnapshot;
>>>>> }
>>>>
>>>> when you think about it, that's good, because it doesn't really make sense
>>>> to call GetTransactionSnapshot() during logical decoding. We jump through
>>>> hoops to make the historic catalog decoding possible with historic
>>>> snapshots, tracking subtransactions that modify catalogs and WAL-logging
>>>> command ids, but they're not suitable for general purpose queries. So I
>>>> think we should turn that into an error, per attached patch.
>>>
>>> Hm. I'm not sure it's a good idea to forbid this. Couldn't there be sane C
>>> code in an output functions calling GetTransactionSnapshot() or such to do
>>> some internal lookups?
>>
>> I haven't seen any. And I don't think that would work correctly while doing
>> logical decoding anyway, because historical snapshots only track XIDs that
>> modify catalogs. regclassout and enumout do work because they use the
>> catalog snapshot rather than GetTransactionSnapshot().
>>
>> (I committed that change in commit 1585ff7387 already, but discussion is
>> still welcome of course)
>
> https://github.com/2ndQuadrant/pglogical does rely on the pre-1585ff7387 code
> for its row_filter feature. row_filter calls ExecEvalExpr() from the output
> plugin, e.g. to evaluate expression "id between 2 AND 4" arising from this
> configuration in the pglogical test suite:
>
> SELECT * FROM pglogical.replication_set_add_table('default', 'basic_dml', false, row_filter := $rf$id between 2 AND 4$rf$);
>
> One of the GetTransactionSnapshot() calls is in pglogical_output_plugin.c
> itself. For that, I could work around the change by forcing the old
> HistoricSnapshot use:
>
> - PushActiveSnapshot(GetTransactionSnapshot());
> + Assert(HistoricSnapshotActive());
> + PushActiveSnapshot(GetCatalogSnapshot(InvalidOid));
>
> That doesn't get far. Calling a plpgsql function in the expression reaches
> the "cannot take query snapshot during logical decoding" error via this stack
> trace:
>
> GetTransactionSnapshot at snapmgr.c:279:3
> exec_eval_simple_expr at pl_exec.c:6214:3
> (inlined by) exec_eval_expr at pl_exec.c:5699:6
> exec_stmt_raise at pl_exec.c:3820:8
> (inlined by) exec_stmts at pl_exec.c:2096:10
> exec_stmt_block at pl_exec.c:1955:6
> exec_toplevel_block at pl_exec.c:1646:7
> plpgsql_exec_function at pl_exec.c:636:5
> plpgsql_call_handler at pl_handler.c:278:11
> fmgr_security_definer at fmgr.c:755:52
> ExecInterpExpr at execExprInterp.c:927:7
> pglogical_change_filter at pglogical_output_plugin.c:663:7
> (inlined by) pg_decode_change at pglogical_output_plugin.c:691:7
> change_cb_wrapper at logical.c:1121:22
> ReorderBufferApplyChange at reorderbuffer.c:2078:3
> (inlined by) ReorderBufferProcessTXN at reorderbuffer.c:2383:7
> DecodeCommit at decode.c:743:3
> (inlined by) xact_decode at decode.c:242:5
> LogicalDecodingProcessRecord at decode.c:123:1
> XLogSendLogical at walsender.c:3442:33
> WalSndLoop at walsender.c:2837:7
> StartLogicalReplication at walsender.c:1504:2
> (inlined by) exec_replication_command at walsender.c:2158:6
> PostgresMain at postgres.c:4762:10
> BackendMain at backend_startup.c:80:2
> postmaster_child_launch at launch_backend.c:291:3
> BackendStartup at postmaster.c:3587:8
> (inlined by) ServerLoop at postmaster.c:1702:6
> PostmasterMain at postmaster.c:1252:6
> main at main.c:165:4
>
>>> Hm. I'm not sure it's a good idea to forbid this. Couldn't there be sane C
>>> code in an output functions calling GetTransactionSnapshot() or such to do
>>> some internal lookups?
>
> I think pglogical_output_plugin.c w/ plpgsql is largely sane when used with a
> plpgsql function that consults only catalogs and the output tuple. If the
> pglogical test suite is representative, that's the usual case for a
> row_filter. A plpgsql function that reads user tables will be fragile with
> concurrent pruning, but a user might sanely accept that fragility. A plpgsql
> function that writes tuples is not sane in a row_filter. How do you see it?
>
> So far, I know of these options:
>
> 1. Make pglogical block the row_filter feature for any v18+ origin.
> 2. Revert postgresql.git commit 1585ff7387.
> 3. Make pglogical use HistoricSnapshot where pglogical_output_plugin.c handles
> snapshots directly. That should keep simple row_filter expressions like
> "col > 0" functioning. Entering plpgsql or similarly-complex logic will
> fail with "cannot take query snapshot during logical decoding", and we'll
> consider that to be working as intended.
> 4. Fail later and lazily, for just the most-unreasonable cases. For example,
> fail when HistoricSnapshot applies to a write operation. (Maybe this
> already fails. I didn't check.)
>
> Which of those or other options should we consider?

Hmm, so what snapshot should you use for these row filter expressions
anyway?

As long as you don't try to access any tables, it doesn't matter
much. Although, the effective catalog snapshot also affects how any
functions used in the expression are resolved. If you CREATE OR REPLACE
the row filter function while logical decoding is active, what version
of the function do you expect to be used? I think that's a little fuzzy,
and you might get different answer for the initial sync step and the
on-going decoding. We don't necessarily need to solve that here.
Nevertheless, what would be the least surprising answer to that?

Currently, in v17 and below, we will use the historic snapshot, which
represents the point in time that we are decoding. Is that the right
choice? I'm not sure. A historic snapshot is only supposed to be used
for catalogs, it's not clear if it works correctly for arbitrary
queries. And it's not clear it's the right choice for resolving the row
filter functions either.

How about always using a fresh snapshot instead? Instead of pushing the
historic snapshot as the active snapshot, _disable_ the historic
snapshot and use GetTransactionSnapshot() to acquire a regular snapshot?

We could implement that in GetTransactionSnapshot() ifself by just
removing the check for HistoricSnapshotActive(), and let it call
GetSnapshotData() as usual. But I still think it's a useful sanity check
that you don't call GetTransactionSnapshot() while a historic snapshot
is active, so I'd prefer for the caller to explicitly disable the
historic snapshot first.

Attached is a patch to pglogical to demonstrate that.

Another apprach is to continue down the path you attempted. There are
many places in plpgsql and elsewhere where we call
GetTransactionSnapshot(), but are they really necessary when you're
executing something like the row filter expression? I think the row
filter expression is supposed to be read-only. There are optimizations
already to avoid GetTransactionSnapshot() calls in read-only functions
(i.e. immutable), but we could expand those to any function in a
read-only transaction, and set XactReadOnly while evaluating the row
filter expression.

The second attached patch makes that change in PostgreSQL code. With
those changes, the pglogical change you attempted to do
"PushActiveSnapshot(GetCatalogSnapshot(InvalidOid))" seems to work. I'm
not sure it covers all the cases though, there might be more
GetTransactionSnapshot() calls lurking.

I think I prefer the change to pglogical to disable the historic
snapshot. It feels more robust. I'm not sure if there's a performance
penalty though, as you now need to call GetSnapshotData() for every
decoded transaction.

Finally, attached is a pglogical test case to test what happens if you
change the datatype of the table, while there's decoding active with a
complex row filter function that also accesses the table. I'm not sure
how that should behave and I think that falls into the category of
"don't do that". But FWIW, on v17 it tries to read it fails with this:

ERROR: could not read blocks 0..0 in file "base/16384/17512": read only
0 of 8192 bytes

while with the attached pglogical-disable-historic-snapshot.patch it
fails more nicely:

[2025-08-15 14:05:41.514 EEST] [3824375] [regression] ERROR: attribute
1 of type rowfilter_ddl_tbl has wrong type
[2025-08-15 14:05:41.514 EEST] [3824375] [regression] DETAIL: Table has
type text, but query expects integer.

- Heikki