Horton

Re: A few patches to clarify snapshot management

Andres Freund <andres@anarazel.de>

View thread
From: Andres Freund <andres@anarazel.de>
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Nathan Bossart <nathandbossart@gmail.com>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Date: 2025-01-06T22:00:32Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Use ereport() rather than elog()

  2. Revert GetTransactionSnapshot() to return historic snapshot during LR

  3. Improve snapmgr.c comment

  4. Assert that a snapshot is active or registered before it's used

  5. Don't allow GetTransactionSnapshot() in logical decoding

  6. Remove unnecessary GetTransactionSnapshot() calls

  7. snapshot scalability: Don't compute global horizons while building snapshots.

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