Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-07-20 13:59:32 -0400, Robert Haas wrote: > It's hard to believe that it's equally good to use the newest > registered snapshot (which is, I think, what you will often get from > GetActiveSnapshot()) and the oldest registered snapshot (which is what > you will get from pairingheap_first()).

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Wed, Jul 20, 2016 at 12:30 PM, Andres Freund wrote: >> And how do you obtain that? The functions that reference >> SnapshotToast are toast_delete_datum, toastrel_value_exists, and >> toast_fetch_datum, toast_fetch_datum_slice, but none of those take a >> snapshot as an

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

Andres Freund wrote: > On 2016-07-20 11:26:11 -0400, Robert Haas wrote: > > On Wed, Jul 20, 2016 at 3:39 AM, Andres Freund wrote: > > >>I think Snapshot's members whenTaken and lsn are updated/initialized > > >>only in GetSnapshotData(). So if GetSnapshotData() is not used,

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-07-20 11:26:11 -0400, Robert Haas wrote: > On Wed, Jul 20, 2016 at 3:39 AM, Andres Freund wrote: > >>I think Snapshot's members whenTaken and lsn are updated/initialized > >>only in GetSnapshotData(). So if GetSnapshotData() is not used, how > >>will you expect those

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Wed, Jul 20, 2016 at 3:39 AM, Andres Freund wrote: >>I think Snapshot's members whenTaken and lsn are updated/initialized >>only in GetSnapshotData(). So if GetSnapshotData() is not used, how >>will you expect those fields to be updated. We need those fields to >>be

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Tue, Jul 19, 2016 at 6:32 PM, Andres Freund wrote: > I mean the only difference between toast / plain heap table WRT > old_snapshot_threshold is that we don't use a mvcc snapshot. We use different functions and never, ever call BufferGetPage -- except for deep in the

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On July 19, 2016 7:43:05 PM PDT, Amit Kapila wrote: >On Wed, Jul 20, 2016 at 7:57 AM, Andres Freund >wrote: >> >> >> On July 19, 2016 7:14:42 PM PDT, Amit Kapila > wrote: >>>On Wed, Jul 20, 2016 at 5:02 AM, Andres Freund

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Wed, Jul 20, 2016 at 7:57 AM, Andres Freund wrote: > > > On July 19, 2016 7:14:42 PM PDT, Amit Kapila wrote: >>On Wed, Jul 20, 2016 at 5:02 AM, Andres Freund >>wrote: >>> On 2016-07-19 18:09:59 -0500, Kevin Grittner wrote:

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On July 19, 2016 7:14:42 PM PDT, Amit Kapila wrote: >On Wed, Jul 20, 2016 at 5:02 AM, Andres Freund >wrote: >> On 2016-07-19 18:09:59 -0500, Kevin Grittner wrote: >>> As far as I can see, to do this the way that Andres and Amit >>> suggest involves

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Wed, Jul 20, 2016 at 5:02 AM, Andres Freund wrote: > On 2016-07-19 18:09:59 -0500, Kevin Grittner wrote: >> As far as I can see, to do this the way that Andres and Amit >> suggest involves tying in to indexam.c and other code in incredibly >> ugly ways. > > Could you

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-07-19 18:09:59 -0500, Kevin Grittner wrote: > As far as I can see, to do this the way that Andres and Amit > suggest involves tying in to indexam.c and other code in incredibly > ugly ways. Could you explain the problem you're seing? Isn't pretty much all all that we need to do: 1) add a

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-07-13 15:57:02 -0500, Kevin Grittner wrote: > A short answer is that a normal table's heap doesn't go through > systable_getnext_ordered(). That function is used both for cases > where the check should not be made, like toast_delete_datum(), and > cases where it should, like

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Wed, Jul 13, 2016 at 12:48 PM, Andres Freund wrote: > On 2016-07-13 10:06:52 -0500, Kevin Grittner wrote: >> On Wed, Jul 13, 2016 at 7:57 AM, Amit Kapila wrote: >>> On Tue, Jul 12, 2016 at 8:34 PM, Kevin Grittner wrote: On

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-07-13 10:06:52 -0500, Kevin Grittner wrote: > On Wed, Jul 13, 2016 at 7:57 AM, Amit Kapila wrote: > > On Tue, Jul 12, 2016 at 8:34 PM, Kevin Grittner wrote: > >> On Fri, Jul 8, 2016 at 1:52 PM, Andres Freund wrote: > >> >

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-07-12 10:04:45 -0500, Kevin Grittner wrote: > On Fri, Jul 8, 2016 at 1:52 PM, Andres Freund wrote: > > > I'm a bit confused, why aren't we simply adding LSN interlock > > checks for toast? Doesn't look that hard? Seems like a much more > > natural course of fixing

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Wed, Jul 13, 2016 at 7:57 AM, Amit Kapila wrote: > On Tue, Jul 12, 2016 at 8:34 PM, Kevin Grittner wrote: >> On Fri, Jul 8, 2016 at 1:52 PM, Andres Freund wrote: >> >>> I'm a bit confused, why aren't we simply adding LSN

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Tue, Jul 12, 2016 at 8:34 PM, Kevin Grittner wrote: > On Fri, Jul 8, 2016 at 1:52 PM, Andres Freund wrote: > >> I'm a bit confused, why aren't we simply adding LSN interlock >> checks for toast? Doesn't look that hard? Seems like a much more >> natural

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Fri, Jul 8, 2016 at 1:52 PM, Andres Freund wrote: > I'm a bit confused, why aren't we simply adding LSN interlock > checks for toast? Doesn't look that hard? Seems like a much more > natural course of fixing this issue? I took some time trying to see what you have in

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-07-08 13:32:35 -0500, Kevin Grittner wrote: > On Fri, Jul 8, 2016 at 12:53 PM, Andres Freund wrote: > > On 2016-07-08 11:00:50 -0500, Kevin Grittner wrote: > >> On Wed, Jul 6, 2016 at 4:55 PM, Andres Freund wrote: > >> > >> > So I don't think that

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Fri, Jul 8, 2016 at 12:53 PM, Andres Freund wrote: > On 2016-07-08 11:00:50 -0500, Kevin Grittner wrote: >> On Wed, Jul 6, 2016 at 4:55 PM, Andres Freund wrote: >> >> > So I don't think that approach still allows old snapshot related >> > cleanups for

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-07-08 11:00:50 -0500, Kevin Grittner wrote: > On Wed, Jul 6, 2016 at 4:55 PM, Andres Freund wrote: > > > So I don't think that approach still allows old snapshot related > > cleanups for toast triggered vacuums? Is that an acceptable > > restriction? > > What I

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Wed, Jul 6, 2016 at 4:55 PM, Andres Freund wrote: > So I don't think that approach still allows old snapshot related > cleanups for toast triggered vacuums? Is that an acceptable > restriction? What I would rather see is that if the heap is vacuumed (whether or not by

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-07-02 14:20:13 -0500, Kevin Grittner wrote: > The possible fixes considered were these: > > (1) Always vacuum the heap before its related TOAST table. I think that's clearly not ok from a cost perspective. > (2) Same as (1) but only when old_snapshot_threshold >= 0. > (3) Allow the

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-06-16 11:57:48 -0700, Andres Freund wrote: > See > https://www.postgresql.org/message-id/20160616183207.wygoktoplycdz...@alap3.anar For posterity's sake, that was supposed to be https://www.postgresql.org/message-id/20160616183207.wygoktoplycdz...@alap3.anarazel.de -- Sent via

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-06-16 13:53:01 -0500, Kevin Grittner wrote: > On Thu, Jun 16, 2016 at 1:19 PM, Kevin Grittner wrote: > > On Thu, Jun 16, 2016 at 11:54 AM, Andres Freund wrote: > >> On 2016-06-16 12:43:34 -0400, Robert Haas wrote: > > >>> Maybe it would help if you

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Thu, Jun 16, 2016 at 1:32 PM, Andres Freund wrote: > With old_snapshot_threshold=1 I indeed can reproduce the issue. I > disabled autovacuum, to make the scheduling more predictable. But it > should "work" just as well with autovacuum. > > S1: CREATE TABLE toasted(largecol

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Thu, Jun 16, 2016 at 1:19 PM, Kevin Grittner wrote: > On Thu, Jun 16, 2016 at 11:54 AM, Andres Freund wrote: >> On 2016-06-16 12:43:34 -0400, Robert Haas wrote: >>> Maybe it would help if you lay out the whole sequence of events, like: >>> >>> S1: Does

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

Hi, On 2016-06-16 13:19:13 -0500, Kevin Grittner wrote: > On Thu, Jun 16, 2016 at 11:54 AM, Andres Freund wrote: > > On 2016-06-16 12:43:34 -0400, Robert Haas wrote: > > >> The root of my confusion is: if we prune a tuple, we'll bump the page > >> LSN, so any session that is

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-06-16 13:16:35 -0500, Kevin Grittner wrote: > On Thu, Jun 16, 2016 at 1:01 PM, Andres Freund wrote: > > > The relevant part is the HeapTupleSatisfiesMVCC check, we're using > > SatisfiesToast for toast lookups. > > > > FWIW, I just tried to reproduce this with

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Thu, Jun 16, 2016 at 11:54 AM, Andres Freund wrote: > On 2016-06-16 12:43:34 -0400, Robert Haas wrote: >> The root of my confusion is: if we prune a tuple, we'll bump the page >> LSN, so any session that is still referencing that tuple will error >> out as soon as it

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Thu, Jun 16, 2016 at 1:01 PM, Andres Freund wrote: > The relevant part is the HeapTupleSatisfiesMVCC check, we're using > SatisfiesToast for toast lookups. > > FWIW, I just tried to reproduce this with old_snapshot_threshold = 0 - > but ran into the problem that I couldn't

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-06-16 13:44:34 -0400, Robert Haas wrote: > On Thu, Jun 16, 2016 at 12:54 PM, Andres Freund wrote: > >> The root of my confusion is: if we prune a tuple, we'll bump the page > >> LSN, so any session that is still referencing that tuple will error > >> out as soon as it

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Thu, Jun 16, 2016 at 12:54 PM, Andres Freund wrote: >> The root of my confusion is: if we prune a tuple, we'll bump the page >> LSN, so any session that is still referencing that tuple will error >> out as soon as it touches the page on which that tuple used to exist. > >

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-06-16 12:43:34 -0400, Robert Haas wrote: > On Thu, Jun 16, 2016 at 12:14 PM, Andres Freund wrote: > >> > The issue isn't there without the feature, because we (should) never > >> > access a tuple/detoast a column when it's invisible enough for the > >> > corresponding

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Thu, Jun 16, 2016 at 12:14 PM, Andres Freund wrote: >> > The issue isn't there without the feature, because we (should) never >> > access a tuple/detoast a column when it's invisible enough for the >> > corresponding toast tuple to be vacuumed away. But with >> >

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-06-16 12:02:39 -0400, Robert Haas wrote: > On Thu, Jun 16, 2016 at 11:37 AM, Andres Freund wrote: > >> If that were really true, why would we not have the problem in > >> current production versions that the toast table could be vacuumed > >> before the heap, leading

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Thu, Jun 16, 2016 at 11:37 AM, Andres Freund wrote: >> If that were really true, why would we not have the problem in >> current production versions that the toast table could be vacuumed >> before the heap, leading to exactly the issue you are talking >> about? > > The

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-06-16 09:50:09 -0500, Kevin Grittner wrote: > On Wed, Jun 15, 2016 at 8:57 PM, Andres Freund wrote: > > > The simplest version of the scenario I'm concerned about is that a tuple > > in a tuple is *not* vacuumed, even though it's elegible to be removed > > due to STO.

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Wed, Jun 15, 2016 at 8:57 PM, Andres Freund wrote: > The simplest version of the scenario I'm concerned about is that a tuple > in a tuple is *not* vacuumed, even though it's elegible to be removed > due to STO. If that tuple has toasted columns, it could be the that the >

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

Kevin Grittner wrote: > test=# -- connection 1 > analyze verbose t1; -- when run after threshold, STO error occurs > INFO: analyzing "public.t1" > INFO: "t1": scanned 45 of 45 pages, containing 8999 live rows and > 1001 dead rows; 8999 rows in sample, 8999 estimated total rows > ERROR:

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-06-15 20:22:24 -0500, Kevin Grittner wrote: > I'm not clear where you see this as being in any way different with > STO. Above it seemed that you saw this as an issue related to > ANALYZE. If there is not early pruning for the table being > analyzed, nothing is at all different. If

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Wed, Jun 15, 2016 at 5:34 PM, Andres Freund wrote: > On 2016-06-15 16:58:25 -0500, Kevin Grittner wrote: >> On Wed, Jun 15, 2016 at 3:25 PM, Andres Freund wrote: >>> On 2016-06-15 14:24:58 -0500, Kevin Grittner wrote: On Wed, Jun 15, 2016 at 2:20

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-06-15 16:58:25 -0500, Kevin Grittner wrote: > On Wed, Jun 15, 2016 at 3:25 PM, Andres Freund wrote: > > On 2016-06-15 14:24:58 -0500, Kevin Grittner wrote: > >> On Wed, Jun 15, 2016 at 2:20 PM, Andres Freund wrote: > >> > >> > We might fetch a

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Wed, Jun 15, 2016 at 2:40 PM, Alvaro Herrera wrote: > Kevin Grittner wrote: >> On Wed, Jun 15, 2016 at 1:59 PM, Alvaro Herrera >> wrote: > >> > We actually go quite some lengths to support this case, even when it's >> > the opinion of many

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Wed, Jun 15, 2016 at 3:25 PM, Andres Freund wrote: > On 2016-06-15 14:24:58 -0500, Kevin Grittner wrote: >> On Wed, Jun 15, 2016 at 2:20 PM, Andres Freund wrote: >> >> > We might fetch a toast tuple which >> > since have been re-purposed for a datum of

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-06-15 14:24:58 -0500, Kevin Grittner wrote: > On Wed, Jun 15, 2016 at 2:20 PM, Andres Freund wrote: > > > We might fetch a toast tuple which > > since have been re-purposed for a datum of a different type. > > How would that happen? Autovac vacuums toast and heap

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Wed, Jun 15, 2016 at 3:40 PM, Alvaro Herrera wrote: > Kevin Grittner wrote: > > On Wed, Jun 15, 2016 at 1:59 PM, Alvaro Herrera > > wrote: > > > > We actually go quite some lengths to support this case, even when it's > > > the opinion of

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Wed, Jun 15, 2016 at 2:59 PM, Alvaro Herrera wrote: > Kevin Grittner wrote: >> On Wed, Jun 15, 2016 at 1:50 PM, Robert Haas wrote: >> >> > The expression index case is the one to worry about; if there is a >> > problem, that's where it is.

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

Kevin Grittner wrote: > On Wed, Jun 15, 2016 at 1:59 PM, Alvaro Herrera > wrote: > > We actually go quite some lengths to support this case, even when it's > > the opinion of many that we shouldn't. For example VACUUM doesn't try > > to find index entries using the

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Wed, Jun 15, 2016 at 2:20 PM, Andres Freund wrote: > We might fetch a toast tuple which > since have been re-purposed for a datum of a different type. How would that happen? -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-06-15 14:50:46 -0400, Robert Haas wrote: > On Wed, Jun 15, 2016 at 2:45 PM, Alvaro Herrera > wrote: > > Robert Haas wrote: > >> On Wed, Jun 15, 2016 at 1:44 PM, Kevin Grittner > >> wrote: > >> > The test I showed creates a situation which (to

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Wed, Jun 15, 2016 at 1:59 PM, Alvaro Herrera wrote: > Kevin Grittner wrote: >> On Wed, Jun 15, 2016 at 1:50 PM, Robert Haas wrote: >> >>> The expression index case is the one to worry about; if there is a >>> problem, that's where it is. What

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Wed, Jun 15, 2016 at 1:54 PM, Alvaro Herrera wrote: > Robert Haas wrote: >> The expression index case is the one to worry about; if there is a >> problem, that's where it is. What bothers me is that a function used >> in an expression index could do anything at all

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

Kevin Grittner wrote: > On Wed, Jun 15, 2016 at 1:50 PM, Robert Haas wrote: > > > The expression index case is the one to worry about; if there is a > > problem, that's where it is. What bothers me is that a function used > > in an expression index could do anything at

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Wed, Jun 15, 2016 at 1:50 PM, Robert Haas wrote: > The expression index case is the one to worry about; if there is a > problem, that's where it is. What bothers me is that a function used > in an expression index could do anything at all - it can read any > table in

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

Robert Haas wrote: > On Wed, Jun 15, 2016 at 2:45 PM, Alvaro Herrera > wrote: > > Maybe it is possible to get into trouble if you're taking a sample for > > an expression index. > > The expression index case is the one to worry about; if there is a > problem, that's

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Wed, Jun 15, 2016 at 2:45 PM, Alvaro Herrera wrote: > Robert Haas wrote: >> On Wed, Jun 15, 2016 at 1:44 PM, Kevin Grittner >> wrote: >> > The test I showed creates a situation which (to ANALYZE) is >> > identical to what you describe -- ANALYZE

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Wed, Jun 15, 2016 at 1:45 PM, Alvaro Herrera wrote: > Maybe it is possible to get into trouble if you're taking a sample for > an expression index. Maybe -- if you are using a function for an index expression which does an explicit SELECT against some database table

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

Robert Haas wrote: > On Wed, Jun 15, 2016 at 1:44 PM, Kevin Grittner > wrote: > > The test I showed creates a situation which (to ANALYZE) is > > identical to what you describe -- ANALYZE sees a page with an LSN > > recent enough that it could have been (and actually has been)

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Wed, Jun 15, 2016 at 1:29 PM, Robert Haas wrote: > On Wed, Jun 15, 2016 at 1:44 PM, Kevin Grittner > wrote:>> So what happens in this scenario: >>> 1. ANALYZE runs really slowly - maybe the user-defined function it's >>> running for the expression

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Wed, Jun 15, 2016 at 1:44 PM, Kevin Grittner wrote:>> So what happens in this scenario: >> 1. ANALYZE runs really slowly - maybe the user-defined function it's >> running for the expression index is extremely long-running. >> 2. Eventually, the snapshot for ANALYZE is older

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Wed, Jun 15, 2016 at 10:46 AM, Robert Haas wrote: > On Sat, Jun 11, 2016 at 11:29 AM, Kevin Grittner wrote: >> I have reviewed the code and run tests to try to find something >> here which could be considered a bug, without finding any problem. >>

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Sat, Jun 11, 2016 at 11:29 AM, Kevin Grittner wrote: > I have reviewed the code and run tests to try to find something > here which could be considered a bug, without finding any problem. > When reading pages for the random sample for ANALYZE (or > auto-analyze) there is not

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Tue, May 24, 2016 at 4:10 PM, Robert Haas wrote: > On Tue, May 24, 2016 at 3:48 PM, Kevin Grittner wrote: >> On Tue, May 24, 2016 at 12:00 PM, Andres Freund wrote: >>> On 2016-05-24 11:24:44 -0500, Kevin Grittner wrote: On

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Fri, Jun 10, 2016 at 10:26 AM, Robert Haas wrote: > On Fri, Jun 10, 2016 at 10:45 AM, Kevin Grittner wrote: >> Since vacuum calls the pruning function, and not the other way >> around, the name you suggest would be technically more correct. >>

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Fri, Jun 10, 2016 at 10:45 AM, Kevin Grittner wrote: > On Thu, Jun 9, 2016 at 6:16 PM, Robert Haas wrote: > >> So I like the idea of centralizing checks in >> RelationAllowsEarlyVacuum, but shouldn't it really be called >> RelationAllowsEarlyPruning?

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Thu, Jun 9, 2016 at 6:16 PM, Robert Haas wrote: > So I like the idea of centralizing checks in > RelationAllowsEarlyVacuum, but shouldn't it really be called > RelationAllowsEarlyPruning? Since vacuum calls the pruning function, and not the other way around, the name

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Thu, Jun 9, 2016 at 10:28 AM, Kevin Grittner wrote: > [Thanks to Robert to stepping up to keep this moving while I was > down yesterday with a minor injury. I'm back on it today.] >> Generally, I think I see the hazard you're concerned about: I had >> failed to realize, as

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

[Thanks to Robert to stepping up to keep this moving while I was down yesterday with a minor injury. I'm back on it today.] On Wed, Jun 8, 2016 at 3:11 PM, Robert Haas wrote: > On Wed, Jun 8, 2016 at 4:04 PM, Kevin Grittner wrote: >> -- connection 1

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Fri, Jun 03, 2016 at 04:29:40PM -0500, Kevin Grittner wrote: > On Fri, May 27, 2016 at 10:35 AM, Kevin Grittner wrote: > > On Tue, May 24, 2016 at 4:10 PM, Robert Haas wrote: > > >> [ANALYZE of index with expression may fail to update statistics > >>

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Wed, Jun 8, 2016 at 4:04 PM, Kevin Grittner wrote: > On Wed, Jun 8, 2016 at 2:49 PM, Robert Haas wrote: >> Do you have a test case that demonstrates a problem, or an explanation >> of why you think there is one? > > With old_snapshot_threshold =

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Wed, Jun 8, 2016 at 2:49 PM, Robert Haas wrote: > Do you have a test case that demonstrates a problem, or an explanation > of why you think there is one? With old_snapshot_threshold = '1min' -- connection 1 drop table if exists t1; create table t1 (c1 int not null);

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Wed, Jun 8, 2016 at 9:40 AM, Kevin Grittner wrote: >>> Of course, ii_BrokenHotChain should be renamed to something like >>> ii_UnsafeForOldSnapshots, and some comments need to be updated; but >>> the above is the substance of it. >> >> I don't know why we'd want to rename it

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Tue, Jun 7, 2016 at 10:40 AM, Robert Haas wrote: > On Sat, Jun 4, 2016 at 4:21 PM, Kevin Grittner wrote: >> the minimal patch to fix behavior in this area would be: >> >> diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c >> index

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Sat, Jun 4, 2016 at 4:21 PM, Kevin Grittner wrote: > On Fri, Jun 3, 2016 at 4:24 PM, Kevin Grittner wrote: >> Consequently, causing the index to be ignored in planning when >> using the old index > > That last line should have read "using an old snapshot"

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Fri, Jun 3, 2016 at 4:24 PM, Kevin Grittner wrote: > Consequently, causing the index to be ignored in planning when > using the old index That last line should have read "using an old snapshot" > is not a nice optimization, but necessary for > correctness. We already

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Fri, May 27, 2016 at 10:35 AM, Kevin Grittner wrote: > On Tue, May 24, 2016 at 4:10 PM, Robert Haas wrote: >> [ANALYZE of index with expression may fail to update statistics >> if ANALYZE runs longer than old_snapshot_threshold] >> If we can get

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Fri, May 27, 2016 at 10:18 AM, Kevin Grittner wrote: > On Fri, May 27, 2016 at 9:58 AM, Kevin Grittner wrote: >> On Tue, May 24, 2016 at 4:56 PM, Andres Freund wrote: > >>> If an old session with >= repeatable read accesses a

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Tue, May 31, 2016 at 10:03 AM, Robert Haas wrote: > On Fri, May 27, 2016 at 10:58 AM, Kevin Grittner wrote: >>> As far as I can see normal index builds will allow concurrent hot >>> prunes and everything; since those only require page-level >>>

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Fri, May 27, 2016 at 10:58 AM, Kevin Grittner wrote: >> As far as I can see normal index builds will allow concurrent hot >> prunes and everything; since those only require page-level >> exclusive locks. >> >> So for !concurrent builds we might end up with a corrupt index. >

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Tue, May 24, 2016 at 4:10 PM, Robert Haas wrote: > On Tue, May 24, 2016 at 3:48 PM, Kevin Grittner wrote: >> On Tue, May 24, 2016 at 12:00 PM, Andres Freund wrote: >>> On 2016-05-24 11:24:44 -0500, Kevin Grittner wrote: On

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Fri, May 27, 2016 at 9:58 AM, Kevin Grittner wrote: > On Tue, May 24, 2016 at 4:56 PM, Andres Freund wrote: >> If an old session with >= repeatable read accesses a clustered >> table (after the cluster committed), they'll now not see any >> errors,

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Tue, May 24, 2016 at 4:56 PM, Andres Freund wrote: > The LSNs of the created index pages will be new, and we'll thus > not necessarily error out when requried. It is *old* LSNs that are *safe* -- *new* LSNs are what trigger the "snapshot too old" error. So your

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-05-24 16:09:27 -0500, Kevin Grittner wrote: > On Tue, May 24, 2016 at 3:54 PM, Andres Freund wrote: > > > what about e.g. concurrent index builds? E.g. IndexBuildHeapRangeScan() > > doesn't > > seem to contain any checks against outdated blocks > > Why would it?

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Tue, May 24, 2016 at 4:09 PM, Kevin Grittner wrote: > On Tue, May 24, 2016 at 3:54 PM, Andres Freund wrote: >> It appears that concurrent index builds are currently broken >> from a quick skim? > > Either you don't understand this feature very well, or

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Tue, May 24, 2016 at 4:10 PM, Robert Haas wrote: > For purposes of > "snapshot too old", though, it will be important that a function in an > index which tries to read data from some other table which has been > pruned cancels itself when necessary. Hm. I'll try to

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Tue, May 24, 2016 at 3:48 PM, Kevin Grittner wrote: > On Tue, May 24, 2016 at 12:00 PM, Andres Freund wrote: >> On 2016-05-24 11:24:44 -0500, Kevin Grittner wrote: >>> On Fri, May 6, 2016 at 8:28 PM, Kevin Grittner wrote: On Fri,

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Tue, May 24, 2016 at 3:54 PM, Andres Freund wrote: > what about e.g. concurrent index builds? E.g. IndexBuildHeapRangeScan() > doesn't > seem to contain any checks against outdated blocks Why would it? We're talking about blocks where there were dead tuples, with the

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-05-24 14:48:35 -0500, Kevin Grittner wrote: > On Tue, May 24, 2016 at 12:00 PM, Andres Freund wrote: > > On 2016-05-24 11:24:44 -0500, Kevin Grittner wrote: > >> On Fri, May 6, 2016 at 8:28 PM, Kevin Grittner wrote: > >>> On Fri, May 6, 2016 at 7:48

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

Kevin Grittner wrote: > On Tue, May 24, 2016 at 12:00 PM, Andres Freund wrote: > > On 2016-05-24 11:24:44 -0500, Kevin Grittner wrote: > >> On Fri, May 6, 2016 at 8:28 PM, Kevin Grittner wrote: > >>> On Fri, May 6, 2016 at 7:48 PM, Andres Freund

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Tue, May 24, 2016 at 12:00 PM, Andres Freund wrote: > On 2016-05-24 11:24:44 -0500, Kevin Grittner wrote: >> On Fri, May 6, 2016 at 8:28 PM, Kevin Grittner wrote: >>> On Fri, May 6, 2016 at 7:48 PM, Andres Freund wrote: >> That

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-05-24 13:04:09 -0500, Kevin Grittner wrote: > On Tue, May 24, 2016 at 12:00 PM, Andres Freund wrote: > > > Analyze IIRC acquires a new snapshot when getting sample rows, > > I could not find anything like that, and a case-insensitive search > of analyze.c finds no

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

Kevin Grittner writes: > On Tue, May 24, 2016 at 12:00 PM, Andres Freund wrote: >> Analyze IIRC acquires a new snapshot when getting sample rows, > I could not find anything like that, and a case-insensitive search > of analyze.c finds no occurrences of

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

Kevin Grittner wrote: > On Tue, May 24, 2016 at 12:00 PM, Andres Freund wrote: > > > Analyze IIRC acquires a new snapshot when getting sample rows, > > I could not find anything like that, and a case-insensitive search > of analyze.c finds no occurrences of "snap". Can you

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Tue, May 24, 2016 at 12:00 PM, Andres Freund wrote: > Analyze IIRC acquires a new snapshot when getting sample rows, I could not find anything like that, and a case-insensitive search of analyze.c finds no occurrences of "snap". Can you remember where you think you saw

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-05-24 11:24:44 -0500, Kevin Grittner wrote: > On Fri, May 6, 2016 at 8:28 PM, Kevin Grittner wrote: > > On Fri, May 6, 2016 at 7:48 PM, Andres Freund wrote: > > >> That comment reminds me of a question I had: Did you consider the effect > >> of

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Fri, May 6, 2016 at 8:28 PM, Kevin Grittner wrote: > On Fri, May 6, 2016 at 7:48 PM, Andres Freund wrote: >> That comment reminds me of a question I had: Did you consider the effect >> of this patch on analyze? It uses a snapshot, and by memory you've

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-05-06 20:28:27 -0500, Kevin Grittner wrote: > On Fri, May 6, 2016 at 7:48 PM, Andres Freund wrote: > > On 2016-05-06 19:43:24 -0500, Kevin Grittner wrote: > >> It's disappointing that I am not getting more consistent numbers, > >> but NUMA can be hard to manage that

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On Fri, May 6, 2016 at 7:48 PM, Andres Freund wrote: > On 2016-05-06 19:43:24 -0500, Kevin Grittner wrote: >> It's disappointing that I am not getting more consistent numbers, >> but NUMA can be hard to manage that way. > > FWIW, in my experience, unless you disable autovacuum

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

On 2016-05-06 19:43:24 -0500, Kevin Grittner wrote: > It's disappointing that I am not getting more consistent numbers, > but NUMA can be hard to manage that way. FWIW, in my experience, unless you disable autovacuum (or rather auto-analyze), the effects from non-predicable analyze runs with

  1   2   >