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
