public inbox for [email protected]
help / color / mirror / Atom feedRe: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
27+ messages / 5 participants
[nested] [flat]
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
@ 2022-04-13 20:28 Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Justin Pryzby @ 2022-04-13 20:28 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; Andres Freund <[email protected]>
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.
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
@ 2022-04-14 00:38 ` Andres Freund <[email protected]>
2022-04-14 13:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Andres Freund @ 2022-04-14 00:38 UTC (permalink / raw)
To: Justin Pryzby <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; [email protected]; [email protected]
Hi,
On 2022-04-13 15:28:19 -0500, Justin Pryzby wrote:
> 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.
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
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
@ 2022-04-14 13:54 ` Robert Haas <[email protected]>
2022-04-14 14:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Robert Haas @ 2022-04-14 13:54 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Justin Pryzby <[email protected]>; Kyotaro Horiguchi <[email protected]>; Erik Rijkers <[email protected]>; Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>
On Wed, Apr 13, 2022 at 8:38 PM Andres Freund <[email protected]> wrote:
> On 2022-04-13 15:28:19 -0500, Justin Pryzby wrote:
> > 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.
>
> 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 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
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 13:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
@ 2022-04-14 14:42 ` Tom Lane <[email protected]>
2022-04-14 15:33 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Tom Lane @ 2022-04-14 14:42 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; Kyotaro Horiguchi <[email protected]>; Erik Rijkers <[email protected]>; Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>
Robert Haas <[email protected]> writes:
> On Wed, Apr 13, 2022 at 8:38 PM Andres Freund <[email protected]> wrote:
>> 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 think you're still on the hook to do something about it for this
> 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
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 13:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 14:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
@ 2022-04-14 15:33 ` Robert Haas <[email protected]>
2022-04-14 15:41 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 15:46 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
0 siblings, 2 replies; 27+ messages in thread
From: Robert Haas @ 2022-04-14 15:33 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; Kyotaro Horiguchi <[email protected]>; Erik Rijkers <[email protected]>; Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Apr 14, 2022 at 10:42 AM Tom Lane <[email protected]> wrote:
> Robert Haas <[email protected]> writes:
> > On Wed, Apr 13, 2022 at 8:38 PM Andres Freund <[email protected]> wrote:
> >> 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 think you're still on the hook to do something about it for this
> > 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.
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.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 13:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 14:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 15:33 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
@ 2022-04-14 15:41 ` Andres Freund <[email protected]>
1 sibling, 0 replies; 27+ messages in thread
From: Andres Freund @ 2022-04-14 15:41 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; Justin Pryzby <[email protected]>; Kyotaro Horiguchi <[email protected]>; Erik Rijkers <[email protected]>; Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi,
On 2022-04-14 11:33:45 -0400, Robert Haas wrote:
> On Thu, Apr 14, 2022 at 10:42 AM Tom Lane <[email protected]> wrote:
> > Robert Haas <[email protected]> writes:
> > > On Wed, Apr 13, 2022 at 8:38 PM Andres Freund <[email protected]> wrote:
> > >> 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 think you're still on the hook to do something about it for this
> > > 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.
>
> 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.
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
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 13:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 14:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 15:33 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
@ 2022-04-14 15:46 ` Tom Lane <[email protected]>
2022-04-14 16:16 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
1 sibling, 1 reply; 27+ messages in thread
From: Tom Lane @ 2022-04-14 15:46 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; Kyotaro Horiguchi <[email protected]>; Erik Rijkers <[email protected]>; Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>
Robert Haas <[email protected]> writes:
> On Thu, Apr 14, 2022 at 10:42 AM Tom Lane <[email protected]> wrote:
>> 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.
> 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.
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
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 13:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 14:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 15:33 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 15:46 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
@ 2022-04-14 16:16 ` Robert Haas <[email protected]>
2022-04-14 16:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Robert Haas @ 2022-04-14 16:16 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; Kyotaro Horiguchi <[email protected]>; Erik Rijkers <[email protected]>; Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Apr 14, 2022 at 11:46 AM Tom Lane <[email protected]> wrote:
> 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.
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?
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 13:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 14:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 15:33 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 15:46 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 16:16 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
@ 2022-04-14 16:54 ` Andres Freund <[email protected]>
2022-04-14 16:58 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 17:21 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 18:47 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
0 siblings, 3 replies; 27+ messages in thread
From: Andres Freund @ 2022-04-14 16:54 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; Justin Pryzby <[email protected]>; Kyotaro Horiguchi <[email protected]>; Erik Rijkers <[email protected]>; Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi,
On 2022-04-14 12:16:45 -0400, Robert Haas wrote:
> 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;
Certainly looks off...
> 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.
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).
> Also, unless we have plans to use HaveRegisteredOrActiveSnapshot() in
> 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
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 13:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 14:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 15:33 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 15:46 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 16:16 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 16:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
@ 2022-04-14 16:58 ` Andres Freund <[email protected]>
2 siblings, 0 replies; 27+ messages in thread
From: Andres Freund @ 2022-04-14 16:58 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; Justin Pryzby <[email protected]>; Kyotaro Horiguchi <[email protected]>; Erik Rijkers <[email protected]>; Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi,
On 2022-04-14 09:54:25 -0700, Andres Freund wrote:
> On 2022-04-14 12:16:45 -0400, Robert Haas wrote:
> > 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;
>
> Certainly looks off...
>
>
> > 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.
>
> 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).
>
>
> > Also, unless we have plans to use HaveRegisteredOrActiveSnapshot() in
> > 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 :(.
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
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 13:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 14:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 15:33 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 15:46 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 16:16 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 16:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
@ 2022-04-14 17:21 ` Robert Haas <[email protected]>
2022-04-14 17:36 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2 siblings, 1 reply; 27+ messages in thread
From: Robert Haas @ 2022-04-14 17:21 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Justin Pryzby <[email protected]>; Kyotaro Horiguchi <[email protected]>; Erik Rijkers <[email protected]>; Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Apr 14, 2022 at 12:54 PM Andres Freund <[email protected]> wrote:
> 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).
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.
> > Also, unless we have plans to use HaveRegisteredOrActiveSnapshot() in
> > 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 :(.
Ouch.
> I'm worried that that could cause additional bugs. Consider code using
> 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.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 13:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 14:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 15:33 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 15:46 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 16:16 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 16:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 17:21 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
@ 2022-04-14 17:36 ` Tom Lane <[email protected]>
2022-04-14 18:03 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 18:19 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
0 siblings, 2 replies; 27+ messages in thread
From: Tom Lane @ 2022-04-14 17:36 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; Kyotaro Horiguchi <[email protected]>; Erik Rijkers <[email protected]>; Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>
Robert Haas <[email protected]> writes:
> 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.
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*.
> 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.
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
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 13:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 14:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 15:33 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 15:46 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 16:16 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 16:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 17:21 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 17:36 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
@ 2022-04-14 18:03 ` Andres Freund <[email protected]>
1 sibling, 0 replies; 27+ messages in thread
From: Andres Freund @ 2022-04-14 18:03 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; Justin Pryzby <[email protected]>; Kyotaro Horiguchi <[email protected]>; Erik Rijkers <[email protected]>; Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi,
On 2022-04-14 13:36:51 -0400, Tom Lane wrote:
> 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.
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
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 13:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 14:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 15:33 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 15:46 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 16:16 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 16:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 17:21 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 17:36 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
@ 2022-04-14 18:19 ` Robert Haas <[email protected]>
2022-04-14 19:05 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
1 sibling, 1 reply; 27+ messages in thread
From: Robert Haas @ 2022-04-14 18:19 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; Kyotaro Horiguchi <[email protected]>; Erik Rijkers <[email protected]>; Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Apr 14, 2022 at 1:36 PM Tom Lane <[email protected]> wrote:
> Robert Haas <[email protected]> writes:
> > 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.
>
> 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*.
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.
> > 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.
>
> 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?
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.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 13:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 14:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 15:33 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 15:46 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 16:16 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 16:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 17:21 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 17:36 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 18:19 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
@ 2022-04-14 19:05 ` Tom Lane <[email protected]>
2022-04-14 19:55 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Tom Lane @ 2022-04-14 19:05 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; Kyotaro Horiguchi <[email protected]>; Erik Rijkers <[email protected]>; Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>
Robert Haas <[email protected]> writes:
> On Thu, Apr 14, 2022 at 1:36 PM Tom Lane <[email protected]> wrote:
>> 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*.
> 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.
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.
> 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.
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
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 13:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 14:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 15:33 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 15:46 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 16:16 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 16:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 17:21 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 17:36 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 18:19 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 19:05 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
@ 2022-04-14 19:55 ` Robert Haas <[email protected]>
2022-04-14 20:26 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Robert Haas @ 2022-04-14 19:55 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; Kyotaro Horiguchi <[email protected]>; Erik Rijkers <[email protected]>; Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Apr 14, 2022 at 3:05 PM Tom Lane <[email protected]> wrote:
> 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.
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?
> 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?
Hmm, that's a good question. I don't know.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 13:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 14:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 15:33 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 15:46 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 16:16 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 16:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 17:21 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 17:36 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 18:19 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 19:05 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 19:55 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
@ 2022-04-14 20:26 ` Tom Lane <[email protected]>
2022-04-15 12:47 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-16 18:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
0 siblings, 2 replies; 27+ messages in thread
From: Tom Lane @ 2022-04-14 20:26 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; Kyotaro Horiguchi <[email protected]>; Erik Rijkers <[email protected]>; Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>
Robert Haas <[email protected]> writes:
> On Thu, Apr 14, 2022 at 3:05 PM Tom Lane <[email protected]> wrote:
>> If you don't register it, then you need to also make sure that it's
>> destroyed whenever that older snapshot is.
> 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?
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
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 13:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 14:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 15:33 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 15:46 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 16:16 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 16:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 17:21 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 17:36 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 18:19 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 19:05 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 19:55 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 20:26 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
@ 2022-04-15 12:47 ` Robert Haas <[email protected]>
1 sibling, 0 replies; 27+ messages in thread
From: Robert Haas @ 2022-04-15 12:47 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; Kyotaro Horiguchi <[email protected]>; Erik Rijkers <[email protected]>; Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>
On Thu, Apr 14, 2022 at 4:26 PM Tom Lane <[email protected]> wrote:
>
> > 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?
>
> 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.
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.
> 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.
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.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 13:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 14:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 15:33 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 15:46 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 16:16 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 16:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 17:21 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 17:36 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 18:19 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 19:05 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 19:55 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 20:26 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
@ 2022-04-16 18:42 ` Tom Lane <[email protected]>
2022-04-17 13:31 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-18 13:22 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
1 sibling, 2 replies; 27+ messages in thread
From: Tom Lane @ 2022-04-16 18:42 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; Kyotaro Horiguchi <[email protected]>; Erik Rijkers <[email protected]>; Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>
Robert Haas <[email protected]> writes:
> 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.
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
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 13:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 14:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 15:33 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 15:46 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 16:16 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 16:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 17:21 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 17:36 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 18:19 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 19:05 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 19:55 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 20:26 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-16 18:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
@ 2022-04-17 13:31 ` Andres Freund <[email protected]>
2022-04-17 15:51 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
1 sibling, 1 reply; 27+ messages in thread
From: Andres Freund @ 2022-04-17 13:31 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; Justin Pryzby <[email protected]>; Kyotaro Horiguchi <[email protected]>; Erik Rijkers <[email protected]>; Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi,
On 2022-04-16 14:42:39 -0400, Tom Lane wrote:
> In the meantime, since it's clear that HaveRegisteredOrActiveSnapshot
> 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
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 13:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 14:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 15:33 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 15:46 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 16:16 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 16:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 17:21 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 17:36 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 18:19 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 19:05 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 19:55 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 20:26 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-16 18:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-17 13:31 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
@ 2022-04-17 15:51 ` Tom Lane <[email protected]>
2022-04-18 14:39 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
0 siblings, 1 reply; 27+ messages in thread
From: Tom Lane @ 2022-04-17 15:51 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Robert Haas <[email protected]>; Justin Pryzby <[email protected]>; Kyotaro Horiguchi <[email protected]>; Erik Rijkers <[email protected]>; Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>
Andres Freund <[email protected]> writes:
> On 2022-04-16 14:42:39 -0400, Tom Lane wrote:
>> 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...
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
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 13:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 14:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 15:33 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 15:46 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 16:16 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 16:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 17:21 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 17:36 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 18:19 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 19:05 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 19:55 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 20:26 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-16 18:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-17 13:31 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-17 15:51 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
@ 2022-04-18 14:39 ` Andres Freund <[email protected]>
2022-04-18 14:51 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-18 20:06 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
0 siblings, 2 replies; 27+ messages in thread
From: Andres Freund @ 2022-04-18 14:39 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; Justin Pryzby <[email protected]>; Kyotaro Horiguchi <[email protected]>; Erik Rijkers <[email protected]>; Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>
Hi,
On 2022-04-17 11:51:58 -0400, Tom Lane wrote:
> Andres Freund <[email protected]> writes:
> > On 2022-04-16 14:42:39 -0400, Tom Lane wrote:
> >> 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...
>
> 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.
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
> CatalogSnapshot or not changes things in any meaningful way.)
It is a meaningful difference, see e.g. the bug referenced above.
> 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.
Yes.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 13:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 14:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 15:33 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 15:46 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 16:16 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 16:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 17:21 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 17:36 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 18:19 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 19:05 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 19:55 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 20:26 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-16 18:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-17 13:31 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-17 15:51 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-18 14:39 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
@ 2022-04-18 14:51 ` Tom Lane <[email protected]>
1 sibling, 0 replies; 27+ messages in thread
From: Tom Lane @ 2022-04-18 14:51 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Robert Haas <[email protected]>; Justin Pryzby <[email protected]>; Kyotaro Horiguchi <[email protected]>; Erik Rijkers <[email protected]>; Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>
Andres Freund <[email protected]> writes:
> On 2022-04-17 11:51:58 -0400, Tom Lane wrote:
>> 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
>> CatalogSnapshot or not changes things in any meaningful way.)
> 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.)
>> 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.
> Yes.
OK, I'll update the open-items page.
regards, tom lane
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 13:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 14:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 15:33 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 15:46 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 16:16 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 16:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 17:21 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 17:36 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 18:19 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 19:05 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 19:55 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 20:26 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-16 18:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-17 13:31 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-17 15:51 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-18 14:39 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
@ 2022-04-18 20:06 ` Tom Lane <[email protected]>
1 sibling, 0 replies; 27+ messages in thread
From: Tom Lane @ 2022-04-18 20:06 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Robert Haas <[email protected]>; Justin Pryzby <[email protected]>; Kyotaro Horiguchi <[email protected]>; Erik Rijkers <[email protected]>; Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>
Andres Freund <[email protected]> writes:
> On 2022-04-17 11:51:58 -0400, Tom Lane wrote:
>> 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.
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
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 13:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 14:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 15:33 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 15:46 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 16:16 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 16:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 17:21 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 17:36 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 18:19 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 19:05 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 19:55 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 20:26 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-16 18:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
@ 2022-04-18 13:22 ` Robert Haas <[email protected]>
1 sibling, 0 replies; 27+ messages in thread
From: Robert Haas @ 2022-04-18 13:22 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; Kyotaro Horiguchi <[email protected]>; Erik Rijkers <[email protected]>; Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>
On Sat, Apr 16, 2022 at 2:42 PM Tom Lane <[email protected]> wrote:
> 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.
OK, thanks for trying that.
> 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.
+1.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 27+ messages in thread
* Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403)
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
2022-04-14 13:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 14:42 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 15:33 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 15:46 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Tom Lane <[email protected]>
2022-04-14 16:16 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Robert Haas <[email protected]>
2022-04-14 16:54 ` Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Andres Freund <[email protected]>
@ 2022-04-14 18:47 ` Tom Lane <[email protected]>
2 siblings, 0 replies; 27+ messages in thread
From: Tom Lane @ 2022-04-14 18:47 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Robert Haas <[email protected]>; Justin Pryzby <[email protected]>; Kyotaro Horiguchi <[email protected]>; Erik Rijkers <[email protected]>; Matthias van de Meent <[email protected]>; PostgreSQL Hackers <[email protected]>
Andres Freund <[email protected]> writes:
> On 2022-04-14 12:16:45 -0400, Robert Haas wrote:
>> 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;
> 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
^ permalink raw reply [nested|flat] 27+ messages in thread
* [PATCH v5a 6/7] Vacuum first pass uses Streaming Read interface
@ 2023-12-31 16:29 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 27+ messages in thread
From: Melanie Plageman @ 2023-12-31 16:29 UTC (permalink / raw)
Now vacuum's first pass, which HOT prunes and records the TIDs of
non-removable dead tuples, uses the streaming read API by implementing a
streaming read callback which invokes heap_vac_scan_get_next_block().
---
src/backend/access/heap/vacuumlazy.c | 79 +++++++++++++++++++++-------
1 file changed, 59 insertions(+), 20 deletions(-)
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 65d257aab83..fbbc87938e4 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -54,6 +54,7 @@
#include "storage/bufmgr.h"
#include "storage/freespace.h"
#include "storage/lmgr.h"
+#include "storage/streaming_read.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
#include "utils/pg_rusage.h"
@@ -168,7 +169,12 @@ typedef struct LVRelState
char *relnamespace;
char *relname;
char *indname; /* Current index name */
- BlockNumber blkno; /* used only for heap operations */
+
+ /*
+ * The current block being processed by vacuum. Used only for heap
+ * operations. Primarily for error reporting and logging.
+ */
+ BlockNumber blkno;
OffsetNumber offnum; /* used only for heap operations */
VacErrPhase phase;
bool verbose; /* VACUUM VERBOSE? */
@@ -189,6 +195,12 @@ typedef struct LVRelState
BlockNumber missed_dead_pages; /* # pages with missed dead tuples */
BlockNumber nonempty_pages; /* actually, last nonempty page + 1 */
+ /*
+ * The most recent block submitted in the streaming read callback by the
+ * first vacuum pass.
+ */
+ BlockNumber blkno_prefetch;
+
/* Statistics output by us, for table */
double new_rel_tuples; /* new estimated total # of tuples */
double new_live_tuples; /* new estimated total # of live tuples */
@@ -232,7 +244,7 @@ typedef struct LVSavedErrInfo
/* non-export function prototypes */
static void lazy_scan_heap(LVRelState *vacrel);
-static bool heap_vac_scan_get_next_block(LVRelState *vacrel, BlockNumber next_block,
+static void heap_vac_scan_get_next_block(LVRelState *vacrel, BlockNumber next_block,
BlockNumber *blkno,
bool *all_visible_according_to_vm);
static bool lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf,
@@ -416,6 +428,9 @@ heap_vacuum_rel(Relation rel, VacuumParams *params,
vacrel->nonempty_pages = 0;
/* dead_items_alloc allocates vacrel->dead_items later on */
+ /* relies on InvalidBlockNumber overflowing to 0 */
+ vacrel->blkno_prefetch = InvalidBlockNumber;
+
/* Allocate/initialize output statistics state */
vacrel->new_rel_tuples = 0;
vacrel->new_live_tuples = 0;
@@ -776,6 +791,22 @@ heap_vacuum_rel(Relation rel, VacuumParams *params,
}
}
+static BlockNumber
+vacuum_scan_pgsr_next(PgStreamingRead *pgsr,
+ void *pgsr_private, void *per_buffer_data)
+{
+ LVRelState *vacrel = pgsr_private;
+ bool *all_visible_according_to_vm = per_buffer_data;
+
+ vacrel->blkno_prefetch++;
+
+ heap_vac_scan_get_next_block(vacrel,
+ vacrel->blkno_prefetch, &vacrel->blkno_prefetch,
+ all_visible_according_to_vm);
+
+ return vacrel->blkno_prefetch;
+}
+
/*
* lazy_scan_heap() -- workhorse function for VACUUM
*
@@ -815,12 +846,11 @@ heap_vacuum_rel(Relation rel, VacuumParams *params,
static void
lazy_scan_heap(LVRelState *vacrel)
{
+ Buffer buf;
BlockNumber rel_pages = vacrel->rel_pages,
next_fsm_block_to_vacuum = 0;
- bool all_visible_according_to_vm;
+ bool *all_visible_according_to_vm;
- /* relies on InvalidBlockNumber overflowing to 0 */
- BlockNumber blkno = InvalidBlockNumber;
VacDeadItems *dead_items = vacrel->dead_items;
const int initprog_index[] = {
PROGRESS_VACUUM_PHASE,
@@ -828,6 +858,11 @@ lazy_scan_heap(LVRelState *vacrel)
PROGRESS_VACUUM_MAX_DEAD_TUPLES
};
int64 initprog_val[3];
+ PgStreamingRead *pgsr;
+
+ pgsr = pg_streaming_read_buffer_alloc(PGSR_FLAG_MAINTENANCE, vacrel,
+ sizeof(bool), vacrel->bstrategy, BMR_REL(vacrel->rel),
+ MAIN_FORKNUM, vacuum_scan_pgsr_next);
/* Report that we're scanning the heap, advertising total # of blocks */
initprog_val[0] = PROGRESS_VACUUM_PHASE_SCAN_HEAP;
@@ -838,13 +873,19 @@ lazy_scan_heap(LVRelState *vacrel)
vacrel->skip.next_unskippable_block = InvalidBlockNumber;
vacrel->skip.vmbuffer = InvalidBuffer;
- while (heap_vac_scan_get_next_block(vacrel, blkno + 1,
- &blkno, &all_visible_according_to_vm))
+ while (BufferIsValid(buf =
+ pg_streaming_read_buffer_get_next(pgsr, (void **) &all_visible_according_to_vm)))
{
- Buffer buf;
Page page;
bool has_lpdead_items;
bool got_cleanup_lock = false;
+ BlockNumber blkno;
+
+ vacrel->blkno = blkno = BufferGetBlockNumber(buf);
+
+ CheckBufferIsPinnedOnce(buf);
+
+ page = BufferGetPage(buf);
vacrel->scanned_pages++;
@@ -912,9 +953,6 @@ lazy_scan_heap(LVRelState *vacrel)
*/
visibilitymap_pin(vacrel->rel, blkno, &vacrel->skip.vmbuffer);
- buf = ReadBufferExtended(vacrel->rel, MAIN_FORKNUM, blkno, RBM_NORMAL,
- vacrel->bstrategy);
- page = BufferGetPage(buf);
/*
* We need a buffer cleanup lock to prune HOT chains and defragment
@@ -970,7 +1008,7 @@ lazy_scan_heap(LVRelState *vacrel)
*/
if (got_cleanup_lock)
lazy_scan_prune(vacrel, buf, blkno, page,
- all_visible_according_to_vm,
+ *all_visible_according_to_vm,
&has_lpdead_items);
/*
@@ -1027,7 +1065,7 @@ lazy_scan_heap(LVRelState *vacrel)
}
/* report that everything is now scanned */
- pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, blkno);
+ pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_SCANNED, vacrel->rel_pages);
/* now we can compute the new value for pg_class.reltuples */
vacrel->new_live_tuples = vac_estimate_reltuples(vacrel->rel, rel_pages,
@@ -1042,6 +1080,8 @@ lazy_scan_heap(LVRelState *vacrel)
Max(vacrel->new_live_tuples, 0) + vacrel->recently_dead_tuples +
vacrel->missed_dead_tuples;
+ pg_streaming_read_free(pgsr);
+
/*
* Do index vacuuming (call each index's ambulkdelete routine), then do
* related heap vacuuming
@@ -1053,11 +1093,11 @@ lazy_scan_heap(LVRelState *vacrel)
* Vacuum the remainder of the Free Space Map. We must do this whether or
* not there were indexes, and whether or not we bypassed index vacuuming.
*/
- if (blkno > next_fsm_block_to_vacuum)
- FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum, blkno);
+ if (vacrel->rel_pages > next_fsm_block_to_vacuum)
+ FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum, vacrel->rel_pages);
/* report all blocks vacuumed */
- pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_VACUUMED, blkno);
+ pgstat_progress_update_param(PROGRESS_VACUUM_HEAP_BLKS_VACUUMED, vacrel->rel_pages);
/* Do final index cleanup (call each index's amvacuumcleanup routine) */
if (vacrel->nindexes > 0 && vacrel->do_index_cleanup)
@@ -1090,7 +1130,7 @@ lazy_scan_heap(LVRelState *vacrel)
*
* The block number and visibility status of the next block to process are set
* in blkno and all_visible_according_to_vm. heap_vac_scan_get_next_block()
- * returns false if there are no further blocks to process.
+ * sets blkno to InvalidBlockNumber if there are no further blocks to process.
*
* vacrel is an in/out parameter here; vacuum options and information about the
* relation are read and vacrel->skippedallvis is set to ensure we don't
@@ -1110,7 +1150,7 @@ lazy_scan_heap(LVRelState *vacrel)
* older XIDs/MXIDs. The vacrel->skippedallvis flag will be set here when the
* choice to skip such a range is actually made, making everything safe.)
*/
-static bool
+static void
heap_vac_scan_get_next_block(LVRelState *vacrel, BlockNumber next_block,
BlockNumber *blkno, bool *all_visible_according_to_vm)
{
@@ -1119,7 +1159,7 @@ heap_vac_scan_get_next_block(LVRelState *vacrel, BlockNumber next_block,
if (next_block >= vacrel->rel_pages)
{
*blkno = InvalidBlockNumber;
- return false;
+ return;
}
if (vacrel->skip.next_unskippable_block == InvalidBlockNumber ||
@@ -1214,7 +1254,6 @@ heap_vac_scan_get_next_block(LVRelState *vacrel, BlockNumber next_block,
*all_visible_according_to_vm = true;
*blkno = next_block;
- return true;
}
/*
--
2.40.1
--lb5e4aqx4b6ik5lq
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v5a-0007-Vacuum-second-pass-uses-Streaming-Read-interface.patch"
^ permalink raw reply [nested|flat] 27+ messages in thread
end of thread, other threads:[~2023-12-31 16:29 UTC | newest]
Thread overview: 27+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-04-13 20:28 Re: TRAP: FailedAssertion("HaveRegisteredOrActiveSnapshot()", File: "toast_internals.c", Line: 670, PID: 19403) Justin Pryzby <[email protected]>
2022-04-14 00:38 ` Andres Freund <[email protected]>
2022-04-14 13:54 ` Robert Haas <[email protected]>
2022-04-14 14:42 ` Tom Lane <[email protected]>
2022-04-14 15:33 ` Robert Haas <[email protected]>
2022-04-14 15:41 ` Andres Freund <[email protected]>
2022-04-14 15:46 ` Tom Lane <[email protected]>
2022-04-14 16:16 ` Robert Haas <[email protected]>
2022-04-14 16:54 ` Andres Freund <[email protected]>
2022-04-14 16:58 ` Andres Freund <[email protected]>
2022-04-14 17:21 ` Robert Haas <[email protected]>
2022-04-14 17:36 ` Tom Lane <[email protected]>
2022-04-14 18:03 ` Andres Freund <[email protected]>
2022-04-14 18:19 ` Robert Haas <[email protected]>
2022-04-14 19:05 ` Tom Lane <[email protected]>
2022-04-14 19:55 ` Robert Haas <[email protected]>
2022-04-14 20:26 ` Tom Lane <[email protected]>
2022-04-15 12:47 ` Robert Haas <[email protected]>
2022-04-16 18:42 ` Tom Lane <[email protected]>
2022-04-17 13:31 ` Andres Freund <[email protected]>
2022-04-17 15:51 ` Tom Lane <[email protected]>
2022-04-18 14:39 ` Andres Freund <[email protected]>
2022-04-18 14:51 ` Tom Lane <[email protected]>
2022-04-18 20:06 ` Tom Lane <[email protected]>
2022-04-18 13:22 ` Robert Haas <[email protected]>
2022-04-14 18:47 ` Tom Lane <[email protected]>
2023-12-31 16:29 [PATCH v5a 6/7] Vacuum first pass uses Streaming Read interface Melanie Plageman <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox