Subject: Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)

From: Justin Pryzby <[email-masked]>
Date: Wed, 13 Apr 2022 15:28:19 -0500

On Wed, Mar 30, 2022 at 05:58:24PM +0900, Kyotaro Horiguchi wrote:
> I'm not still confident on this, but it should be better than the v1.

+Andres as this seems to be related to 277692220.
I added it as an Opened Item.

---

From: Andres Freund <[email-masked]>
Date: Wed, 13 Apr 2022 17:38:02 -0700

Hi,

> +Andres as this seems to be related to 277692220.

FWIW, that'd just mean it's an old bug that wasn't easily noticeable
before, not that it's the fault of 277692220.

> I added it as an Opened Item.

IOW, it'd belong in "Older bugs affecting stable branches".

Greetings,

Andres Freund

---

From: Robert Haas <[email-masked]>
Date: Thu, 14 Apr 2022 09:54:55 -0400

> before, not that it's the fault of 277692220.

I think you're still on the hook to do something about it for this
release. You could decide to revert the commit adding the assertion
and punt on doing thing about the underlying bug, but we can't just be
like "oh, an assertion is failing, we'll get to that someday".

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Tom Lane <[email-masked]>
Date: Thu, 14 Apr 2022 10:42:53 -0400

> release.

I think you're trying to shoot the messenger.  As Andres says,
277692220 just exposes that there is some pre-existing bug here.
It's probably related to 84f5c2908, so I was planning to take a
look at it at some point, but there are a few other higher-priority
bugs in the way.

I see no point in reverting 277692220.  Removing the Assert would
prevent, or at least complicate, detection of other similar bugs.
And it'd do nothing to help end users, who won't have assertions
enabled anyway.

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Thu, 14 Apr 2022 11:33:45 -0400

> look at it at some point, but there are a few other higher-priority

Well, if you're willing to look at it that's fine, but I just don't
agree that it's OK to just commit things that add failing assertions
and drive off into the sunset. The code is always going to have a
certain number of unfixed bugs, and that's fine, and finding them is
good in itself, but people want to be able to run the software in the
meantime, and some of those people are developers or other individuals
who want to run it with assertions enabled. It's a judgement call
whether this assertion failure is going to bite enough people to be a
problem, but if it were something that happened easily enough to
prevent you from working on the source code, I'm sure you wouldn't be
OK with leaving it in there until someone got around to looking at it.
Given that it took about a month and a half for someone to report them
problem, it's not as bad as all that, but I guess I won't be surprised
if we keep getting complaints until something gets done.

---

From: Andres Freund <[email-masked]>
Date: Thu, 14 Apr 2022 08:41:13 -0700

Hi,

On 2022-04-14 11:33:45 -0400, Robert Haas wrote:
> and drive off into the sunset.

I'm not planning to ride into the sunset / ignore this issue. All I said
is that it's imo not the right thing to say that that commit broke
things in 15. And that not a semantics game, because it means that the
fix needs to go back further than 277692220.

Greetings,

Andres Freund

---

From: Tom Lane <[email-masked]>
Date: Thu, 14 Apr 2022 11:46:46 -0400

> and drive off into the sunset.

I don't think Andres had any intention of ignoring it indefinitely.
What he is doing is prioritizing it lower than several of the other
open items, an opinion I fully agree with.

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Thu, 14 Apr 2022 12:16:45 -0400

> open items, an opinion I fully agree with.

Well, my point is that, historically, relegating things to the older
bugs section often means nothing happens, which I think would not be
great in this case. However, I'll try to shut up about the procedural
issues for the moment, since I seem to be in the minority.

I got curious and looked at the underlying problem here and I am
wondering whether HaveRegisteredOrActiveSnapshot() is just buggy. It
seems to me that the code is always going to return true if there are
any active snapshots, and the rest of the function is intended to test
whether there is a registered snapshot other than the catalog
snapshot. But I don't think that's what this code does:

     if (pairingheap_is_empty(&RegisteredSnapshots) ||
         !pairingheap_is_singular(&RegisteredSnapshots))
         return false;

     return CatalogSnapshot == NULL;

So, if there are 0 active snapshots we return false. And if there is 1
active snapshot then we return true if there's no catalog snapshot and
otherwise false. So far so good. But if there are 2 or more registered
snapshots then the pairing heap will be neither empty nor singular so
it seems to me we will return false, which seems to me to be the wrong
answer. I tried this:

diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index a0be0c411a..4e8e26a362 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -1646,9 +1646,10 @@ HaveRegisteredOrActiveSnapshot(void)
      * removed at any time due to invalidation processing. If explicitly
      * registered more than one snapshot has to be in RegisteredSnapshots.
      */
-    if (pairingheap_is_empty(&RegisteredSnapshots) ||
-        !pairingheap_is_singular(&RegisteredSnapshots))
+    if (pairingheap_is_empty(&RegisteredSnapshots))
         return false;
+    if (!pairingheap_is_singular(&RegisteredSnapshots))
+        return true;

     return CatalogSnapshot == NULL;
 }

I find that 'make check-world' passes with this change, which is
disturbing, because it also passes without this change. That means we
don't have any tests that reach HaveRegisteredOrActiveSnapshot() with
more than one registered snapshot.

Also, unless we have plans to use HaveRegisteredOrActiveSnapshot() in
more places, I think we should consider revising the whole approach
here. The way init_toast_snapshot() is coded, we basically have some
code that tests for active and registered snapshots and finds the
oldest one. We error out if there isn't one. And then we get to this
assertion, which checks the same stuff a second time but with an
additional check to see whether we ended up with the catalog snapshot.
Wouldn't it make more sense if GetOldestSnapshot() just refused to
return the catalog snapshot in the first place?

---

From: Andres Freund <[email-masked]>
Date: Thu, 14 Apr 2022 09:54:25 -0700

Hi,

>      return CatalogSnapshot == NULL;

Certainly looks off...

> more than one registered snapshot.

