Thread: Re: A few patches to clarify snapshot management
On Mon, Dec 16, 2024 at 12:06:33PM +0200, Heikki Linnakangas wrote: > While working on the CSN snapshot patch, I got sidetracked looking closer > into the snapshot tracking in snapmgr.c. Attached are a few patches to > clarify some things. I haven't yet looked closely at what you are proposing, but big +1 from me for the general idea. I recently found myself wishing for a lot more commentary about this stuff [0]. [0] https://postgr.es/m/Z0dB1ld2iPcS6nC9%40nathan -- nathan
On 2024-12-20 19:31:01 +0200, Heikki Linnakangas wrote: > On 16/12/2024 23:56, Nathan Bossart wrote: > > On Mon, Dec 16, 2024 at 12:06:33PM +0200, Heikki Linnakangas wrote: > > > While working on the CSN snapshot patch, I got sidetracked looking closer > > > into the snapshot tracking in snapmgr.c. Attached are a few patches to > > > clarify some things. > > > > I haven't yet looked closely at what you are proposing, but big +1 from me > > for the general idea. I recently found myself wishing for a lot more > > commentary about this stuff [0]. > > > > [0] https://postgr.es/m/Z0dB1ld2iPcS6nC9%40nathan > > 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? Greetings, Andres Freund
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)
--
Heikki Linnakangas
Neon (https://neon.tech)
On 06/01/2025 23:30, Heikki Linnakangas wrote: > On 20/12/2024 19:31, Heikki Linnakangas wrote: >>> /* >>> * Struct representing all kind of possible snapshots. >>> * >>> * There are several different kinds of snapshots: >>> * * Normal MVCC snapshots >>> * * MVCC snapshots taken during recovery (in Hot-Standby mode) >>> * * Historic MVCC snapshots used during logical decoding >>> * * snapshots passed to HeapTupleSatisfiesDirty() >>> * * snapshots passed to HeapTupleSatisfiesNonVacuumable() >>> * * snapshots used for SatisfiesAny, Toast, Self where no members are >>> * accessed. >>> * >>> * TODO: It's probably a good idea to split this struct using a NodeTag >>> * similar to how parser and executor nodes are handled, with one >>> type for >>> * each different kind of snapshot to avoid overloading the meaning of >>> * individual fields. >>> */ >>> typedef struct SnapshotData >> >> I'm thinking of implementing that TODO, splitting SnapshotData into >> separate structs like MVCCSnapshotData, SnapshotDirtyData, etc. It >> seems to me most places can assume that you're dealing with MVCC >> snapshots, and if we had separate types for them, could be using >> MVCCSnapshot instead of the generic Snapshot. Only the table and index >> AM functions need to deal with non-MVCC snapshots. > > Here's a draft of that. Going through this exercise clarified a few > things to me that I didn't realize before: > > - The executor only deals with MVCC snapshots. Special snapshots are > only for the lower-level AM interfaces. > - Only MVCC snapshots can be pushed to the active stack > - Only MVCC or historic MVCC snapshots can be registered with a resource > owner I committed the patches adding comments on Tuesday. Here's an updated version of the patch to split SnapshotData into different structs. The second, new patch simplifies the historic snapshot reference counting during logical decoding. It's in principle independent from the first patch, but it was hard to see how the opportunity before splitting the structs. -- Heikki Linnakangas Neon (https://neon.tech)