Thread: getting rid of SnapshotNow
There seems to be a consensus that we should try to get rid of SnapshotNow entirely now that we have MVCC catalog scans, so I'm attaching two patches that together come close to achieving that goal: 1. snapshot-error-v1.patch introduces a new special snapshot, called SnapshotError. In the cases where we set SnapshotNow as a sort of default snapshot, this patch changes the code to use SnapshotError instead. This affects scan->xs_snapshot in genam.c and estate->es_snapshot in execUtils.c. This passes make check-world, so apparently there is no code in the core distribution that does this. However, this is safer for third-party code, which will ERROR instead of seg faulting. The alternative approach would be to use InvalidSnapshot, which I think would be OK too if people dislike this approach. 2. snapshot-self-not-now-v1.patch changes several uses of SnapshotNow to use SnapshotSelf instead. These include pgrowlocks(), pgstat_heap(), and get_actual_variable_range(). In all of those cases, only an approximately-correct answer is needed, so the change should be fine. I'd also generally expect that it's very unlikely for any of these things to get called in a context where the table being scanned has been updated by the current transaction after the most recent command-counter increment, so in practice the change in semantics will probably not be noticeable at all. Barring objections, I'll commit both of these next week. With that done, the only remaining uses of SnapshotNow in our code base will be in currtid_byreloid() and currtid_byrelname(). So far no one on this list has been able to understand clearly what the purpose of those functions is, so I'm copying this email to pgsql-odbc in case someone there can provide more insight. If I were a betting man, I'd bet that they are used in contexts where the difference between SnapshotNow and SnapshotSelf wouldn't matter there, either. For example, if those functions are always invoked in a query that does nothing but call those functions, the difference wouldn't be visible. If we don't want to risk any change to the semantics, we can (1) grit our teeth and keep SnapshotNow around or (2) use an instantaneous MVCC snapshot there, and accept that people who have very large connection counts and extremely heavy use of those functions may see a performance regression. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
Robert Haas <robertmhaas@gmail.com> writes:
> 1. snapshot-error-v1.patch introduces a new special snapshot, called
> SnapshotError. In the cases where we set SnapshotNow as a sort of
> default snapshot, this patch changes the code to use SnapshotError
> instead. This affects scan->xs_snapshot in genam.c and
> estate->es_snapshot in execUtils.c. This passes make check-world, so
> apparently there is no code in the core distribution that does this.
> However, this is safer for third-party code, which will ERROR instead
> of seg faulting. The alternative approach would be to use
> InvalidSnapshot, which I think would be OK too if people dislike this
> approach.
FWIW, I think using InvalidSnapshot would be preferable to introducing
a new concept for what's pretty much the same thing.
> With that done, the only remaining uses of SnapshotNow in our code
> base will be in currtid_byreloid() and currtid_byrelname(). So far no
> one on this list has been able to understand clearly what the purpose
> of those functions is, so I'm copying this email to pgsql-odbc in case
> someone there can provide more insight.
I had the idea they were used for a client-side implementation of WHERE
CURRENT OF. Perhaps that's dead code and could be removed entirely?
> If we don't want to risk any change to the semantics, we can (1) grit
> our teeth and keep SnapshotNow around or (2) use an instantaneous MVCC
> snapshot there, and accept that people who have very large connection
> counts and extremely heavy use of those functions may see a
> performance regression.
Of those I'd go for (2); it's unlikely that an extra snapshot would be
visible compared to the query roundtrip overhead. But another attractive
possibility is to make these functions use GetActiveSnapshot(), which
would avoid taking any new snapshot.
regards, tom lane
On Thu, Jul 18, 2013 at 10:34 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> 1. snapshot-error-v1.patch introduces a new special snapshot, called >> SnapshotError. In the cases where we set SnapshotNow as a sort of >> default snapshot, this patch changes the code to use SnapshotError >> instead. This affects scan->xs_snapshot in genam.c and >> estate->es_snapshot in execUtils.c. This passes make check-world, so >> apparently there is no code in the core distribution that does this. >> However, this is safer for third-party code, which will ERROR instead >> of seg faulting. The alternative approach would be to use >> InvalidSnapshot, which I think would be OK too if people dislike this >> approach. > > FWIW, I think using InvalidSnapshot would be preferable to introducing > a new concept for what's pretty much the same thing. Andres voted the other way on the previous thread. I'll wait and see if there are any other opinions. The SnapshotError concept seemed attractive to me initially, but I'm not as excited about it after seeing how it turned out, so maybe it's best to do it as you suggest. >> With that done, the only remaining uses of SnapshotNow in our code >> base will be in currtid_byreloid() and currtid_byrelname(). So far no >> one on this list has been able to understand clearly what the purpose >> of those functions is, so I'm copying this email to pgsql-odbc in case >> someone there can provide more insight. > > I had the idea they were used for a client-side implementation of WHERE > CURRENT OF. Perhaps that's dead code and could be removed entirely? It's been reported that ODBC still uses them. >> If we don't want to risk any change to the semantics, we can (1) grit >> our teeth and keep SnapshotNow around or (2) use an instantaneous MVCC >> snapshot there, and accept that people who have very large connection >> counts and extremely heavy use of those functions may see a >> performance regression. > > Of those I'd go for (2); it's unlikely that an extra snapshot would be > visible compared to the query roundtrip overhead. But another attractive > possibility is to make these functions use GetActiveSnapshot(), which > would avoid taking any new snapshot. You could probably construct a case where the overhead is visible, if you ran the functions many times in a single query, but arguably no one does that. Also, Andres's test case that involves running BEGIN; SELECT txid_current(); very short sleep; COMMIT; in several hundred sessions at once is pretty brutal on PGXACT and makes the overhead of taking extra snapshots a lot more visible. I'm not too familiar with GetActiveSnapshot(), but wouldn't that change the user-visible semantics? If, for example, someone's using that function to test whether the row has been updated since their snapshot was taken, that use case would get broken. SnapshotSelf would be change from the current behavior in many fewer cases than using an older snapshot. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas escribió: > On Thu, Jul 18, 2013 at 10:34 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Robert Haas <robertmhaas@gmail.com> writes: > >> 1. snapshot-error-v1.patch introduces a new special snapshot, called > >> SnapshotError. In the cases where we set SnapshotNow as a sort of > >> default snapshot, this patch changes the code to use SnapshotError > >> instead. This affects scan->xs_snapshot in genam.c and > >> estate->es_snapshot in execUtils.c. This passes make check-world, so > >> apparently there is no code in the core distribution that does this. > >> However, this is safer for third-party code, which will ERROR instead > >> of seg faulting. The alternative approach would be to use > >> InvalidSnapshot, which I think would be OK too if people dislike this > >> approach. > > > > FWIW, I think using InvalidSnapshot would be preferable to introducing > > a new concept for what's pretty much the same thing. > > Andres voted the other way on the previous thread. I'll wait and see > if there are any other opinions. The SnapshotError concept seemed > attractive to me initially, but I'm not as excited about it after > seeing how it turned out, so maybe it's best to do it as you suggest. Yeah ... SnapshotError is a way to ensure the server doesn't crash if an extension hasn't been fixed in order not to cause a crash if it doesn't use the APIs correctly. However, there's many other ways for a C-language extension to cause crashes, so I don't think this is buying us much. > >> With that done, the only remaining uses of SnapshotNow in our code > >> base will be in currtid_byreloid() and currtid_byrelname(). So far no > >> one on this list has been able to understand clearly what the purpose > >> of those functions is, so I'm copying this email to pgsql-odbc in case > >> someone there can provide more insight. > > > > I had the idea they were used for a client-side implementation of WHERE > > CURRENT OF. Perhaps that's dead code and could be removed entirely? > > It's been reported that ODBC still uses them. They don't show up in a quick grep of psqlodbc's source code, FWIW. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Thu, Jul 18, 2013 at 11:54 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > They don't show up in a quick grep of psqlodbc's source code, FWIW. Hmm. Maybe we should just remove them and see if anyone complains. We could always put them back (or make them available via contrib) if it's functionality someone actually needs. The last discussion of those functions was in 2007 and nobody seemed too sure back then either, so maybe the rumor that anyone is actually using this is no more than rumor. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2013-07-18 12:01:39 -0400, Robert Haas wrote: > On Thu, Jul 18, 2013 at 11:54 AM, Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: > > They don't show up in a quick grep of psqlodbc's source code, FWIW. > > Hmm. Maybe we should just remove them and see if anyone complains. > We could always put them back (or make them available via contrib) if > it's functionality someone actually needs. The last discussion of > those functions was in 2007 and nobody seemed too sure back then > either, so maybe the rumor that anyone is actually using this is no > more than rumor. I am pretty sure they are still used. A quick grep on a not too old checkout prooves that... Note that the sql accessible functions are named currtid and currtid2 (yes, really)... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund escribió: > On 2013-07-18 12:01:39 -0400, Robert Haas wrote: > > On Thu, Jul 18, 2013 at 11:54 AM, Alvaro Herrera > > <alvherre@2ndquadrant.com> wrote: > > > They don't show up in a quick grep of psqlodbc's source code, FWIW. > > > > Hmm. Maybe we should just remove them and see if anyone complains. > > We could always put them back (or make them available via contrib) if > > it's functionality someone actually needs. The last discussion of > > those functions was in 2007 and nobody seemed too sure back then > > either, so maybe the rumor that anyone is actually using this is no > > more than rumor. > > I am pretty sure they are still used. A quick grep on a not too old > checkout prooves that... Note that the sql accessible functions are > named currtid and currtid2 (yes, really)... Ah, yeah, that does show up. I had grepped for 'currtid_'. Sorry. They're all in positioned_load() in results.c. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Thu, Jul 18, 2013 at 12:25 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Ah, yeah, that does show up. I had grepped for 'currtid_'. Sorry. > They're all in positioned_load() in results.c. Well, in that case, we'll have to keep it around. I still wish we could get a clear answer to the question of how it's being used. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
(2013/07/18 23:54), Robert Haas wrote:
> On Thu, Jul 18, 2013 at 10:34 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> 1. snapshot-error-v1.patch introduces a new special snapshot, called
>>> SnapshotError. In the cases where we set SnapshotNow as a sort of
>>> default snapshot, this patch changes the code to use SnapshotError
>>> instead. This affects scan->xs_snapshot in genam.c and
>>> estate->es_snapshot in execUtils.c. This passes make check-world, so
>>> apparently there is no code in the core distribution that does this.
>>> However, this is safer for third-party code, which will ERROR instead
>>> of seg faulting. The alternative approach would be to use
>>> InvalidSnapshot, which I think would be OK too if people dislike this
>>> approach.
>>
>> FWIW, I think using InvalidSnapshot would be preferable to introducing
>> a new concept for what's pretty much the same thing.
>
> Andres voted the other way on the previous thread. I'll wait and see
> if there are any other opinions. The SnapshotError concept seemed
> attractive to me initially, but I'm not as excited about it after
> seeing how it turned out, so maybe it's best to do it as you suggest.
>
>>> With that done, the only remaining uses of SnapshotNow in our code
>>> base will be in currtid_byreloid() and currtid_byrelname(). So far no
>>> one on this list has been able to understand clearly what the purpose
>>> of those functions is, so I'm copying this email to pgsql-odbc in case
>>> someone there can provide more insight.
>>
>> I had the idea they were used for a client-side implementation of WHERE
>> CURRENT OF. Perhaps that's dead code and could be removed entirely?
>
> It's been reported that ODBC still uses them.
Though PostgreSQL's TID is similar to Orale's ROWID, it is transient
and changed after update operations unfortunately. I implemented
the currtid_xx functions to supplement the difference. For example
currtid(relname, original tid)
(hopefully) returns the current tid of the original row when it is
updated.
regards,
Hiroshi Inoue
On Thu, Jul 18, 2013 at 08:46:48AM -0400, Robert Haas wrote: > 1. snapshot-error-v1.patch introduces a new special snapshot, called > SnapshotError. In the cases where we set SnapshotNow as a sort of > default snapshot, this patch changes the code to use SnapshotError > instead. This affects scan->xs_snapshot in genam.c and > estate->es_snapshot in execUtils.c. This passes make check-world, so > apparently there is no code in the core distribution that does this. > However, this is safer for third-party code, which will ERROR instead > of seg faulting. The alternative approach would be to use > InvalidSnapshot, which I think would be OK too if people dislike this > approach. I don't have a strong opinion. Anything it diagnoses is a code bug, probably one that makes the affected extension useless until it's fixed. But the patch is small and self-contained. I think the benefit, more than making things safer in production, would be reducing the amount of time the developer needs to zero in on the problem. It wouldn't be the first time we've done that; compare AtEOXact_Buffers(). Does this particular class of bug deserve that aid? I don't know. > 2. snapshot-self-not-now-v1.patch changes several uses of SnapshotNow > to use SnapshotSelf instead. These include pgrowlocks(), > pgstat_heap(), and get_actual_variable_range(). In all of those > cases, only an approximately-correct answer is needed, so the change > should be fine. I'd also generally expect that it's very unlikely for > any of these things to get called in a context where the table being > scanned has been updated by the current transaction after the most > recent command-counter increment, so in practice the change in > semantics will probably not be noticeable at all. SnapshotSelf is awfully special; currently, you can grep for all uses of it and find a collection of callers with highly-technical needs. Diluting that with a handful of callers that legitimately preferred SnapshotNow but don't care enough to mind SnapshotSelf in its place brings a minor loss of clarity. From an accuracy perspective, GetActiveSnapshot() does seem ideal for get_actual_variable_range(). That's independent of any hurry to remove SnapshotNow. A possible disadvantage is that older snapshots could waste time scanning back through newer index entries, when a more-accessible value would be good enough for estimation purposes. To me, the major advantage of removing SnapshotNow is to force all third-party code to reevaluate. But that could be just as well achieved by renaming it to, say, SnapshotImmediate. If there are borderline-legitimate SnapshotNow uses in our code base, I'd lean toward a rename instead. Even if we decide to remove every core use, third-party code might legitimately reach a different conclusion on similar borderline cases. Thanks, nm -- Noah Misch EnterpriseDB http://www.enterprisedb.com
Noah Misch <noah@leadboat.com> writes:
> To me, the major advantage of removing SnapshotNow is to force all
> third-party code to reevaluate. But that could be just as well
> achieved by renaming it to, say, SnapshotImmediate. If there are
> borderline-legitimate SnapshotNow uses in our code base, I'd lean
> toward a rename instead. Even if we decide to remove every core use,
> third-party code might legitimately reach a different conclusion on
> similar borderline cases.
Meh. If there is third-party code with a legitimate need for
SnapshotNow, all we'll have done is to create an annoying version
dependency for them. So if we think that's actually a likely scenario,
we shouldn't rename it. But the entire point of this change IMO is that
we *don't* think there is a legitimate use-case for SnapshotNow.
Indeed, I'm thinking I don't believe in SnapshotSelf anymore either.
It's got all the same consistency issues as SnapshotNow. In fact, it
has *more* issues, because it's also vulnerable to weirdnesses caused by
inconsistent ordering of tuple updates among multiple tuples updated by
the same command.
Why not tell people to use SnapshotDirty if they need a
not-guaranteed-consistent result? At least then it's pretty obvious
that you're getting some randomness in with your news.
regards, tom lane
On Fri, Jul 19, 2013 at 1:27 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Noah Misch <noah@leadboat.com> writes: >> To me, the major advantage of removing SnapshotNow is to force all >> third-party code to reevaluate. But that could be just as well >> achieved by renaming it to, say, SnapshotImmediate. If there are >> borderline-legitimate SnapshotNow uses in our code base, I'd lean >> toward a rename instead. Even if we decide to remove every core use, >> third-party code might legitimately reach a different conclusion on >> similar borderline cases. > > Meh. If there is third-party code with a legitimate need for > SnapshotNow, all we'll have done is to create an annoying version > dependency for them. So if we think that's actually a likely scenario, > we shouldn't rename it. But the entire point of this change IMO is that > we *don't* think there is a legitimate use-case for SnapshotNow. > > Indeed, I'm thinking I don't believe in SnapshotSelf anymore either. > It's got all the same consistency issues as SnapshotNow. In fact, it > has *more* issues, because it's also vulnerable to weirdnesses caused by > inconsistent ordering of tuple updates among multiple tuples updated by > the same command. > > Why not tell people to use SnapshotDirty if they need a > not-guaranteed-consistent result? At least then it's pretty obvious > that you're getting some randomness in with your news. You know, I didn't really consider that before, but I kind of like it. I think that would be entirely suitable (and perhaps better) for pgstattuple and get_actual_variable_range(). On further reflection, I think perhaps pgrowlocks should just register a fresh MVCC snapshot and use that. Using SnapshotDirty would return TIDs of unseen tuples, which does not seem to be what is wanted there. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2013-07-19 08:57:01 +0900, Inoue, Hiroshi wrote: > >>I had the idea they were used for a client-side implementation of WHERE > >>CURRENT OF. Perhaps that's dead code and could be removed entirely? > > > >It's been reported that ODBC still uses them. > > Though PostgreSQL's TID is similar to Orale's ROWID, it is transient > and changed after update operations unfortunately. I implemented > the currtid_xx functions to supplement the difference. For example > > currtid(relname, original tid) > > (hopefully) returns the current tid of the original row when it is > updated. That is only guaranteed to work though when you're in a transaction old enough to prevent removal of the old or intermediate row versions. E.g. BEGIN; INSERT INTO foo...; -- last tid (0, 1) COMMIT; BEGIN; SELECT currtid(foo, '(0, 1')); COMMIT; can basically return no or even an arbitrarily different row. Same with an update... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On 2013-07-19 01:27:41 -0400, Tom Lane wrote: > Noah Misch <noah@leadboat.com> writes: > > To me, the major advantage of removing SnapshotNow is to force all > > third-party code to reevaluate. But that could be just as well > > achieved by renaming it to, say, SnapshotImmediate. If there are > > borderline-legitimate SnapshotNow uses in our code base, I'd lean > > toward a rename instead. Even if we decide to remove every core use, > > third-party code might legitimately reach a different conclusion on > > similar borderline cases. I don't think there are many people that aren't active on -hackers that can actually understand the implications of using SnapshotNow. Given -hackers hasn't fully grasped them in several cases... And even if those borderline cases are safe, that's really only valid for a specific postgres version. Catering to that doesn't seem like a good idea to me. > Indeed, I'm thinking I don't believe in SnapshotSelf anymore either. > It's got all the same consistency issues as SnapshotNow. In fact, it > has *more* issues, because it's also vulnerable to weirdnesses caused by > inconsistent ordering of tuple updates among multiple tuples updated by > the same command. Hm. I kind of can see the point of it in constraint code where it probably would be rather hard to remove usage of it, but e.g. the sepgsql usage looks pretty dubious to me. At least in the cases where the constraint code uses them I don't think the SnapshotNow dangers apply since those specific rows should be locked et al. The selinux usage looks like a design flaw to me, but I don't really understand that code, so I very well may be wrong. > Why not tell people to use SnapshotDirty if they need a > not-guaranteed-consistent result? At least then it's pretty obvious > that you're getting some randomness in with your news. Especially if we're going to lower the lock level of some commands, but even now, that opens us to more issues due to nonmatching table definitions et al. That doesn't sound like a good idea to me. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
(2013/07/19 22:03), Andres Freund wrote: > On 2013-07-19 08:57:01 +0900, Inoue, Hiroshi wrote: >>>> I had the idea they were used for a client-side implementation of WHERE >>>> CURRENT OF. Perhaps that's dead code and could be removed entirely? >>> >>> It's been reported that ODBC still uses them. >> >> Though PostgreSQL's TID is similar to Orale's ROWID, it is transient >> and changed after update operations unfortunately. I implemented >> the currtid_xx functions to supplement the difference. For example >> >> currtid(relname, original tid) >> >> (hopefully) returns the current tid of the original row when it is >> updated. > > That is only guaranteed to work though when you're in a transaction old > enough to prevent removal of the old or intermediate row versions. E.g. Yes it's what I meant by (hopefully). At the time when I implemented currtid(), I was able to use TIDs in combination with OIDs. regards, Hiroshi Inoue
(2013/07/18 21:46), Robert Haas wrote: > There seems to be a consensus that we should try to get rid of > SnapshotNow entirely now that we have MVCC catalog scans, so I'm > attaching two patches that together come close to achieving that goal: ... > With that done, the only remaining uses of SnapshotNow in our code > base will be in currtid_byreloid() and currtid_byrelname(). So far no > one on this list has been able to understand clearly what the purpose > of those functions is, so I'm copying this email to pgsql-odbc in case > someone there can provide more insight. If I were a betting man, I'd > bet that they are used in contexts where the difference between > SnapshotNow and SnapshotSelf wouldn't matter there, either. Using SnapshotSelf instead of SnapshotNow for currtid_ () wouldn't matter. regards, Hiroshi Inoue
On 2013-07-20 00:49:11 +0900, Hiroshi Inoue wrote: > (2013/07/18 21:46), Robert Haas wrote: > >There seems to be a consensus that we should try to get rid of > >SnapshotNow entirely now that we have MVCC catalog scans, so I'm > >attaching two patches that together come close to achieving that goal: > > ... > > >With that done, the only remaining uses of SnapshotNow in our code > >base will be in currtid_byreloid() and currtid_byrelname(). So far no > >one on this list has been able to understand clearly what the purpose > >of those functions is, so I'm copying this email to pgsql-odbc in case > >someone there can provide more insight. If I were a betting man, I'd > >bet that they are used in contexts where the difference between > >SnapshotNow and SnapshotSelf wouldn't matter there, either. > > Using SnapshotSelf instead of SnapshotNow for currtid_ () wouldn't > matter. I think it actually might. You could get into dicey situations if you use currtid_ in a query performing updates or inserts because it would see the to-be-inserted tuple... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@2ndquadrant.com> writes:
> On 2013-07-20 00:49:11 +0900, Hiroshi Inoue wrote:
>> Using SnapshotSelf instead of SnapshotNow for currtid_ () wouldn't
>> matter.
> I think it actually might. You could get into dicey situations if you
> use currtid_ in a query performing updates or inserts because it would
> see the to-be-inserted tuple...
I'm pretty sure Hiroshi-san was only opining about whether it would
matter for ODBC's usage. IIUC, ODBC is using this function to re-fetch
rows that it inserted, updated, or at least selected-for-update in a
previous command of the current transaction, so actually any snapshot
would do fine.
In any case, since I moved the goalposts by suggesting that SnapshotSelf
is just as dangerous as SnapshotNow, what we need to know is whether
it'd be all right to change this code to use a fresh MVCC snapshot;
and if not, why not. It's pretty hard to see a reason why client-side
code would want to make use of the results of a non-MVCC snapshot.
regards, tom lane
Robert Haas escribió: > On Fri, Jul 19, 2013 at 1:27 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Why not tell people to use SnapshotDirty if they need a > > not-guaranteed-consistent result? At least then it's pretty obvious > > that you're getting some randomness in with your news. > On further reflection, I think perhaps pgrowlocks should just register > a fresh MVCC snapshot and use that. Using SnapshotDirty would return > TIDs of unseen tuples, which does not seem to be what is wanted there. I think seeing otherwise invisible rows is useful in pgrowlocks. It helps observe the effects on tuples written by concurrent transactions during experimentation. But then, maybe this functionality belongs in pageinspect instead. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Fri, Jul 19, 2013 at 12:29 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Robert Haas escribió: >> On Fri, Jul 19, 2013 at 1:27 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> > Why not tell people to use SnapshotDirty if they need a >> > not-guaranteed-consistent result? At least then it's pretty obvious >> > that you're getting some randomness in with your news. > >> On further reflection, I think perhaps pgrowlocks should just register >> a fresh MVCC snapshot and use that. Using SnapshotDirty would return >> TIDs of unseen tuples, which does not seem to be what is wanted there. > > I think seeing otherwise invisible rows is useful in pgrowlocks. It > helps observe the effects on tuples written by concurrent transactions > during experimentation. But then, maybe this functionality belongs in > pageinspect instead. It does seem like it should be useful, at least as an option. But I feel like changing that ought to be separate from getting rid of SnapshotNow. It seems like too big of a behavior change to pass off as a harmless tweak. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas escribió: > On Fri, Jul 19, 2013 at 12:29 PM, Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: > > I think seeing otherwise invisible rows is useful in pgrowlocks. It > > helps observe the effects on tuples written by concurrent transactions > > during experimentation. But then, maybe this functionality belongs in > > pageinspect instead. > > It does seem like it should be useful, at least as an option. But I > feel like changing that ought to be separate from getting rid of > SnapshotNow. It seems like too big of a behavior change to pass off > as a harmless tweak. Agreed. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Fri, Jul 19, 2013 at 12:51 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Robert Haas escribió: >> On Fri, Jul 19, 2013 at 12:29 PM, Alvaro Herrera >> <alvherre@2ndquadrant.com> wrote: > >> > I think seeing otherwise invisible rows is useful in pgrowlocks. It >> > helps observe the effects on tuples written by concurrent transactions >> > during experimentation. But then, maybe this functionality belongs in >> > pageinspect instead. >> >> It does seem like it should be useful, at least as an option. But I >> feel like changing that ought to be separate from getting rid of >> SnapshotNow. It seems like too big of a behavior change to pass off >> as a harmless tweak. > > Agreed. So any change we make to pgrowlocks is going to have some behavior consequences. 1. If we use SnapshotSelf, then nobody will notice the difference unless this is used as part of a query that locks or modifies tuples in the table being examined. But in that case you might see the results of the current query. Thus, I think this is the smallest possible behavior change, but Tom doesn't like SnapshotSelf any more than he likes SnapshotNow. 2. If we use SnapshotDirty, then the difference is probably noticeable, because you'll see the results of concurrent, uncommitted transactions. Maybe useful, but probably shouldn't be the new default. 3. If we use a fresh MVCC snapshot, then when you scan a table you'll see the state of play as of the beginning of your scan rather than the state of play as of when your scan reaches the target page. This might be noticeable on a large table. However, it might also be thought an improvement. 4. If we use GetActiveSnapshot, all the comments about about a fresh MVCC snapshot still apply. However, the snapshot in question could be even more stale, especially in repeatable read or serializable mode. However, this might be thought a more consistent behavior than what we have now. And I'm guessing that this function is typically run as its own transaction, so in practice this doesn't seem much different from an MVCC snapshot, only cheaper. At the moment, I dislike #2 and slightly prefer #4 to #3. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas escribió: > 4. If we use GetActiveSnapshot, all the comments about about a fresh > MVCC snapshot still apply. However, the snapshot in question could be > even more stale, especially in repeatable read or serializable mode. > However, this might be thought a more consistent behavior than what we > have now. And I'm guessing that this function is typically run as its > own transaction, so in practice this doesn't seem much different from > an MVCC snapshot, only cheaper. > > At the moment, I dislike #2 and slightly prefer #4 to #3. +1 for #4, and if we ever need more then we can provide a non-default way to get at #2. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
(2013/07/20 1:11), Tom Lane wrote: > Andres Freund <andres@2ndquadrant.com> writes: >> On 2013-07-20 00:49:11 +0900, Hiroshi Inoue wrote: >>> Using SnapshotSelf instead of SnapshotNow for currtid_ () wouldn't >>> matter. > >> I think it actually might. You could get into dicey situations if you >> use currtid_ in a query performing updates or inserts because it would >> see the to-be-inserted tuple... > > I'm pretty sure Hiroshi-san was only opining about whether it would > matter for ODBC's usage. IIUC, ODBC is using this function to re-fetch > rows that it inserted, updated, or at least selected-for-update in a > previous command of the current transaction, so actually any snapshot > would do fine. > > In any case, since I moved the goalposts by suggesting that SnapshotSelf > is just as dangerous as SnapshotNow, what we need to know is whether > it'd be all right to change this code to use a fresh MVCC snapshot; > and if not, why not. It's pretty hard to see a reason why client-side > code would want to make use of the results of a non-MVCC snapshot. OK I agree to replace SnapshotNow for currtid_xx() by a MVCC-snapshot. regards, Hiroshi Inoue
On Fri, Jul 19, 2013 at 1:20 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: >> 4. If we use GetActiveSnapshot, all the comments about about a fresh >> MVCC snapshot still apply. However, the snapshot in question could be >> even more stale, especially in repeatable read or serializable mode. >> However, this might be thought a more consistent behavior than what we >> have now. And I'm guessing that this function is typically run as its >> own transaction, so in practice this doesn't seem much different from >> an MVCC snapshot, only cheaper. >> >> At the moment, I dislike #2 and slightly prefer #4 to #3. > > +1 for #4, and if we ever need more then we can provide a non-default > way to get at #2. OK, done. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Jul 18, 2013 at 8:46 AM, Robert Haas <robertmhaas@gmail.com> wrote: > There seems to be a consensus that we should try to get rid of > SnapshotNow entirely now that we have MVCC catalog scans, so I'm > attaching two patches that together come close to achieving that goal: > > 1. snapshot-error-v1.patch introduces a new special snapshot, called > SnapshotError. In the cases where we set SnapshotNow as a sort of > default snapshot, this patch changes the code to use SnapshotError > instead. This affects scan->xs_snapshot in genam.c and > estate->es_snapshot in execUtils.c. This passes make check-world, so > apparently there is no code in the core distribution that does this. > However, this is safer for third-party code, which will ERROR instead > of seg faulting. The alternative approach would be to use > InvalidSnapshot, which I think would be OK too if people dislike this > approach. It seems the consensus was mildly for InvalidSnapshot, so I did it that way. > 2. snapshot-self-not-now-v1.patch changes several uses of SnapshotNow > to use SnapshotSelf instead. These include pgrowlocks(), > pgstat_heap(), and get_actual_variable_range(). In all of those > cases, only an approximately-correct answer is needed, so the change > should be fine. I'd also generally expect that it's very unlikely for > any of these things to get called in a context where the table being > scanned has been updated by the current transaction after the most > recent command-counter increment, so in practice the change in > semantics will probably not be noticeable at all. Tom proposed that we use SnapshotDirty for this case; let me just ask whether there are any security concerns around that. pgstattuple only displays aggregate information so I think that's OK, but I wonder if the value found in get_actual_variable_range() can leak out in EXPLAIN output or whatever. I can't particularly think of any reason why that would actually matter, but I've generally shied away from exposing data written by uncommitted transactions, and this would be a step in the other direction. Does this worry anyone else or am I being paranoid? But thinking about it a little more, I wonder why get_actual_variable_range() is using a snapshot at all. Presumably what we want there is to find the last index key, regardless of the visibility of the heap tuple to which it points. We don't really need to consult the heap at all, one would think; the value we need ought to be present in the index tuple. If we're going to use a snapshot for simplicity of coding, maybe the right thing is SnapshotAny. After all, even if the index tuples are all dead, we still have to scan them, so it's still relevant for costing purposes. Thoughts? > With that done, the only remaining uses of SnapshotNow in our code > base will be in currtid_byreloid() and currtid_byrelname(). So far no > one on this list has been able to understand clearly what the purpose > of those functions is, so I'm copying this email to pgsql-odbc in case > someone there can provide more insight. If I were a betting man, I'd > bet that they are used in contexts where the difference between > SnapshotNow and SnapshotSelf wouldn't matter there, either. For > example, if those functions are always invoked in a query that does > nothing but call those functions, the difference wouldn't be visible. > If we don't want to risk any change to the semantics, we can (1) grit > our teeth and keep SnapshotNow around or (2) use an instantaneous MVCC > snapshot there, and accept that people who have very large connection > counts and extremely heavy use of those functions may see a > performance regression. It seems like we're leaning toward a fresh MVCC snapshot for this case. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
>> 2. snapshot-self-not-now-v1.patch changes several uses of SnapshotNow
>> to use SnapshotSelf instead. These include pgrowlocks(),
>> pgstat_heap(), and get_actual_variable_range().
> Tom proposed that we use SnapshotDirty for this case; let me just ask
> whether there are any security concerns around that. pgstattuple only
> displays aggregate information so I think that's OK, but I wonder if
> the value found in get_actual_variable_range() can leak out in EXPLAIN
> output or whatever. I can't particularly think of any reason why that
> would actually matter, but I've generally shied away from exposing
> data written by uncommitted transactions, and this would be a step in
> the other direction. Does this worry anyone else or am I being
> paranoid?
As far as get_actual_variable_range() is concerned, an MVCC snapshot
would probably be the thing to use anyway; I see no need for the planner
to be using estimates that are "more up to date" than that. pgrowlocks
and pgstat_heap() might be in a different category.
> But thinking about it a little more, I wonder why
> get_actual_variable_range() is using a snapshot at all. Presumably
> what we want there is to find the last index key, regardless of the
> visibility of the heap tuple to which it points.
No, what we ideally want is to know the current variable range that
would be seen by the query being planned.
regards, tom lane
On Tue, Jul 23, 2013 at 12:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >>> 2. snapshot-self-not-now-v1.patch changes several uses of SnapshotNow >>> to use SnapshotSelf instead. These include pgrowlocks(), >>> pgstat_heap(), and get_actual_variable_range(). > >> Tom proposed that we use SnapshotDirty for this case; let me just ask >> whether there are any security concerns around that. pgstattuple only >> displays aggregate information so I think that's OK, but I wonder if >> the value found in get_actual_variable_range() can leak out in EXPLAIN >> output or whatever. I can't particularly think of any reason why that >> would actually matter, but I've generally shied away from exposing >> data written by uncommitted transactions, and this would be a step in >> the other direction. Does this worry anyone else or am I being >> paranoid? > > As far as get_actual_variable_range() is concerned, an MVCC snapshot > would probably be the thing to use anyway; I see no need for the planner > to be using estimates that are "more up to date" than that. pgrowlocks > and pgstat_heap() might be in a different category. > >> But thinking about it a little more, I wonder why >> get_actual_variable_range() is using a snapshot at all. Presumably >> what we want there is to find the last index key, regardless of the >> visibility of the heap tuple to which it points. > > No, what we ideally want is to know the current variable range that > would be seen by the query being planned. Oh, really? Well, should we use GetActiveSnapshot() then? That surprises me, though. I really thought the point was to cost the index scan, and surely that will be slowed down even by entries we can't see. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Jul 23, 2013 at 12:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> As far as get_actual_variable_range() is concerned, an MVCC snapshot
>> would probably be the thing to use anyway;
> That surprises me, though. I really thought the point was to cost the
> index scan, and surely that will be slowed down even by entries we
> can't see.
No, the usage (or the main usage anyway) is for selectivity estimation,
ie how many rows will the query fetch.
regards, tom lane
On Tue, Jul 23, 2013 at 2:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Tue, Jul 23, 2013 at 12:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> As far as get_actual_variable_range() is concerned, an MVCC snapshot >>> would probably be the thing to use anyway; > >> That surprises me, though. I really thought the point was to cost the >> index scan, and surely that will be slowed down even by entries we >> can't see. > > No, the usage (or the main usage anyway) is for selectivity estimation, > ie how many rows will the query fetch. OK, so GetActiveSnapshot()? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> OK, so GetActiveSnapshot()?
Works for me.
regards, tom lane
On Tue, Jul 23, 2013 at 2:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Works for me. OK. I've taken care of all remaining uses of SnapshotNow in the code base. I think we can go ahead and remove it, now. Patch attached. (And there was, hopefully, much rejoicing.) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
Robert Haas <robertmhaas@gmail.com> writes:
> OK. I've taken care of all remaining uses of SnapshotNow in the code
> base. I think we can go ahead and remove it, now. Patch attached.
> (And there was, hopefully, much rejoicing.)
What about SnapshotSelf?
regards, tom lane
On 2013-07-26 08:49:38 -0400, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > OK. I've taken care of all remaining uses of SnapshotNow in the code > > base. I think we can go ahead and remove it, now. Patch attached. > > > (And there was, hopefully, much rejoicing.) > > What about SnapshotSelf? I thought about that yesterday and I think we should replace the usages which aren't easily replaceable (constraint stuff) with an mvcc snapshot, just one treats our transaction's current CommandId as visible. That should be doable? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 2013-07-25 19:24:53 -0400, Robert Haas wrote: > (And there was, hopefully, much rejoicing.) Definitely! Thanks. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Fri, Jul 26, 2013 at 8:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> OK. I've taken care of all remaining uses of SnapshotNow in the code >> base. I think we can go ahead and remove it, now. Patch attached. > >> (And there was, hopefully, much rejoicing.) > > What about SnapshotSelf? Well, that's still used in _bt_check_unique, unique_key_recheck (trigger function to do a deferred uniqueness check), RI_FKey_check, and rather extensively by sepgsql. I don't really have much desire to do the work to get rid of it, though. Getting rid of SnapshotNow is arguably important on the grounds that third-party code may be using it, and doing this will force them to do it the new way instead, and that's got some value. I'm not sure if anything's already been committed that relies on MVCC catalog access, but several things have certainly been proposed and it's a good bet that 9.4 will rely on the catalog access using MVCC-semantics, so forcing third-party code to stop using SnapshotNow will prevent subtle bugs. But there's no similar joy for SnapshotSelf. You can argue that all of the things that we're doing with it are crufty, but nobody's complaining about any of them, and some of them are in places where the cost of an additional MVCC snapshot on every iteration might be much more serious than anything we ever saw for catalog scans. So I'm personally content to leave it well enough alone. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Jul 26, 2013 at 8:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What about SnapshotSelf?
> Well, that's still used in _bt_check_unique, unique_key_recheck
> (trigger function to do a deferred uniqueness check), RI_FKey_check,
> and rather extensively by sepgsql. I don't really have much desire to
> do the work to get rid of it, though.
Hm. I agree the first three may be all right, but I can't help
suspecting that sepgsql is doing the wrong thing here.
regards, tom lane
On Fri, Jul 26, 2013 at 9:18 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Fri, Jul 26, 2013 at 8:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> What about SnapshotSelf? > >> Well, that's still used in _bt_check_unique, unique_key_recheck >> (trigger function to do a deferred uniqueness check), RI_FKey_check, >> and rather extensively by sepgsql. I don't really have much desire to >> do the work to get rid of it, though. > > Hm. I agree the first three may be all right, but I can't help > suspecting that sepgsql is doing the wrong thing here. sepgsql is using SnapshotSelf to find the old version of a tuple that was updated by the core code just before. That should be safe in the sense that there can't be a currently-committing transaction somewhere else that's updated that tuple, if we know that our own uncommitted transaction has done a transactional update. There was a recent thread discussing whether another API might be better, and I'd be prepared to concede that it might be. But I don't think it's drop-dead broken. Not that I really object if someone wants to have a go at getting rid of SnapshotSelf, but I think it'd be worth articulating what we hope to accomplish by so doing. For example, the btree README says the following about the deletion algorithm: --- ... The reason we do it is to provide an interlock between non-full VACUUM and indexscans. Since VACUUM deletes index entries before deleting tuples, the super-exclusive lock guarantees that VACUUM can't delete any heap tuple that an indexscanning process might be about to visit. (This guarantee works only for simple indexscans that visit the heap in sync with the index scan, not for bitmap scans. We only need the guarantee when using non-MVCC snapshot rules; in an MVCC snapshot, it wouldn't matter if the heap tuple were replaced with an unrelated tuple at the same TID, because the new tuple wouldn't be visible to our scan anyway.) --- Obviously, when we were using SnapshotNow for catalog access, changing anything here was a non-starter. But now that we're not, it might be worth asking whether there are few enough users of non-MVCC rules that we could apply some suitable treatment to those that remain and then change the locking protocol here. And if we did do that, would there be enough performance benefit to justify the work? I don't have answers to those questions, and the answer may well be that we should leave things as they are, but I think the questions are worth thinking about. Aside from any possible advantage in further trimming the list of available snapshot types, I'd like to spend some time thinking about what we can do that's safe and useful in terms of reducing lock levels; or even adding completely new facilities that would have been DOA in the old world. I have high hopes in both areas, but I wouldn't be surprised to find that there are problems we haven't thought about yet. I think our dependence on SnapshotNow has wormed itself into our design choices in deep ways, and I suspect it's going to take a good deal of thought to figure out exactly what we can improve and how. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Jul 26, 2013 at 9:18 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> Well, that's still used in _bt_check_unique, unique_key_recheck
>>> (trigger function to do a deferred uniqueness check), RI_FKey_check,
>>> and rather extensively by sepgsql. I don't really have much desire to
>>> do the work to get rid of it, though.
>> Hm. I agree the first three may be all right, but I can't help
>> suspecting that sepgsql is doing the wrong thing here.
> sepgsql is using SnapshotSelf to find the old version of a tuple that
> was updated by the core code just before.
Oh. OK, then it reduces to the same case as the other three, ie we're
looking at tuples we know to be update-locked.
> [ interesting ruminations snipped ]
Yeah, removing SnapshotNow catalog access certainly opens the doors
for a lot of new thinking.
regards, tom lane
On 2013-07-26 09:50:32 -0400, Robert Haas wrote: > sepgsql is using SnapshotSelf to find the old version of a tuple that > was updated by the core code just before. That should be safe in the > sense that there can't be a currently-committing transaction somewhere > else that's updated that tuple, if we know that our own uncommitted > transaction has done a transactional update. There was a recent > thread discussing whether another API might be better, and I'd be > prepared to concede that it might be. But I don't think it's > drop-dead broken. It's safe for the tuples updated in that transaction, but it's not safe to look at anything else if you expect results without the SnapshotNow problems. E.g. looking at a newly created attribute is fine, but iterating over all attributes not necessarily. I am more concerned about the care needed when placing CommandCounterIncrement()'s somewhere though. It seems more than likely that this will get repeatedly broken, even if it's not atm (which I doubt). E.g. inheritance handling seems to be rather wonky WRT this. > Not that I really object if someone wants to have a go at getting rid > of SnapshotSelf, but I think it'd be worth articulating what we hope > to accomplish by so doing. Agreed. From the internal usages there doesn't seem to be too much pressure. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Fri, Jul 26, 2013 at 10:16 AM, Andres Freund <andres@2ndquadrant.com> wrote: > I am more concerned about the care needed when placing > CommandCounterIncrement()'s somewhere though. It seems more than likely > that this will get repeatedly broken, even if it's not atm (which I > doubt). E.g. inheritance handling seems to be rather wonky WRT this. There may well be bugs. I am fine with reviewing patches to improve the code in this area, but I don't plan to take it upon myself to rewrite that code. Either it's working as expected, or nobody's using it, because we're not getting any bug reports. >> Not that I really object if someone wants to have a go at getting rid >> of SnapshotSelf, but I think it'd be worth articulating what we hope >> to accomplish by so doing. > > Agreed. From the internal usages there doesn't seem to be too much > pressure. So unless there are objections to the patch as posted, I'm going to apply that next week. This in no way precludes more work in this area later, but since we're likely to break third-party code with this change, we might as well get it out of the way as early in the release cycle as possible. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Jul 25, 2013 at 07:24:53PM -0400, Robert Haas wrote:
> - /* Used by pre-9.0 binary upgrades */
> - if (tuple->t_infomask & HEAP_MOVED_OFF)
> - {
> - TransactionId xvac = HeapTupleHeaderGetXvac(tuple);
> -
> - if (TransactionIdIsCurrentTransactionId(xvac))
> - return false;
> - if (!TransactionIdIsInProgress(xvac))
> - {
> - if (TransactionIdDidCommit(xvac))
> - {
> - SetHintBits(tuple, buffer, HEAP_XMIN_INVALID,
> - InvalidTransactionId);
> - return false;
> - }
> - SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED,
> - InvalidTransactionId);
> - }
> - }
> - /* Used by pre-9.0 binary upgrades */
> - else if (tuple->t_infomask & HEAP_MOVED_IN)
> - {
> - TransactionId xvac = HeapTupleHeaderGetXvac(tuple);
> -
> - if (!TransactionIdIsCurrentTransactionId(xvac))
> - {
> - if (TransactionIdIsInProgress(xvac))
> - return false;
> - if (TransactionIdDidCommit(xvac))
> - SetHintBits(tuple, buffer, HEAP_XMIN_COMMITTED,
> - InvalidTransactionId);
> - else
> - {
> - SetHintBits(tuple, buffer, HEAP_XMIN_INVALID,
> - InvalidTransactionId);
> - return false;
> - }
> - }
> - }
One interesting aspect of this patch is that the backend code is no
longer even checking HEAP_MOVED_OFF and HEAP_MOVED_IN. However, we
can't reuse those bits because they could be set from pre-9.0 rows.
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://enterprisedb.com
+ It's impossible for everything to be true. +
On 2013-08-05 11:17:08 -0400, Bruce Momjian wrote: > One interesting aspect of this patch is that the backend code is no > longer even checking HEAP_MOVED_OFF and HEAP_MOVED_IN. However, we > can't reuse those bits because they could be set from pre-9.0 rows. The other tqual.c .satisfies routines still check it - and have to do so. It'd be nice to get rid of that, but this patch doesn't seem to get us nearer towards it :( Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On Mon, Aug 5, 2013 at 05:22:28PM +0200, Andres Freund wrote: > On 2013-08-05 11:17:08 -0400, Bruce Momjian wrote: > > One interesting aspect of this patch is that the backend code is no > > longer even checking HEAP_MOVED_OFF and HEAP_MOVED_IN. However, we > > can't reuse those bits because they could be set from pre-9.0 rows. > > The other tqual.c .satisfies routines still check it - and have to do > so. > > It'd be nice to get rid of that, but this patch doesn't seem to get us > nearer towards it :( Oh, sorry, thanks for pointing that out --- somehow I missed it. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. +