Re: [HACKERS] index-only count(*) for indexes supporting bitmap scans

2017-11-13 Thread Alexander Kuzmenkov
I've pushed the executor part of this, but mostly not the planner part, because I didn't think the latter was anywhere near ready for prime time: the regression test changes it was causing were entirely bogus. Hi Tom, Thanks for the commit and the explanation. I'll try to address your comments

Re: [HACKERS] index-only count(*) for indexes supporting bitmap scans

2017-11-01 Thread Tom Lane
Alexander Kuzmenkov writes: > On 04.09.2017 15:17, Alexey Chernyshov wrote: >> make check-world fails on contrib/postgres_fdw because of Subquery Scan on >> ... Probably, query plan was changed. > Thanks for testing! This is the same problem as the one in > 'select_distinct' I mentioned before.

Re: [HACKERS] Index only scan for cube and seg

2017-10-29 Thread Andrey Borodin
Hi! > 29 окт. 2017 г., в 2:24, Alexander Korotkov > написал(а): > > As I can see, cube GiST code always uses DatumGetNDBOX() macro to transform > Datum to (NDBOX *). > > #define DatumGetNDBOX(x) ((NDBOX *) PG_DETOAST_DATUM(x)) > > Thus, it should be safe to just remove both compress/deco

Re: [HACKERS] Index only scan for cube and seg

2017-10-28 Thread Alexander Korotkov
On Fri, Oct 27, 2017 at 9:54 PM, Tom Lane wrote: > Robert Haas writes: > > On Thu, Oct 26, 2017 at 12:22 PM, Andrey Borodin > wrote: > >> For cube there is new default opclass. > > > I seem to recall that changing the default opclass causes unsolvable > > problems with upgrades. You might want

Re: [HACKERS] Index only scan for cube and seg

2017-10-27 Thread Tom Lane
Robert Haas writes: > On Thu, Oct 26, 2017 at 12:22 PM, Andrey Borodin wrote: >> For cube there is new default opclass. > I seem to recall that changing the default opclass causes unsolvable > problems with upgrades. You might want to check the archives for > previous discussions of this issue;

Re: [HACKERS] Index only scan for cube and seg

2017-10-27 Thread Robert Haas
On Thu, Oct 26, 2017 at 12:22 PM, Andrey Borodin wrote: > For cube there is new default opclass. I seem to recall that changing the default opclass causes unsolvable problems with upgrades. You might want to check the archives for previous discussions of this issue; unfortunately, I don't recall

[HACKERS] Index only scan for cube and seg

2017-10-26 Thread Andrey Borodin
Hi hackers! Here are patches enabling Index Only Scan for cube and seg extensions. These patches follow this discussion [0]. For cube there is new default opclass. We cannot drop old opclass, because it could TOAST come cube values in rare occasions. Index Only Scan is enabled only for newly c

Re: [HACKERS] Index expression syntax

2017-09-29 Thread Konstantin Knizhnik
On 29.09.2017 11:03, Marko Tiikkaja wrote: On Fri, Sep 29, 2017 at 9:31 AM, Konstantin Knizhnik mailto:k.knizh...@postgrespro.ru>> wrote: I wonder why syntax error is produced in this case: postgres=# create index metaindex on foo using gin(to_tsvector('english', x)||to_tsvector(

Re: [HACKERS] Index expression syntax

2017-09-29 Thread Marko Tiikkaja
On Fri, Sep 29, 2017 at 9:31 AM, Konstantin Knizhnik < k.knizh...@postgrespro.ru> wrote: > I wonder why syntax error is produced in this case: > > postgres=# create index metaindex on foo using gin(to_tsvector('english', > x)||to_tsvector('english',y)); > ERROR: syntax error at or near "||" > LIN

[HACKERS] Index expression syntax

2017-09-29 Thread Konstantin Knizhnik
I wonder why syntax error is produced in this case: postgres=# create index metaindex on foo using gin(to_tsvector('english', x)||to_tsvector('english',y)); ERROR: syntax error at or near "||" LINE 1: ...taindex on foo using gin(to_tsvector('english', x)||to_tsvec...

Re: [HACKERS] Index Only Scan support for cube

2017-09-20 Thread Alexander Korotkov
On Wed, Sep 20, 2017 at 8:26 AM, Andrey Borodin wrote: > Hi hackers! > > 23 мая 2017 г., в 14:53, Andrew Borodin > написал(а): > > > > Here's a small patch that implements fetch function necessary for > > Index Only Scans that use cube data type. > > Tom Lane have just commited d3a4f89 (Allow no

Re: [HACKERS] Index Only Scan support for cube

