Horton

Re: A few patches to clarify snapshot management

Heikki Linnakangas <hlinnaka@iki.fi>

View thread
From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Andres Freund <andres@anarazel.de>
Cc: Nathan Bossart <nathandbossart@gmail.com>, "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Date: 2025-01-07T09:55:00Z
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 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)