Re: A few patches to clarify snapshot management - Mailing list pgsql-hackers

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
AND4$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