2017-09-19 Thread Andrey Borodin
Hi hackers! > 23 мая 2017 г., в 14:53, Andrew Borodin написал(а): > > Here's a small patch that implements fetch function necessary for > Index Only Scans that use cube data type. Tom Lane have just commited d3a4f89 (Allow no-op GiST support functions to be omitted) Thanks, Tom! : ) "Index Only

Re: [HACKERS] index-only count(*) for indexes supporting bitmap scans

2017-09-07 Thread Alexey Chernyshov
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:tested, passed One thing I have noticed is a trailing whitespace after "bogu

Re: [HACKERS] index-only count(*) for indexes supporting bitmap scans

2017-09-04 Thread Alexander Kuzmenkov
On 04.09.2017 15:17, Alexey Chernyshov wrote: make check-world fails on contrib/postgres_fdw because of Subquery Scan on ... Probably, query plan was changed. Hi Alexey, Thanks for testing! This is the same problem as the one in 'select_distinct' I mentioned before. I changed the test, the u

Re: [HACKERS] index-only count(*) for indexes supporting bitmap scans

2017-09-04 Thread Alexey Chernyshov
The following review has been posted through the commitfest application: make installcheck-world: tested, failed Implements feature: not tested Spec compliant: not tested Documentation:not tested Hi Alexander, make check-world fails on contrib/postgres_fdw because of

Re: [HACKERS] index-only count(*) for indexes supporting bitmap scans

2017-08-21 Thread Alexander Kumenkov
|Hello everyone, I made a new patch according to the previous comments. It is simpler now, only adding a few checks to the bitmap heap scan node. When the target list for the bitmap heap scan is empty, and there is no filter, and the bitmap page generated by the index scan is exact, and the c

Re: [HACKERS] Index created in BEFORE trigger not updated during INSERT

2017-06-06 Thread Albe Laurenz
Tom Lane wrote: >> Andres Freund writes: >>> Hm, strategically sprinkled CheckTableNotInUse() might do the trick? > > Attached is a proposed patch that closes off this problem. I've tested > it to the extent that it blocks Albe's example and passes check-world. I tested it, and it works fine.

Re: [HACKERS] Index created in BEFORE trigger not updated during INSERT

2017-06-04 Thread Tom Lane
Michael Paquier writes: > The patch looks good to me, could you add a regression test? Done, thanks for the review. I stuck the test into triggers.sql, which is not completely on point since there are other ways to get to this error. But if we're thinking of it as a framework for testing other

Re: [HACKERS] Index created in BEFORE trigger not updated during INSERT

2017-06-03 Thread Andres Freund
On 2017-06-03 18:23:33 -0400, Tom Lane wrote: > Attached is a proposed patch that closes off this problem. I've tested > it to the extent that it blocks Albe's example and passes check-world. Cool. > I'm unsure whether to back-patch or not; the main argument for not doing > so is that if any ext

Re: [HACKERS] Index created in BEFORE trigger not updated during INSERT

2017-06-03 Thread Michael Paquier
On Sun, Jun 4, 2017 at 7:23 AM, Tom Lane wrote: > I'm unsure whether to back-patch or not; the main argument for not doing > so is that if any extensions are calling DefineIndex() directly, this > would be an API break for them. Given what a weird case this is, I'm not > sure it's worth that. Kn

Re: [HACKERS] Index created in BEFORE trigger not updated during INSERT

2017-06-03 Thread Tom Lane
I wrote: > Andres Freund writes: >> Hm, strategically sprinkled CheckTableNotInUse() might do the trick? > +1. We can't reasonably make it work: the outer query already has its > list of indexes that need to be inserted into. Also, if you try to > make the index via ALTER TABLE ADD CONSTRAINT i

Re: [HACKERS] Index created in BEFORE trigger not updated during INSERT

2017-05-28 Thread Tom Lane
Andres Freund writes: > On 2017-05-24 08:26:24 -0400, Robert Haas wrote: >> I'm willing to bet that nobody ever thought about that case very hard. >> It seems like we should either make it work or prohibit it, but I >> can't actually see quite how to do either off-hand. > Hm, strategically sprink

Re: [HACKERS] Index created in BEFORE trigger not updated during INSERT