Part of that is because right now the assertion is placed "too deep" -
it should be much higher up, so it's reached even if there's not
actually a toast datum. But there's of other bugs preventing that :(. A
lot of bugs have been hidden by the existence of CatalogSnapshot (which
of course isn't something one actually can rely on).

> more places,

I think we should, but there's the other bugs that need to be fixed
first :(. Namely that we have plenty places doing catalog accesses
without an active or registered snapshot :(.

> I think we should consider revising the whole approach here. The way
> init_toast_snapshot() is coded, we basically have some code that tests
> for active and registered snapshots and finds the oldest one. We error
> out if there isn't one. And then we get to this assertion, which
> checks the same stuff a second time but with an additional check to
> see whether we ended up with the catalog snapshot.  Wouldn't it make
> more sense if GetOldestSnapshot() just refused to return the catalog
> snapshot in the first place?

I'm worried that that could cause additional bugs. Consider code using
GetOldestSnapshot() to check if tuples need to be preserved or such.

Greetings,

Andres Freund

---

From: Andres Freund <[email-masked]>
Date: Thu, 14 Apr 2022 09:58:37 -0700

Hi,

> without an active or registered snapshot :(.

Ah, we actually were debating some of these issues more recently, in:
https://www.postgresql.org/message-id/20220311030721.olixpzcquqkw2qyt%40alap3.anarazel.de
https://www.postgresql.org/message-id/20220311021047.hgtqkrl6n52srvdu%40alap3.anarazel.de

It looks like the same bug, and that the patch in this thread fixes
them. And that we need to backpatch the fix, right?

Greetings,

Andres Freund

---

From: Robert Haas <[email-masked]>
Date: Thu, 14 Apr 2022 13:21:36 -0400

> of course isn't something one actually can rely on).

I am wondering whether ffaa44cb559db332baeee7d25dedd74a61974203 bet on
the wrong horse. When I invented CatalogSnapshot, I intended for it to
be an ephemeral snapshot that we'd keep for as long as we could safely
do so and then discard it as soon as there's any hint of a problem.
But that commit really takes the opposite approach, trying to keep
CatalogSnapshot valid for a longer period of time instead of just
chucking it.

> without an active or registered snapshot :(.

Ouch.

> GetOldestSnapshot() to check if tuples need to be preserved or such.

But there is no such code: GetOldestSnapshot() has only one caller.
And I don't think we'd add more just to do something like that,
because we have other mechanisms that are specifically designed for
testing whether tuples are prunable that are better suited to such
tasks. It should really only be used when you don't know which of the
backend's current snapshots ought to be used for something, but are
sure that using the oldest one will be good enough. And in that kind
of situation, it's hard to see why using the catalog snapshot would
ever be the right idea.

---

From: Tom Lane <[email-masked]>
Date: Thu, 14 Apr 2022 13:36:51 -0400

> CatalogSnapshot valid for a longer period of time instead of just

Not really following?  ISTM the point of ffaa44cb5 was that if we don't
include the CatalogSnapshot in our advertised xmin, then the length
of time we can safely use it is *zero*.

> of situation, it's hard to see why using the catalog snapshot would

What if the reason we need a snapshot is to detoast some toasted item
we read from a catalog with the CatalogSnapshot?  There might not be
any other valid snapshot, so I don't think I buy your argument here.

... Unless your argument is that the session should always have an
older non-catalog snapshot, which I'm not sure whether I buy or not.
But if we do believe that, then adding mechanisms to force it to be so
could be an alternative solution.  But why is that better than what
we're doing?

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Thu, 14 Apr 2022 11:03:32 -0700

Hi,

> any other valid snapshot, so I don't think I buy your argument here.

We definitely do have places doing that, but is it ever actually safe?
Part of the catalog access might cause cache invalidations to be
processed, which can invalidate the snapshot (including resetting
MyProc->xmin). Afaics we always would have push or register the
snapshot, either will copy the snapshot, I think?

Greetings,

Andres Freund

---

From: Robert Haas <[email-masked]>
Date: Thu, 14 Apr 2022 14:19:08 -0400

> of time we can safely use it is *zero*.

No, I don't think so. I'm proposing that you shouldn't be taking a
catalog snapshot unless you already hold some other snapshot, and that
the catalog snapshot should always be newer than whatever that other
snapshot is. If we made that be true, then your catalog snapshot
couldn't ever be the thing holding back xmin -- and then I think it
doesn't need to be registered.

> could be an alternative solution.  But why is that better than what

That is exactly my argument, but I'm not actually sure whether it is
in fact better. I was responding to Andres's statement that
CatalogSnapshot was hiding a lot of bugs because it makes it look like
we have a snapshot when we don't really. And my point is that
ffaa44cb559db332baeee7d25dedd74a61974203 made such problems a lot more
likely, because before that the snapshot was not in
RegisteredSnapshots, and therefore anybody asking "hey, do we have a
snapshot?" wasn't likely to get confused, because they would only see
the catalog snapshot if they specifically went and looked at
CatalogSnapshot(Set), and if you do that, hopefully you'll think about
the special semantics of that snapshot and write code that works with
them. But with CatalogSnapshot in RegisteredSnapshots, any code that
looks through RegisteredSnapshots has to worry about whether what it's
finding there is actually just the CatalogSnapshot, and if Andres's
statement that we have a lot of bugs here is to be believed, then we
have not done a good job finding and updating all such code. We can
continue down the path of finding and fixing it -- or we can back out
parts of ffaa44cb559db332baeee7d25dedd74a61974203.

Just to be clear, I'm not debating that that commit fixed some real
problems and I think parts of it are really necessary fixes. But to
quote from the commit message:

    The CatalogSnapshot was not plugged into SnapshotResetXmin()'s accounting
    for whether MyPgXact->xmin could be cleared or advanced.  In normal
    transactions this was masked by the fact that the transaction snapshot
    would be older, but during backend startup and certain utility commands
    it was possible to re-use the CatalogSnapshot after MyPgXact->xmin had
    been cleared ...

And what I'm suggesting is that *perhaps* we ought to have fixed those
"during backend startup and certain utility commands" by having
SnapshotResetXmin() do InvalidateCatalogSnapshot(), and maybe also
made the code in those commands that's doing catalog lookups hold
acquire and hold a snapshot of its own around the operation. The
alternative you chose, namely, incorporating the xmin into the
backend's xmin computation, I think can also be made to work. I am
just not sure that it's the best approach.

---

From: Tom Lane <[email-masked]>
Date: Thu, 14 Apr 2022 14:47:26 -0400

> Certainly looks off...

Yeah, this is broken.  Whilst waiting around for a build on wrasse's
host, I reproduced the problem shown in this thread, and here's what
I see at the point of the exception:

(gdb) p RegisteredSnapshots
$5 = {ph_compare = 0x9a6000 <xmin_cmp>, ph_arg = 0x0, 
  ph_root = 0xec3168 <CatalogSnapshotData+72>}
(gdb) p *RegisteredSnapshots.ph_root
$6 = {first_child = 0x2d85d70, next_sibling = 0x0, prev_or_parent = 0x0}
(gdb) p CatalogSnapshotData
$7 = {snapshot_type = SNAPSHOT_MVCC, xmin = 52155, xmax = 52155, 
  xip = 0x2d855b0, xcnt = 0, subxip = 0x2de9130, subxcnt = 0, 
  suboverflowed = false, takenDuringRecovery = false, copied = false, 
  curcid = 0, speculativeToken = 0, vistest = 0x0, active_count = 0, 
  regd_count = 0, ph_node = {first_child = 0x2d85d70, next_sibling = 0x0, 
    prev_or_parent = 0x0}, whenTaken = 0, lsn = 0, snapXactCompletionCount = 1}
(gdb) p CatalogSnapshot    
$8 = (Snapshot) 0xec3120 <CatalogSnapshotData>
(gdb) p *(Snapshot) (0x2d85d70-72)
$9 = {snapshot_type = SNAPSHOT_MVCC, xmin = 52155, xmax = 52155, xip = 0x0, 
  xcnt = 0, subxip = 0x0, subxcnt = 0, suboverflowed = false, 
  takenDuringRecovery = false, copied = true, curcid = 0, 
  speculativeToken = 0, vistest = 0x0, active_count = 0, regd_count = 2, 
  ph_node = {first_child = 0x0, next_sibling = 0x0, 
    prev_or_parent = 0xec3168 <CatalogSnapshotData+72>}, whenTaken = 0, 
  lsn = 0, snapXactCompletionCount = 0}
(gdb) p ActiveSnapshot
$10 = (ActiveSnapshotElt *) 0x0

So in fact there IS another registered snapshot, and
HaveRegisteredOrActiveSnapshot is just lying.  I think the
correct test is more nearly what we have in
InvalidateCatalogSnapshotConditionally:

    if (CatalogSnapshot &&
        ActiveSnapshot == NULL &&
        pairingheap_is_singular(&RegisteredSnapshots))
            // then the CatalogSnapshot is the only one.

Ergo, this actually is a bug in 277692220.

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Thu, 14 Apr 2022 15:05:50 -0400

> doesn't need to be registered.

If you don't register it, then you need to also make sure that it's
destroyed whenever that older snapshot is.  Which I think would require
either a lot of useless/inefficient CatalogSnapshot destructions, or
infrastructure that's more or less isomorphic to the RegisteredSnapshot
heap.

> we have a snapshot when we don't really.

Well, we DO have a snapshot, and it is 100% perfectly safe to use, if it's
registered.  Andres' complaint is that that snapshot might get invalidated
when you weren't expecting it, but I'm not really convinced that we have
all that many bugs of that ilk.  Wouldn't CLOBBER_CACHE_ALWAYS testing
find them?

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Thu, 14 Apr 2022 15:55:30 -0400

> infrastructure that's more or less isomorphic to the RegisteredSnapshot

Well, if that's true, then I agree that it's a good argument against
that approach. But I guess I'm confused as to why we'd end up in that
situation. Suppose we do these two things:

1. Decree that SnapshotResetXmin calls InvalidateCatalogSnapshot. It's
the other way around right now, but that's only because we're
registering the catalog snapshot.
2. Bomb out in GetCatalogSnapshot if you don't have an active or
registered snapshot already.

Is there some reason we'd need any more infrastructure than that?

> all that many bugs of that ilk.  Wouldn't CLOBBER_CACHE_ALWAYS testing

Hmm, that's a good question. I don't know.

---

From: Tom Lane <[email-masked]>
Date: Thu, 14 Apr 2022 16:26:00 -0400

Robert Haas <[email-masked]> writes:
>> destroyed whenever that older snapshot is.

> Is there some reason we'd need any more infrastructure than that?

Yes.

1. Create snapshot 1 (beginning of transaction).
2. Create catalog snapshot (okay because of snapshot 1).
3. Create snapshot 2.
4. Destroy snapshot 1.
5. Catalog snapshot is still there and is now the oldest.

The implementation you propose would have to also forbid this sequence
of events, which is (a) difficult and (b) would add instability to the
system, since there's really no reason that this should be Not OK.

I'm basically not on board with adding complication to make the system
less performant and more brittle, and I don't see how the direction
you want to go isn't that.

(BTW, this thought experiment also puts a hole in the test added by
277692220: even if HaveRegisteredOrActiveSnapshot were doing what
it claims to do, it would allow use of the catalog snapshot for
detoasting after step 4, which I suppose is not what Andres intended.)

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Fri, 15 Apr 2022 08:47:14 -0400

> 5. Catalog snapshot is still there and is now the oldest.

Sorry, I'm not seeing the problem. If we call SnapshotResetXmin()
after step 4, then the catalog snapshot would get invalidated under my
proposal. If we don't, then our advertised xmin has not changed and
nothing can be pruned out from under us.

> you want to go isn't that.

Well ... I agree that brittle is bad, but it's not clear to me which
way is actually less brittle. As for performant, I think you might be
misjudging the situation. My original design for removing SnapshotNow
didn't even have the catalog snapshot - it just took a new snapshot
every time. That was mostly fine, but Andres found a somewhat extreme
test case where it exhibited a significant regression, so I added the
catalog snapshot stuff to work around that. So I'm not AT ALL
convinced that giving catalog snapshots longer lifetimes is a relevant
thing to do. There's some value in it if you can construct a test case
where the overall rate of snapshot taking is extremely high, but in
normal cases that isn't so. It's certainly not worth complicating the
code for backend startup or DDL commands to reduce the number of
snapshots, because you're never going to have those things happening
at a high enough rate to matter, or so I believe.

The way you get a benefit from CatalogSnapshot is to construct a
workload with a lot of really cheap SQL statements each of which has
to do a bunch of catalog lookups, and then run that at high
concurrency. The concurrency makes taking snapshots more expensive,
because the cache lines are contended, and having the same command use
the same snapshot over and over again instead of taking new ones then
brings the cost down enough to be measurable. But people run SQL
statements in a tight loop, not DDL or backend startup.

---

From: Tom Lane <[email-masked]>
Date: Sat, 16 Apr 2022 14:42:39 -0400

Robert Haas <[email-masked]> writes:
> thing to do.

Perhaps not.  But right now we have to first think about correctness and
how much trouble it will be to get to correctness.  ISTM you are arguing
for a design in which it's required that there is always a registered
or active snapshot that's older than the catalog snapshot (if any).
I tried revising snapmgr.c to enforce that, starting with adding 

@@ -421,6 +421,13 @@ GetNonHistoricCatalogSnapshot(Oid relid)
   
     if (CatalogSnapshot == NULL)
     {
+        /*
+         * The catalog snapshot must always be newer than some active or
+         * registered snapshot.  (XXX explain why)
+         */
+        Assert(ActiveSnapshot != NULL ||
+               !pairingheap_is_empty(&RegisteredSnapshots));
+
         /* Get new snapshot. */
         CatalogSnapshot = GetSnapshotData(&CatalogSnapshotData);
 
and this blew up with truly impressive thoroughness.  The autovac
launcher, logical replication launcher, and incoming backends all
fail this assertion instantly, making it impossible to find out
what else might be broken --- but I'm sure there is a lot.

(If you want to try this for yourself, remember that the postmaster
will relaunch the AV and LR launchers on failure, meaning that your
disk will fill with core files very very quickly.  Just sayin'.)

So maybe we can go that direction, but it's going to require a lot of
code additions to push extra snapshots in places that haven't bothered
to date; and I'm not convinced that that'd be buying us anything
except more GetSnapshotData() calls.

Plan B is to grant catalog snapshots some more-durable status than
what Plan A envisions.  I'm not very sure about the details, but if
you don't want to go that route then you'd better set about making
the above assertion work.

In the meantime, since it's clear that HaveRegisteredOrActiveSnapshot
is failing to meet its contract, I'm going to go fix it.  I think
(based on the above argument) that what it intends to enforce is not
really the system design we need, but it certainly isn't helping
anyone that it enforces that design incorrectly.

			regards, tom lane

---

From: Andres Freund <[email-masked]>
Date: Sun, 17 Apr 2022 06:31:12 -0700

Hi,

On 2022-04-16 14:42:39 -0400, Tom Lane wrote:
> is failing to meet its contract, I'm going to go fix it.

+1

> I think (based on the above argument) that what it intends to enforce
> is not really the system design we need, but it certainly isn't
> helping anyone that it enforces that design incorrectly.

I think it's approximately right for the current caller. But that caller
likely needs an improved design around snapshots...

Greetings,

Andres Freund

---

From: Tom Lane <[email-masked]>
Date: Sun, 17 Apr 2022 11:51:58 -0400

> likely needs an improved design around snapshots...

Yeah, I think the real issue is that checking
HaveRegisteredOrActiveSnapshot in this way doesn't provide a very
good guarantee of what we really want to know, which is that the
session's advertised xmin is old enough to prevent removal of
whatever toast data we're trying to fetch.  The fact that we
have a snapshot at the instant of fetch doesn't prove that it
existed continually since we fetched the toast reference, which
seems to be the condition we actually need to assure.  (And TBH
I see little reason to think that whether the snapshot is the
CatalogSnapshot or not changes things in any meaningful way.)

I don't yet see a practical way to check for the real concern.
While it's something to worry about, there's no reason to think
that v15 is any worse than prior versions in this area, is there?
So I'm inclined to remove this from the list of v15 open items,
or at least demote the remaining concern to "older bug" status.

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Mon, 18 Apr 2022 09:22:00 -0400

> what else might be broken --- but I'm sure there is a lot.

OK, thanks for trying that.

> anyone that it enforces that design incorrectly.

+1.

---

From: Andres Freund <[email-masked]>
Date: Mon, 18 Apr 2022 07:39:01 -0700

Hi,

On 2022-04-17 11:51:58 -0400, Tom Lane wrote:
> whatever toast data we're trying to fetch.

Right. It's better than what was there before though - I added
HaveRegisteredOrActiveSnapshot() in the course of
7c38ef2a5d6cf6d8dc3834399d7a1c364d64ce64. Where the problem was that we
didn't have *any* snapshot other than the catalog snapshot, and the
catalog snapshot only sometimes (iirc for that bug it depended on the
order in which objects were deleted).  That makes such bugs much harder
to detect.

> The fact that we have a snapshot at the instant of fetch doesn't prove
> that it existed continually since we fetched the toast reference,
> which seems to be the condition we actually need to assure.

Right.

> (And TBH I see little reason to think that whether the snapshot is the

It is a meaningful difference, see e.g. the bug referenced above.

Yes.

Greetings,

Andres Freund

---

From: Tom Lane <[email-masked]>
Date: Mon, 18 Apr 2022 10:51:08 -0400

> It is a meaningful difference, see e.g. the bug referenced above.

Well, that's true given the current arrangements for managing
CatalogSnapshot; but that doesn't make the CatalogSnapshot any
less of a protection when it exists.  The direction I was vaguely
imagining is that we create some refcount-like infrastructure
directly ensuring that once a snapshot is used to read a toast
reference, it gets kept around until we dereference or discard
that reference.  With a scheme like that, there'd be no reason to
discriminate against a CatalogSnapshot as being the protective
snapshot.

(I hasten to add that I have no idea how to make this half-baked
plan work, and there may be better solutions anyway.)

> Yes.

OK, I'll update the open-items page.

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Mon, 18 Apr 2022 16:06:58 -0400

> Right.

BTW, after thinking about this for a bit I am less concerned than
I was about the system being full of bugs of this ilk.  The executor
per se should be fine because it does everything under a live snapshot.
We had bugs with cases that shove executor output into long-lived
tuplestores, but we've dealt with that scenario.  Catalog updates
performed on tuples fetched from a catalog scan seem safe enough too.
Andres was worried about catalog updates performed using tuples fetched
from catcache, but that's not a problem because we detoasted every value
when it went into the catcache, cf 08e261cbc.

(Mind you, 08e261cbc's solution is risky performancewise, because it
means we have to re-toast every value during such catalog updates,
instead of being able to carry the old values of unchanged columns
forward.  But it's not a correctness bug.)

(Also, the whining I did in 08e261cbc's commit message is no longer
relevant now that we read catalogs with MVCC snapshots.)

There may be some corner cases that aren't described by any of these
three blanket scenarios, but they've got to be pretty few and far
between.

			regards, tom lane