2017-05-24 Thread Andres Freund
On 2017-05-24 08:26:24 -0400, Robert Haas wrote: > On Mon, May 22, 2017 at 7:05 AM, Albe Laurenz wrote: > > Not that it is a useful use case, but I believe that this is > > a bug that causes index corruption: > > > > CREATE TABLE mytable( > >id integer PRIMARY KEY, > >id2 integer NOT NULL

Re: [HACKERS] Index created in BEFORE trigger not updated during INSERT

2017-05-24 Thread Robert Haas
On Mon, May 22, 2017 at 7:05 AM, Albe Laurenz wrote: > Not that it is a useful use case, but I believe that this is > a bug that causes index corruption: > > CREATE TABLE mytable( >id integer PRIMARY KEY, >id2 integer NOT NULL > ); > > CREATE FUNCTION makeindex() RETURNS trigger >LANGU

[HACKERS] Index Only Scan support for cube

2017-05-23 Thread Andrew Borodin
Hi, hackers! Here's a small patch that implements fetch function necessary for Index Only Scans that use cube data type. I reuse function g_cube_decompress() instead of creating new function g_cube_fetch(). Essentially, they both have to detoast data. How do you think, is it better to create a sh

[HACKERS] Index created in BEFORE trigger not updated during INSERT

2017-05-22 Thread Albe Laurenz
Not that it is a useful use case, but I believe that this is a bug that causes index corruption: CREATE TABLE mytable( id integer PRIMARY KEY, id2 integer NOT NULL ); CREATE FUNCTION makeindex() RETURNS trigger LANGUAGE plpgsql AS $$BEGIN CREATE INDEX ON mytable(id2); RETURN NEW; E

Re: [HACKERS] index-only count(*) for indexes supporting bitmap scans

2017-04-12 Thread Alexander Kuzmenkov
On 12.04.2017 12:29, Alexander Korotkov wrote: That's a cool feature for FTS users! Please, register this patch to the next commitfest. I've added this to the 2017-07 commitfest: https://commitfest.postgresql.org/14/1117/ Also, what is planning overhead of this patch? That's worth t

Re: [HACKERS] index-only count(*) for indexes supporting bitmap scans

2017-04-12 Thread Alexander Kuzmenkov
On 12.04.2017 17:24, Tom Lane wrote: TBH, I'm not sure you need to do any of that work. Have you got evidence that the planner will fail to choose the right plan regardless? I'm particularly unconvinced that choose_bitmap_and is a critical problem, because once you're into having to AND multiple

Re: [HACKERS] index-only count(*) for indexes supporting bitmap scans

2017-04-12 Thread Tom Lane
Alexander Kuzmenkov writes: > With planner, the changes are more complex. Two things must be done > there. First, when the tlist is empty, we must use a different cost > function for bitmap heap scan, because the heap access pattern is > different. Second, choose_bitmap_and() must use a differe

Re: [HACKERS] index-only count(*) for indexes supporting bitmap scans

2017-04-12 Thread Alexander Kuzmenkov
On 12.04.2017 15:04, Tom Lane wrote: Andrew Gierth writes: "Alexander" == Alexander Kuzmenkov writes: Alexander> Structurally, the patch consists of two major parts: a Alexander> specialized executor node Why? It strikes me that the significant fact here is not that we're doing count(*), b

Re: [HACKERS] index-only count(*) for indexes supporting bitmap scans

2017-04-12 Thread Tom Lane
Andrew Gierth writes: > "Alexander" == Alexander Kuzmenkov writes: > Alexander> Structurally, the patch consists of two major parts: a > Alexander> specialized executor node > Why? > It strikes me that the significant fact here is not that we're doing > count(*), but that we don't need any co

Re: [HACKERS] index-only count(*) for indexes supporting bitmap scans

2017-04-12 Thread Andrew Gierth
> "Alexander" == Alexander Kuzmenkov writes: Alexander> Structurally, the patch consists of two major parts: a Alexander> specialized executor node Why? It strikes me that the significant fact here is not that we're doing count(*), but that we don't need any columns from the bitmap heap s

Re: [HACKERS] index-only count(*) for indexes supporting bitmap scans

2017-04-12 Thread Alexander Korotkov
On Wed, Apr 12, 2017 at 12:40 AM, Alexander Korotkov < a.korot...@postgrespro.ru> wrote: > On Tue, Apr 11, 2017 at 8:24 PM, Alexander Kuzmenkov < > a.kuzmen...@postgrespro.ru> wrote: > >> I would like to propose a patch that speeds up the queries of the form >> 'select >> count(*) ... where ...',

Re: [HACKERS] index-only count(*) for indexes supporting bitmap scans

2017-04-11 Thread Alexander Korotkov
On Tue, Apr 11, 2017 at 8:24 PM, Alexander Kuzmenkov < a.kuzmen...@postgrespro.ru> wrote: > I would like to propose a patch that speeds up the queries of the form > 'select > count(*) ... where ...', where the restriction clauses can be satisfied > by some > indexes. At the moment, such queries u

[HACKERS] index-only count(*) for indexes supporting bitmap scans

2017-04-11 Thread Alexander Kuzmenkov
Hi, I would like to propose a patch that speeds up the queries of the form 'select count(*) ... where ...', where the restriction clauses can be satisfied by some indexes. At the moment, such queries use index-only scans followed by aggregation. Index-only scans are only possible for indexes

Re: [HACKERS] Index usage for elem-contained-by-const-range clauses

2017-03-28 Thread David Steele
On 3/24/17 10:50 AM, David Steele wrote: Hi Pritam, On 3/17/17 5:41 PM, Pritam Baral wrote: So sorry. I'm attaching the correct version of the original with this, in case you want to test the limited implementation, because I still have to go through Tom's list of suggestions. BTW, the patch

Re: [HACKERS] Index usage for elem-contained-by-const-range clauses

2017-03-27 Thread Alexander Korotkov
On Sat, Mar 18, 2017 at 12:41 AM, Pritam Baral wrote: > On Friday 10 March 2017 07:59 PM, Alexander Korotkov wrote: > >> Hi, Pritam! > > I've assigned to review this patch. > > On Thu, Feb 23, >> 2017 at 2:17 AM, Pritam Baral wrote: > > >> The topic has been previously discussed[0] on the -per

Re: [HACKERS] Index usage for elem-contained-by-const-range clauses

2017-03-24 Thread David Steele
Hi Pritam, On 3/17/17 5:41 PM, Pritam Baral wrote: So sorry. I'm attaching the correct version of the original with this, in case you want to test the limited implementation, because I still have to go through Tom's list of suggestions. BTW, the patch is for applying on top of REL9_6_2, and wh

Re: [HACKERS] Index usage for elem-contained-by-const-range clauses

2017-03-17 Thread Pritam Baral
On Friday 10 March 2017 07:59 PM, Alexander Korotkov wrote: Hi, Pritam! > > I've assigned to review this patch. > > On Thu, Feb 23, 2017 at 2:17 AM, Pritam Baral wrote: > > The topic has been previously discussed[0] on the -performance mailing list, > about four years ago. > > In tha

Re: [HACKERS] Index usage for elem-contained-by-const-range clauses

2017-03-17 Thread Pritam Baral
On Sunday 12 March 2017 01:58 AM, Jim Nasby wrote: On 3/10/17 8:29 AM, Alexander Korotkov wrote: >> That's cool idea. But I would say more. Sometimes it's useful to >> transform "intcol between x and y" into "intcol <@ 'x,y'::int4range". >> btree_gin supports "intcol between x and y" as over

Re: [HACKERS] Index usage for elem-contained-by-const-range clauses

2017-03-17 Thread Tom Lane
I wrote: > * You're not bothering to insert any inputcollid into the generated > comparison operator nodes. I'm not sure why that fails to fall over > for text comparisons (if indeed it does fail ...) but it's wrong. > Use the range type's collation there. Oh ... looking at this again, I realize

Re: [HACKERS] Index usage for elem-contained-by-const-range clauses

2017-03-14 Thread Tom Lane
Pritam Baral writes: > The topic has been previously discussed[0] on the -performance mailing list, > about four years ago. > In that thread, Tom suggested[0] the planner could be made to "expand > "intcol <@ > 'x,y'::int4range" into "intcol between x and y", using something similar > to the > ind

Re: [HACKERS] Index usage for elem-contained-by-const-range clauses

2017-03-11 Thread Jim Nasby
On 3/10/17 8:29 AM, Alexander Korotkov wrote: That's cool idea. But I would say more. Sometimes it's useful to transform "intcol between x and y" into "intcol <@ 'x,y'::int4range". btree_gin supports "intcol between x and y" as overlap of "intcol >= x" and "intcol <= y". That is very ineffici

Re: [HACKERS] Index usage for elem-contained-by-const-range clauses

2017-03-10 Thread Alexander Korotkov
Hi, Pritam! I've assigned to review this patch. On Thu, Feb 23, 2017 at 2:17 AM, Pritam Baral wrote: > The topic has been previously discussed[0] on the -performance mailing > list, > about four years ago. > > In that thread, Tom suggested[0] the planner could be made to "expand > "intcol <@ >

Re: [HACKERS] Index usage for elem-contained-by-const-range clauses

2017-02-26 Thread Robert Haas
On Thu, Feb 23, 2017 at 4:47 AM, Pritam Baral wrote: > The topic has been previously discussed[0] on the -performance mailing list, > about four years ago. > > In that thread, Tom suggested[0] the planner could be made to "expand > "intcol <@ > 'x,y'::int4range" into "intcol between x and y", usin

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-23 Thread Jim Nasby
On 2/19/17 5:27 AM, Robert Haas wrote: (1) a multi-batch hash join, (2) a nested loop, and (3) a merge join. (2) is easy to implement but will generate a ton of random I/O if the table is not resident in RAM. (3) is most suitable for very large tables but takes more work to code, and is also li

[HACKERS] Index usage for elem-contained-by-const-range clauses

2017-02-22 Thread Pritam Baral
The topic has been previously discussed[0] on the -performance mailing list, about four years ago. In that thread, Tom suggested[0] the planner could be made to "expand "intcol <@ 'x,y'::int4range" into "intcol between x and y", using something similar to the index LIKE optimization (ie, the "spec

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-19 Thread Robert Haas
On Sun, Feb 19, 2017 at 3:52 PM, Pavan Deolasee wrote: > This particular case of corruption results in a heap tuple getting indexed > by a wrong key (or to be precise, indexed by its old value). So the only way > to detect the corruption is to look at each index key and check if it > matches with

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-19 Thread Pavan Deolasee
On Sun, Feb 19, 2017 at 3:43 PM, Robert Haas wrote: > On Fri, Feb 17, 2017 at 11:15 PM, Tom Lane wrote: > > > Ah, nah, scratch that. If any post-index-build pruning had occurred on a > > page, the evidence would be gone --- the non-matching older tuples would > > be removed and what remained wo

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-19 Thread Robert Haas
On Fri, Feb 17, 2017 at 11:15 PM, Tom Lane wrote: > I wrote: >> However, you might be able to find it without so much random I/O. >> I'm envisioning a seqscan over the table, in which you simply look for >> HOT chains in which the indexed columns aren't all the same. When you >> find one, you'd h

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-17 Thread Peter Geoghegan
On Fri, Feb 17, 2017 at 9:31 AM, Tom Lane wrote: > This seems like it'd be quite a different tool than amcheck, though. > Also, it would only find broken-HOT-chain corruption, which might be > a rare enough issue to not deserve a single-purpose tool. FWIW, my ambition for amcheck is that it will

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-17 Thread Tom Lane
I wrote: > However, you might be able to find it without so much random I/O. > I'm envisioning a seqscan over the table, in which you simply look for > HOT chains in which the indexed columns aren't all the same. When you > find one, you'd have to do a pretty expensive index lookup to confirm > wh

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-17 Thread Tom Lane
Peter Geoghegan writes: > The difference with a test that could detect this variety of > corruption is that that would need to visit the heap, which tends to > be much larger than any one index, or even all indexes. That would > probably need to be random I/O, too. It might be possible to mostly >

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-17 Thread Peter Geoghegan
On Fri, Feb 17, 2017 at 8:23 AM, Keith Fiske wrote: > It's not the load I'm worried about, it's the locks that are required at > some point during the rebuild. Doing an index rebuild here and there isn't a > big deal, but trying to do it for an entire heavily loaded, multi-terabyte > database is h

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-17 Thread Alvaro Herrera
Keith Fiske wrote: > I can understandable if it's simply not possible, but if it is, I think in > any cases of data corruption, having some means to check for it to be sure > you're in the clear would be useful. Maybe it is possible. I just didn't try, since it didn't seem very useful. -- Álva

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-17 Thread Keith Fiske
On Fri, Feb 17, 2017 at 11:12 AM, Alvaro Herrera wrote: > Keith Fiske wrote: > > > Was just curious if anyone was able to come up with any sort of method to > > test whether an index was corrupted by this bug, other than just waiting > > for bad query results? We've used concurrent index rebuildi

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-17 Thread Alvaro Herrera
Keith Fiske wrote: > Was just curious if anyone was able to come up with any sort of method to > test whether an index was corrupted by this bug, other than just waiting > for bad query results? We've used concurrent index rebuilding quite > extensively over the years to remove bloat from busy sys

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-17 Thread Keith Fiske
On Mon, Feb 6, 2017 at 10:17 PM, Amit Kapila wrote: > On Mon, Feb 6, 2017 at 10:28 PM, Tom Lane wrote: > > Amit Kapila writes: > >> Hmm. Consider that the first time relcahe invalidation occurs while > >> computing id_attrs, so now the retry logic will compute the correct > >> set of attrs (co

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-06 Thread Amit Kapila
On Mon, Feb 6, 2017 at 10:28 PM, Tom Lane wrote: > Amit Kapila writes: >> Hmm. Consider that the first time relcahe invalidation occurs while >> computing id_attrs, so now the retry logic will compute the correct >> set of attrs (considering two indexes, if we take the example given by >> Alvaro

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-06 Thread Pavan Deolasee
On Mon, Feb 6, 2017 at 11:54 PM, Tom Lane wrote: > After some discussion among the release team, we've concluded that the > best thing to do is to push Pavan's/my patch into today's releases. > This does not close the matter by any means: we should continue to > study whether there are related bu

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-06 Thread Tom Lane
After some discussion among the release team, we've concluded that the best thing to do is to push Pavan's/my patch into today's releases. This does not close the matter by any means: we should continue to study whether there are related bugs or whether there's a more principled way of fixing this

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-06 Thread Robert Treat
On Sun, Feb 5, 2017 at 9:42 PM, Pavan Deolasee wrote: > On Mon, Feb 6, 2017 at 5:41 AM, Peter Geoghegan wrote: >> On Sun, Feb 5, 2017 at 4:09 PM, Robert Haas wrote: >> > I don't think this kind of black-and-white thinking is very helpful. >> > Obviously, data corruption is bad. However, this bu

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-06 Thread Tom Lane
Alvaro Herrera writes: > Tom Lane wrote: >> Better to fix the callers so that they don't have the assumption you >> refer to. Or maybe we could adjust the API of RelationGetIndexAttrBitmap >> so that it returns all the sets needed by a given calling module at >> once, which would allow us to guar

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-06 Thread Alvaro Herrera
Tom Lane wrote: > Better to fix the callers so that they don't have the assumption you > refer to. Or maybe we could adjust the API of RelationGetIndexAttrBitmap > so that it returns all the sets needed by a given calling module at > once, which would allow us to guarantee they're consistent. No

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-06 Thread Tom Lane
Amit Kapila writes: > Hmm. Consider that the first time relcahe invalidation occurs while > computing id_attrs, so now the retry logic will compute the correct > set of attrs (considering two indexes, if we take the example given by > Alvaro above.). However, the attrs computed for hot_* and key

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-06 Thread Alvaro Herrera
Andres Freund wrote: > To show what I mean here's an *unpolished* and *barely tested* patch > implementing the first of my suggestions. > > Alvaro, Pavan, I think should address the issue as well? Hmm, interesting idea. Maybe a patch along these lines is a good way to make index-list cache less

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-06 Thread Alvaro Herrera
Tom Lane wrote: > Pavan Deolasee writes: > > 2. In the second patch, we tried to recompute attribute lists if a relcache > > flush happens in between and index list is invalidated. We've seen problems > > with that, especially it getting into an infinite loop with > > CACHE_CLOBBER_ALWAYS. Not c

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-06 Thread Martín Marqués
El 05/02/17 a las 21:57, Tomas Vondra escribió: > > +1 to not rushing fixes into releases. While I think we now finally > understand the mechanics of this bug, the fact that we came up with > three different fixes in this thread, only to discover issues with each > of them, warrants some caution.

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-06 Thread Vladimir Borodin
> 6 февр. 2017 г., в 4:57, Peter Geoghegan написал(а): > > I meant that I find the fact that there were no user reports in all > these years to be a good reason to not proceed for now in this > instance. Well, we had some strange situations with indexes (see below for example) but I couldn’t e

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Amit Kapila
On Mon, Feb 6, 2017 at 9:47 AM, Pavan Deolasee wrote: > > > On Mon, Feb 6, 2017 at 9:41 AM, Amit Kapila wrote: >> >> >> >> Hmm. Consider that the first time relcahe invalidation occurs while >> computing id_attrs, so now the retry logic will compute the correct >> set of attrs (considering two i

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Pavan Deolasee
On Mon, Feb 6, 2017 at 8:01 AM, Pavan Deolasee wrote: > >> > I like this approach. I'll run the patch on a build with > CACHE_CLOBBER_ALWAYS, but I'm pretty sure it will be ok. > While it looks certain that the fix will miss this point release, FWIW I ran this patch with CACHE_CLOBBER_ALWAYS and

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Pavan Deolasee
On Mon, Feb 6, 2017 at 9:41 AM, Amit Kapila wrote: > > > Hmm. Consider that the first time relcahe invalidation occurs while > computing id_attrs, so now the retry logic will compute the correct > set of attrs (considering two indexes, if we take the example given by > Alvaro above.). I don't

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Amit Kapila
On Mon, Feb 6, 2017 at 8:35 AM, Pavan Deolasee wrote: > > > On Mon, Feb 6, 2017 at 8:15 AM, Amit Kapila wrote: >> >> On Mon, Feb 6, 2017 at 8:01 AM, Pavan Deolasee >> wrote: >> > >> > >> > On Mon, Feb 6, 2017 at 1:44 AM, Tom Lane wrote: >> >> >> >> >> >> >> >> > 2. In the second patch, we tried

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Andres Freund
On 2017-02-05 22:34:34 -0500, Tom Lane wrote: > Pavan Deolasee writes: > The point is that there's a nontrivial chance of a hasty fix introducing > worse problems than we fix. > > Given the lack of consensus about exactly how to fix this, I'm feeling > like it's a good idea if whatever we come up

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Tom Lane
Pavan Deolasee writes: > On Mon, Feb 6, 2017 at 5:44 AM, Andres Freund wrote: >> +1. I don't think we serve our users by putting out a nontrivial bugfix >> hastily. Nor do I think we serve them in this instance by delaying the >> release to cover this fix; there's enough other fixes in the relea

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Andres Freund
On 2017-02-05 21:49:57 -0500, Tom Lane wrote: > Andres Freund writes: > > I've not yet read the full thread, but I'm a bit confused so far. We > > obviously can get changing information about indexes here, but isn't > > that something we have to deal with anyway? If we guarantee that we > > don't

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Pavan Deolasee
On Mon, Feb 6, 2017 at 5:44 AM, Andres Freund wrote: > On 2017-02-05 12:51:09 -0500, Tom Lane wrote: > > Michael Paquier writes: > > > On Sun, Feb 5, 2017 at 6:53 PM, Pavel Stehule > wrote: > > >> I agree with Pavan - a release with known important bug is not good > idea. > > > > > This bug has

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Pavan Deolasee
On Mon, Feb 6, 2017 at 8:15 AM, Amit Kapila wrote: > On Mon, Feb 6, 2017 at 8:01 AM, Pavan Deolasee > wrote: > > > > > > On Mon, Feb 6, 2017 at 1:44 AM, Tom Lane wrote: > >> > >> > >> > >> > 2. In the second patch, we tried to recompute attribute lists if a > >> > relcache > >> > flush happens

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Tom Lane
Andres Freund writes: > I've not yet read the full thread, but I'm a bit confused so far. We > obviously can get changing information about indexes here, but isn't > that something we have to deal with anyway? If we guarantee that we > don't loose knowledge that there's a pending invalidation, wh

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Andres Freund
On 2017-02-06 08:08:01 +0530, Amit Kapila wrote: > I don't see in your patch that you are setting rd_bitmapsvalid to 0. IIRC a plain relcache rebuild should do that (note there's also no place that directly resets rd_indexattrs). > Also, I think this suffers from the same problem as the patch pr

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Peter Geoghegan
On Sun, Feb 5, 2017 at 6:42 PM, Pavan Deolasee wrote: > I'm not sure that just because the bug wasn't reported by a user, makes it > any less critical. As Tomas pointed down thread, the nature of the bug is > such that the users may not discover it very easily, but that doesn't mean > it couldn't

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Amit Kapila
On Mon, Feb 6, 2017 at 8:01 AM, Pavan Deolasee wrote: > > > On Mon, Feb 6, 2017 at 1:44 AM, Tom Lane wrote: >> >> >> >> > 2. In the second patch, we tried to recompute attribute lists if a >> > relcache >> > flush happens in between and index list is invalidated. We've seen >> > problems >> > wit

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Pavan Deolasee
On Mon, Feb 6, 2017 at 5:41 AM, Peter Geoghegan wrote: > On Sun, Feb 5, 2017 at 4:09 PM, Robert Haas wrote: > > I don't think this kind of black-and-white thinking is very helpful. > > Obviously, data corruption is bad. However, this bug has (from what > > one can tell from this thread) been wi

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Amit Kapila
On Mon, Feb 6, 2017 at 6:27 AM, Andres Freund wrote: > Hi, > > On 2017-02-05 16:37:33 -0800, Andres Freund wrote: >> > RelationGetIndexList(Relation relation) >> > @@ -4746,8 +4747,10 @@ RelationGetIndexPredicate(Relation relat >> > * we can include system attributes (e.g., OID) in the bitmap

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Pavan Deolasee
On Mon, Feb 6, 2017 at 1:44 AM, Tom Lane wrote: > > > > 2. In the second patch, we tried to recompute attribute lists if a > relcache > > flush happens in between and index list is invalidated. We've seen > problems > > with that, especially it getting into an infinite loop with > > CACHE_CLOBBER

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Peter Geoghegan
On Sun, Feb 5, 2017 at 4:57 PM, Tomas Vondra wrote: > OTOH I disagree with the notion that bugs that are not driven by user > reports are somehow less severe. Some data corruption bugs cause quite > visible breakage - segfaults, immediate crashes, etc. Those are pretty clear > bugs, and are report

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Andres Freund
Hi, On 2017-02-05 16:37:33 -0800, Andres Freund wrote: > > RelationGetIndexList(Relation relation) > > @@ -4746,8 +4747,10 @@ RelationGetIndexPredicate(Relation relat > > * we can include system attributes (e.g., OID) in the bitmap > > representation. > > * > > * Caller had better hold at

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Tomas Vondra
On 02/06/2017 01:11 AM, Peter Geoghegan wrote: On Sun, Feb 5, 2017 at 4:09 PM, Robert Haas wrote: I don't think this kind of black-and-white thinking is very helpful. Obviously, data corruption is bad. However, this bug has (from what one can tell from this thread) been with us for over a decad

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Andres Freund
On 2017-02-05 15:14:59 -0500, Tom Lane wrote: > I do not like any of the other patches proposed in this thread, because > they fail to guarantee delivering an up-to-date attribute bitmap to the > caller. I think we need a retry loop, and I think that it needs to look > like the attached. That loo

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Andres Freund
On 2017-02-05 12:51:09 -0500, Tom Lane wrote: > Michael Paquier writes: > > On Sun, Feb 5, 2017 at 6:53 PM, Pavel Stehule > > wrote: > >> I agree with Pavan - a release with known important bug is not good idea. > > > This bug has been around for some time, so I would recommend taking > > the t

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Peter Geoghegan
On Sun, Feb 5, 2017 at 4:09 PM, Robert Haas wrote: > I don't think this kind of black-and-white thinking is very helpful. > Obviously, data corruption is bad. However, this bug has (from what > one can tell from this thread) been with us for over a decade; it must > necessarily be either low-prob

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Robert Haas
On Sun, Feb 5, 2017 at 1:34 PM, Martín Marqués wrote: > El 05/02/17 a las 10:03, Michael Paquier escribió: >> On Sun, Feb 5, 2017 at 6:53 PM, Pavel Stehule >> wrote: >>> I agree with Pavan - a release with known important bug is not good idea. >> >> This bug has been around for some time, so I w

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Tom Lane
[ Having now read the whole thread, I'm prepared to weigh in ... ] Pavan Deolasee writes: > This seems like a real problem to me. I don't know what the consequences > are, but definitely having various attribute lists to have different view > of the set of indexes doesn't seem right. For sure.

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Martín Marqués
El 05/02/17 a las 10:03, Michael Paquier escribió: > On Sun, Feb 5, 2017 at 6:53 PM, Pavel Stehule wrote: >> I agree with Pavan - a release with known important bug is not good idea. > > This bug has been around for some time, so I would recommend taking > the time necessary to make the best fix

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Pavel Stehule
2017-02-05 18:51 GMT+01:00 Tom Lane : > Michael Paquier writes: > > On Sun, Feb 5, 2017 at 6:53 PM, Pavel Stehule > wrote: > >> I agree with Pavan - a release with known important bug is not good > idea. > > > This bug has been around for some time, so I would recommend taking > > the time neces

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Tom Lane
Michael Paquier writes: > On Sun, Feb 5, 2017 at 6:53 PM, Pavel Stehule wrote: >> I agree with Pavan - a release with known important bug is not good idea. > This bug has been around for some time, so I would recommend taking > the time necessary to make the best fix possible, even if it means >

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Michael Paquier
On Sun, Feb 5, 2017 at 6:53 PM, Pavel Stehule wrote: > I agree with Pavan - a release with known important bug is not good idea. This bug has been around for some time, so I would recommend taking the time necessary to make the best fix possible, even if it means waiting for the next round of min

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-05 Thread Pavel Stehule
2017-02-05 7:54 GMT+01:00 Pavan Deolasee : > > On Sat, Feb 4, 2017 at 11:54 PM, Tom Lane wrote: > >> >> Based on Pavan's comments, I think trying to force this into next week's >> releases would be extremely unwise. If the bug went undetected this long, >> it can wait for a fix for another three

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-04 Thread Pavan Deolasee
On Sat, Feb 4, 2017 at 11:54 PM, Tom Lane wrote: > > Based on Pavan's comments, I think trying to force this into next week's > releases would be extremely unwise. If the bug went undetected this long, > it can wait for a fix for another three months. Yes, I think bug existed ever since and we

Re: [HACKERS] Index corruption with CREATE INDEX CONCURRENTLY

2017-02-04 Thread Tom Lane
Alvaro Herrera writes: > I intend to commit this soon to all branches, to ensure it gets into the > next set of minors. Based on Pavan's comments, I think trying to force this into next week's releases would be extremely unwise. If the bug went undetected this long, it can wait for a fix for ano

  1   2   3   4   5   6   7   8   9   10   >