Re: [HACKERS] BRIN range operator class
Emre Hasegeli wrote: I pushed patches 04 and 07, as well as adopting some of the changes to the regression test in 06. I'm afraid I caused a bit of merge pain for you -- sorry about that. No problem. I rebased the remaining ones. Thanks, pushed. There was a proposed change by Emre to renumber operator -|- to 17 for range types (from 6 I think). I didn't include that as I think it should be a separate commit. Also, we're now in debt of the test strategy for the union procedure. I will work with Emre in the coming days to get that sorted out. I'm now thinking that something in src/test/modules is the most appropriate. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
Emre Hasegeli wrote:
I pushed patches 04 and 07, as well as adopting some of the changes to
the regression test in 06. I'm afraid I caused a bit of merge pain for
you -- sorry about that.
No problem. I rebased the remaining ones.
Thanks!
After some back-and-forth between Emre and me, here's an updated patch.
My changes are cosmetic; for a detailed rundown, see
https://github.com/alvherre/postgres/commits/brin-inclusion
Note that datatype point was removed: it turns out that unless we get
box_contain_pt changed to use FPlt() et al, indexes created with this
opclass would be corrupt. And we cannot simply change box_contain_pt,
because that would break existing GiST and SP-GiST indexes that use it
today and pg_upgrade to 9.5! So that needs to be considered separately.
Also, removing point support means remove the CAST support procedure,
because there is no use for it in the supported types. Also, patch 05
in the previous submissions goes away completely because there's no need
for those (box,point) operators anymore.
There's nothing Earth-shattering here that hasn't been seen in previous
submissions by Emre.
One item of note is that this patch is blindly removing the assert-only
blocks as previously discussed, without any replacement. Need to think
more on how to put something back ...
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training Services
diff --git a/doc/src/sgml/brin.sgml b/doc/src/sgml/brin.sgml
index 92dac7c..b26bcf3 100644
--- a/doc/src/sgml/brin.sgml
+++ b/doc/src/sgml/brin.sgml
@@ -72,7 +72,9 @@
para
The firsttermminmax/
operator classes store the minimum and the maximum values appearing
- in the indexed column within the range.
+ in the indexed column within the range. The firstterminclusion/
+ operator classes store a value which includes the values in the indexed
+ column within the range.
/para
table id=brin-builtin-opclasses-table
@@ -252,6 +254,18 @@
/entry
/row
row
+ entryliteralinet_inclusion_ops/literal/entry
+ entrytypeinet/type/entry
+ entry
+ literalamp;amp;/
+ literalgt;gt;/
+ literalgt;gt;=/
+ literallt;lt;/literal
+ literallt;lt;=/literal
+ literal=/literal
+ /entry
+/row
+row
entryliteralbpchar_minmax_ops/literal/entry
entrytypecharacter/type/entry
entry
@@ -373,6 +387,25 @@
/entry
/row
row
+ entryliteralrange_inclusion_ops//entry
+ entrytypeany range type/type/entry
+ entry
+ literalamp;amp;/
+ literalamp;gt;/
+ literalamp;lt;/
+ literalgt;gt;/
+ literallt;lt;/
+ literallt;@/
+ literal=/
+ literal@gt;/
+ literallt;/literal
+ literallt;=/literal
+ literal=/literal
+ literalgt;=/literal
+ literalgt;/literal
+ /entry
+/row
+row
entryliteralpg_lsn_minmax_ops/literal/entry
entrytypepg_lsn/type/entry
entry
@@ -383,6 +416,43 @@
literalgt;/literal
/entry
/row
+row
+ entryliteralbox_inclusion_ops//entry
+ entrytypebox/type/entry
+ entry
+ literalamp;amp;/
+ literalamp;gt;/
+ literalamp;lt;/
+ literalgt;gt;/
+ literallt;lt;/
+ literallt;@/
+ literal~=/
+ literal@gt;/
+ literalamp;gt;|/
+ literal|amp;lt;/
+ literalgt;gt;|/
+ literal|lt;lt;/literal
+ /entry
+/row
+row
+ entryliteralpoint_box_inclusion_ops//entry
+ entrytypepoint/type/entry
+ entry
+ literalamp;amp;/
+ literalamp;gt;/
+ literalamp;lt;/
+ literalgt;gt;/
+ literallt;lt;/
+ literallt;@/
+ literal~=/
+ literalamp;gt;|/
+ literal|amp;lt;/
+ literalgt;gt;|/
+ literal|lt;lt;/literal
+ literalgt;^/
+ literal|lt;^/literal
+ /entry
+/row
/tbody
/tgroup
/table
diff --git a/src/backend/access/brin/Makefile b/src/backend/access/brin/Makefile
index ac44fcd..fb36882 100644
--- a/src/backend/access/brin/Makefile
+++ b/src/backend/access/brin/Makefile
@@ -13,6 +13,6 @@ top_builddir = ../../../..
include $(top_builddir)/src/Makefile.global
OBJS = brin.o brin_pageops.o brin_revmap.o brin_tuple.o brin_xlog.o \
- brin_minmax.o
+ brin_minmax.o brin_inclusion.o
include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 2b5fb8d..1995125 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -105,11 +105,6 @@ brininsert(PG_FUNCTION_ARGS)
BrinMemTuple *dtup;
BlockNumber heapBlk;
int keyno;
-#ifdef USE_ASSERT_CHECKING
- BrinTuple *tmptup;
- BrinMemTuple *tmpdtup;
- Size tmpsiz;
-#endif
CHECK_FOR_INTERRUPTS();
@@ -137,45 +132,6 @@ brininsert(PG_FUNCTION_ARGS)
dtup = brin_deform_tuple(bdesc, brtup);
-#ifdef USE_ASSERT_CHECKING
- {
- /*
- * When assertions are enabled, we use this as an opportunity
Re: [HACKERS] BRIN range operator class
Alvaro Herrera wrote: In patch 05, you use straight etc comparisons of point/box values. All the other code in that file AFAICS uses FPlt() macros and others; I assume we should do likewise. Oooh, looking at the history of this I just realized that the comments signed tgl are actually Thomas G. Lockhart, not Tom G. Lane! See commit 9e2a87b62db87fc4175b00dabfd26293a2d072fa -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
Emre Hasegeli wrote: I pushed patches 04 and 07, as well as adopting some of the changes to the regression test in 06. I'm afraid I caused a bit of merge pain for you -- sorry about that. No problem. I rebased the remaining ones. In patch 05, you use straight etc comparisons of point/box values. All the other code in that file AFAICS uses FPlt() macros and others; I assume we should do likewise. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
So, in reading these patches, it came to me that we might want to have pg_upgrade mark indexes invalid if we in the future change the implementation of some opclass. For instance, the inclusion opclass submitted here uses three columns: the indexed value itself, plus two booleans; each of these booleans is a workaround for some nasty design decision in the underlying datatypes. One boolean is unmergeable: if a block range contains both IPv4 and IPv6 addresses, we mark it as 'unmergeable' and then every query needs to visit that block range always. The other boolean is contains empty and is used for range types: it is set if the empty value is present somewhere in the block range. If in the future, for instance, we come up with a way to store the ipv4 plus ipv6 info, we will want to change the page format. If we add a page version to the metapage, we can detect the change at pg_upgrade time and force a reindex of the index. Thoughts? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
On 05/12/2015 10:49 PM, Alvaro Herrera wrote:
If in the future, for instance, we come up with a way to store the ipv4
plus ipv6 info, we will want to change the page format. If we add a
page version to the metapage, we can detect the change at pg_upgrade
time and force a reindex of the index.
A version number in the metapage is a certainly a good idea. But we
already have that, don't we? :
/* Metapage definitions */
typedef struct BrinMetaPageData
{
uint32 brinMagic;
uint32 brinVersion;
BlockNumber pagesPerRange;
BlockNumber lastRevmapPage;
} BrinMetaPageData;
#define BRIN_CURRENT_VERSION1
#define BRIN_META_MAGIC 0xA8109CFA
Did you have something else in mind?
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
Heikki Linnakangas wrote:
On 05/12/2015 10:49 PM, Alvaro Herrera wrote:
If in the future, for instance, we come up with a way to store the ipv4
plus ipv6 info, we will want to change the page format. If we add a
page version to the metapage, we can detect the change at pg_upgrade
time and force a reindex of the index.
A version number in the metapage is a certainly a good idea. But we already
have that, don't we? :
/* Metapage definitions */
typedef struct BrinMetaPageData
{
uint32 brinMagic;
uint32 brinVersion;
BlockNumber pagesPerRange;
BlockNumber lastRevmapPage;
} BrinMetaPageData;
#define BRIN_CURRENT_VERSION 1
#define BRIN_META_MAGIC 0xA8109CFA
Did you have something else in mind?
Yeah, I was thinking we could have a separate version number for the
opclass code as well. An external extension could change that, for
instance. Also, we could change the 'inclusion' version and leave
minmax alone.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
I pushed patches 04 and 07, as well as adopting some of the changes to the regression test in 06. I'm afraid I caused a bit of merge pain for you -- sorry about that. No problem. I rebased the remaining ones. brin-inclusion-v09-02-strategy-numbers.patch Description: Binary data brin-inclusion-v09-03-remove-assert-checking.patch Description: Binary data brin-inclusion-v09-05-box-vs-point-operators.patch Description: Binary data brin-inclusion-v09-06-inclusion-opclasses.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
Emre Hasegeli wrote: After looking at 05 again, I don't like the same as % business. Creating a whole new class of exceptions is not my thing, particularly not in a regression test whose sole purpose is to look for exceptional (a.k.a. wrong) cases. I would much rather define the opclasses for those two datatypes using the existing @ operators rather than create operators for this purpose. We can add a note to the docs, for historical reasons the brin opclass for datatype box/point uses the @ operator instead of , or something like that. I worked around this by adding point @ box operator as the overlap strategy and removed additional operators. That works for me. I pushed patches 04 and 07, as well as adopting some of the changes to the regression test in 06. I'm afraid I caused a bit of merge pain for you -- sorry about that. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
Looking at patch 04, it seems to me that it would be better to have the OpcInfo struct carry the typecache struct rather than the type OID, so that we can avoid repeated typecache lookups in brin_deform_tuple; Here's the patch. Looks better to me. I will incorporate with this patch. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
Can you please explain what is the purpose of patch 07? I'm not sure I understand; are we trying to avoid having to add pg_amproc entries for these operators and instead piggy-back on btree opclass definitions? Not too much in love with that idea; I see that there is less tedium in that the brin opclass definition is simpler. One disadvantage is a 3x increase in the number of syscache lookups to get the function you need, unless I'm reading things wrong. Maybe this is not performance critical. It doesn't use btree opclass definitions. It uses brin opclass pg_amop entries instead of duplicating them in pg_amproc. The pg_amproc.h header says: * The amproc table identifies support procedures associated with index * operator families and classes. These procedures can't be listed in pg_amop * since they are not the implementation of any indexable operator. In our case, these procedures can be listed in pg_amop as they are implementations of indexable operators. The more important change on this patch is to request procedures for the right data types. Minmax opclasses return wrong results without this patch. You can reproduce it with this query on the regression database: select * from brintest where timestampcol = '1979-01-29 11:05:09'::timestamptz; Anyway I tried applying it on isolation, and found that it fails the assertion that tests the union support proc in brininsert. That doesn't seem okay. I mean, it's okay not to run the test for the inclusion opclasses, but why does it now fail in minmax which was previously passing? Couldn't figure it out. The regression tests passed when I tried it on the current master. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
Alvaro Herrera alvhe...@2ndquadrant.com writes: Let's think together and try to find a reasonable way to get the union procedures tested regularly. It is pretty clear that having them run only when the race condition occurs is not acceptable; bugs go unnoticed. [ just a drive-by comment... ] Maybe you could set up a testing mode that forces the race condition to occur? Then you could test the calling code paths, not only the union procedures per se. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
I again have to refuse the notion that removing the assert-only block without any replacement is acceptable. I just spent a lot of time tracking down what turned out to be a bug in your patch 07: /* Adjust maximum, if B's max is greater than A's max */ - needsadj = FunctionCall2Coll(minmax_get_procinfo(bdesc, attno, - PROCNUM_GREATER), - colloid, col_b-bv_values[1], col_a-bv_values[1]); + frmg = minmax_get_strategy_procinfo(bdesc, attno, attr-atttypid, + BTGreaterStrategyNumber); + needsadj = FunctionCall2Coll(frmg, colloid, col_b-bv_values[0], + col_a-bv_values[0]); Note the removed lines use array index 1, while the added lines use array index 0. The only reason I noticed this is because I applied this patch without the others and saw the assertion fire; how would I have noticed the problem had I just removed it? Let's think together and try to find a reasonable way to get the union procedures tested regularly. It is pretty clear that having them run only when the race condition occurs is not acceptable; bugs go unnoticed. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
On 05/05/2015 01:10 PM, Emre Hasegeli wrote: I already replied his email [1]. Which issues do you mean? Sorry, my bad please ignore the previous email. -- Andreas Karlsson -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
Looking at patch 04, it seems to me that it would be better to have
the OpcInfo struct carry the typecache struct rather than the type OID,
so that we can avoid repeated typecache lookups in brin_deform_tuple;
something like
/* struct returned by OpcInfo amproc */
typedef struct BrinOpcInfo
{
/* Number of columns stored in an index column of this opclass */
uint16 oi_nstored;
/* Opaque pointer for the opclass' private use */
void *oi_opaque;
/* Typecache entries of the stored columns */
TypeCacheEntry oi_typcache[FLEXIBLE_ARRAY_MEMBER];
} BrinOpcInfo;
Looking into it now.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
Alvaro Herrera wrote:
Looking at patch 04, it seems to me that it would be better to have
the OpcInfo struct carry the typecache struct rather than the type OID,
so that we can avoid repeated typecache lookups in brin_deform_tuple;
Here's the patch.
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training Services
commit 53f9b1ac9a4b73eddcb7acc99aeacff34336f7ad
Author: Alvaro Herrera alvhe...@alvh.no-ip.org
Date: Tue May 5 16:19:53 2015 -0300
use typcache rather than oid
diff --git a/contrib/pageinspect/brinfuncs.c b/contrib/pageinspect/brinfuncs.c
index 1b15a7b..bd3191d 100644
--- a/contrib/pageinspect/brinfuncs.c
+++ b/contrib/pageinspect/brinfuncs.c
@@ -181,7 +181,7 @@ brin_page_items(PG_FUNCTION_ARGS)
column-nstored = opcinfo-oi_nstored;
for (i = 0; i opcinfo-oi_nstored; i++)
{
-getTypeOutputInfo(opcinfo-oi_typids[i], output, isVarlena);
+getTypeOutputInfo(opcinfo-oi_typcache[i]-type_id, output, isVarlena);
fmgr_info(output, column-outputFn[i]);
}
diff --git a/doc/src/sgml/brin.sgml b/doc/src/sgml/brin.sgml
index 1ac282c..92dac7c 100644
--- a/doc/src/sgml/brin.sgml
+++ b/doc/src/sgml/brin.sgml
@@ -428,8 +428,8 @@ typedef struct BrinOpcInfo
/* Opaque pointer for the opclass' private use */
void *oi_opaque;
-/* Type IDs of the stored columns */
-Oid oi_typids[FLEXIBLE_ARRAY_MEMBER];
+/* Type cache entries of the stored columns */
+TypeCacheEntry *oi_typcache[FLEXIBLE_ARRAY_MEMBER];
} BrinOpcInfo;
/programlisting
structnameBrinOpcInfo/.structfieldoi_opaque/ can be used by the
diff --git a/src/backend/access/brin/brin_minmax.c b/src/backend/access/brin/brin_minmax.c
index 299d6f7..ce8652d 100644
--- a/src/backend/access/brin/brin_minmax.c
+++ b/src/backend/access/brin/brin_minmax.c
@@ -62,8 +62,8 @@ brin_minmax_opcinfo(PG_FUNCTION_ARGS)
result-oi_nstored = 2;
result-oi_opaque = (MinmaxOpaque *)
MAXALIGN((char *) result + SizeofBrinOpcInfo(2));
- result-oi_typids[0] = typoid;
- result-oi_typids[1] = typoid;
+ result-oi_typcache[0] = result-oi_typcache[1] =
+ lookup_type_cache(typoid, 0);
PG_RETURN_POINTER(result);
}
diff --git a/src/backend/access/brin/brin_tuple.c b/src/backend/access/brin/brin_tuple.c
index 08fa998..22ce74a 100644
--- a/src/backend/access/brin/brin_tuple.c
+++ b/src/backend/access/brin/brin_tuple.c
@@ -68,7 +68,7 @@ brtuple_disk_tupdesc(BrinDesc *brdesc)
{
for (j = 0; j brdesc-bd_info[i]-oi_nstored; j++)
TupleDescInitEntry(tupdesc, attno++, NULL,
- brdesc-bd_info[i]-oi_typids[j],
+ brdesc-bd_info[i]-oi_typcache[j]-type_id,
-1, 0);
}
@@ -444,8 +444,8 @@ brin_deform_tuple(BrinDesc *brdesc, BrinTuple *tuple)
for (i = 0; i brdesc-bd_info[keyno]-oi_nstored; i++)
dtup-bt_columns[keyno].bv_values[i] =
datumCopy(values[valueno++],
- brdesc-bd_tupdesc-attrs[keyno]-attbyval,
- brdesc-bd_tupdesc-attrs[keyno]-attlen);
+ brdesc-bd_info[keyno]-oi_typcache[i]-typbyval,
+ brdesc-bd_info[keyno]-oi_typcache[i]-typlen);
dtup-bt_columns[keyno].bv_hasnulls = hasnulls[keyno];
dtup-bt_columns[keyno].bv_allnulls = false;
diff --git a/src/include/access/brin_internal.h b/src/include/access/brin_internal.h
index 84eed61..1486d04 100644
--- a/src/include/access/brin_internal.h
+++ b/src/include/access/brin_internal.h
@@ -16,6 +16,7 @@
#include storage/bufpage.h
#include storage/off.h
#include utils/relcache.h
+#include utils/typcache.h
/*
@@ -32,13 +33,13 @@ typedef struct BrinOpcInfo
/* Opaque pointer for the opclass' private use */
void *oi_opaque;
- /* Type IDs of the stored columns */
- Oid oi_typids[FLEXIBLE_ARRAY_MEMBER];
+ /* Type cache entries of the stored columns */
+ TypeCacheEntry *oi_typcache[FLEXIBLE_ARRAY_MEMBER];
} BrinOpcInfo;
/* the size of a BrinOpcInfo for the given number of columns */
#define SizeofBrinOpcInfo(ncols) \
- (offsetof(BrinOpcInfo, oi_typids) + sizeof(Oid) * ncols)
+ (offsetof(BrinOpcInfo, oi_typcache) + sizeof(TypeCacheEntry *) * ncols)
typedef struct BrinDesc
{
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
After looking at 05 again, I don't like the same as % business. Creating a whole new class of exceptions is not my thing, particularly not in a regression test whose sole purpose is to look for exceptional (a.k.a. wrong) cases. I would much rather define the opclasses for those two datatypes using the existing @ operators rather than create operators for this purpose. We can add a note to the docs, for historical reasons the brin opclass for datatype box/point uses the @ operator instead of , or something like that. AFAICS this is just some pretty small changes to patches 05 and 06. Will you please resubmit? I just pushed patch 01, and I'm looking at 04 next. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
On 05/05/2015 11:57 AM, Emre Hasegeli wrote: From my point of view as a reviewer this patch set is very close to being committable. Thank you. The new versions are attached. Nice, I think it is ready now other than the issues Alvaro raised in his review[1]. Have you given those any thought? Notes 1. http://www.postgresql.org/message-id/20150406211724.gh4...@alvh.no-ip.org Andreas -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
On 05/05/2015 04:24 AM, Alvaro Herrera wrote: Stefan Keller wrote: I'm keen to see if a PostGIS specialist jumps in and adds PostGIS geometry support. Did you test the patch proposed here already? It could be a very good contribution. Indeed, I have done some testing of the patch but more people testing would be nice. Andreas -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
Indeed, I have done some testing of the patch but more people testing would be nice. The inclusion opclass should work for other data types as long required operators and SQL level support functions are supplied. Maybe it would work for PostGIS, too. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
Can you please explain what is the purpose of patch 07? I'm not sure I understand; are we trying to avoid having to add pg_amproc entries for these operators and instead piggy-back on btree opclass definitions? Not too much in love with that idea; I see that there is less tedium in that the brin opclass definition is simpler. One disadvantage is a 3x increase in the number of syscache lookups to get the function you need, unless I'm reading things wrong. Maybe this is not performance critical. Anyway I tried applying it on isolation, and found that it fails the assertion that tests the union support proc in brininsert. That doesn't seem okay. I mean, it's okay not to run the test for the inclusion opclasses, but why does it now fail in minmax which was previously passing? Couldn't figure it out. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
Nice, I think it is ready now other than the issues Alvaro raised in his review[1]. Have you given those any thought? I already replied his email [1]. Which issues do you mean? [1] http://www.postgresql.org/message-id/CAE2gYzxQ-Gk3q3jYWT=1enlebsgcgu28+1axml4omcwjbkp...@mail.gmail.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
Hi, 2015-05-05 2:51 GMT+02:00 Andreas Karlsson andr...@proxel.se: From my point of view as a reviewer this patch set is very close to being committable. I'd like to thank already now to all committers and reviewers and hope BRIN makes it into PG 9.5. As a database instructor, conference organisator and geospatial specialist I'm looking forward for this clever new index. I'm keen to see if a PostGIS specialist jumps in and adds PostGIS geometry support. Yours, S. 2015-05-05 2:51 GMT+02:00 Andreas Karlsson andr...@proxel.se: From my point of view as a reviewer this patch set is very close to being committable. = brin-inclusion-v06-01-sql-level-support-functions.patch This patch looks good. = brin-inclusion-v06-02-strategy-numbers.patch This patch looks good, but shouldn't it be merged with 07? = brin-inclusion-v06-03-remove-assert-checking.patch As you wrote earlier this is needed because the new range indexes would violate the asserts. I think it is fine to remove the assertion. = brin-inclusion-v06-04-fix-brin-deform-tuple.patch This patch looks good and can be committed separately. = brin-inclusion-v06-05-box-vs-point-operators.patch This patch looks good and can be committed separately. = brin-inclusion-v06-06-inclusion-opclasses.patch - operator classes store the union of the values in the indexed column is not technically true. It stores something which covers all of the values. - Missing space in except box and point*/. - Otherwise looks good. = brin-inclusion-v06-07-remove-minmax-amprocs.patch Shouldn't this be merged with 02? Otherwise it looks good. -- Andreas Karlsson -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
From my point of view as a reviewer this patch set is very close to being committable. = brin-inclusion-v06-01-sql-level-support-functions.patch This patch looks good. = brin-inclusion-v06-02-strategy-numbers.patch This patch looks good, but shouldn't it be merged with 07? = brin-inclusion-v06-03-remove-assert-checking.patch As you wrote earlier this is needed because the new range indexes would violate the asserts. I think it is fine to remove the assertion. = brin-inclusion-v06-04-fix-brin-deform-tuple.patch This patch looks good and can be committed separately. = brin-inclusion-v06-05-box-vs-point-operators.patch This patch looks good and can be committed separately. = brin-inclusion-v06-06-inclusion-opclasses.patch - operator classes store the union of the values in the indexed column is not technically true. It stores something which covers all of the values. - Missing space in except box and point*/. - Otherwise looks good. = brin-inclusion-v06-07-remove-minmax-amprocs.patch Shouldn't this be merged with 02? Otherwise it looks good. -- Andreas Karlsson -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
Stefan Keller wrote: Hi, 2015-05-05 2:51 GMT+02:00 Andreas Karlsson andr...@proxel.se: From my point of view as a reviewer this patch set is very close to being committable. I'd like to thank already now to all committers and reviewers and hope BRIN makes it into PG 9.5. As a database instructor, conference organisator and geospatial specialist I'm looking forward for this clever new index. Appreciated. The base BRIN code is already in 9.5, so barring significant issues you should see it in the next major release. Support for geometry types and the like is still pending, but I hope to get to it shortly. I'm keen to see if a PostGIS specialist jumps in and adds PostGIS geometry support. Did you test the patch proposed here already? It could be a very good contribution. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
On 04/06/2015 09:36 PM, Emre Hasegeli wrote: Yes but they were also required by this patch. This version adds more functions and operators. I can split them appropriately after your review. Ok, sounds fine to me. It is now split. In which order should I apply the patches? I also agree with Alvaro's comments. -- Andreas Karlsson -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
On Mon, Apr 6, 2015 at 5:17 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Thanks for the updated patch; I will at it as soon as time allows. (Not really all that soon, regrettably.) Judging from a quick look, I think patches 1 and 5 can be committed quickly; they imply no changes to other parts of BRIN. (Not sure why 1 and 5 are separate. Any reason for this?) Also patch 2. Patch 4 looks like a simple bugfix (or maybe a generalization) of BRIN framework code; should also be committable right away. Needs a closer look of course. Is this still pending? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
Robert Haas wrote: On Mon, Apr 6, 2015 at 5:17 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Thanks for the updated patch; I will at it as soon as time allows. (Not really all that soon, regrettably.) Judging from a quick look, I think patches 1 and 5 can be committed quickly; they imply no changes to other parts of BRIN. (Not sure why 1 and 5 are separate. Any reason for this?) Also patch 2. Patch 4 looks like a simple bugfix (or maybe a generalization) of BRIN framework code; should also be committable right away. Needs a closer look of course. Is this still pending? Yeah. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
Judging from a quick look, I think patches 1 and 5 can be committed quickly; they imply no changes to other parts of BRIN. (Not sure why 1 and 5 are separate. Any reason for this?) Also patch 2. Not much reason except that 1 includes only functions, but 5 includes operators. Patch 4 looks like a simple bugfix (or maybe a generalization) of BRIN framework code; should also be committable right away. Needs a closer look of course. Patch 3 is a problem. That code is there because the union proc is only used in a corner case in Minmax, so if we remove it, user-written Union procs are very likely to remain buggy for long. If you have a better idea to test Union in Minmax, or some other way to turn that stuff off for the range stuff, I'm all ears. Just lets make sure the support procs are tested to avoid stupid bugs. Before I introduced that, my Minmax Union proc was all wrong. I removed this test because I don't see a way to support it. I believe any other implementation that is more complicated than minmax will fail in there. It is better to cache them with the regression tests, so I tried to improve them. GiST, SP-GiST and GIN don't have similar checks, but they have more complicated user defined functions. Patch 7 I don't understand. Will have to look closer. Are you saying Minmax will depend on Btree opclasses? I remember thinking in doing it that way at some point, but wasn't convinced for some reason. No, there isn't any additional dependency. It makes minmax operator classes use the procedures from the pg_amop instead of adding them to pg_amproc. It also makes the operator class safer for cross data type usage. Actually, I just checked and find out that we got wrong answers from index on the current master without this patch. You can reproduce it with this query on the regression database: select * from brintest where timestampcol = '1979-01-29 11:05:09'::timestamptz; inclusion-opclasses patch make it possible to add cross type brin regression tests. I will add more of them on the next version. Patch 6 seems the real meat of your own stuff. I think there should be a patch 8 also but it's not attached ... ?? I had another commit not to intended to be sent. Sorry about that. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
Thanks for the updated patch; I will at it as soon as time allows. (Not really all that soon, regrettably.) Judging from a quick look, I think patches 1 and 5 can be committed quickly; they imply no changes to other parts of BRIN. (Not sure why 1 and 5 are separate. Any reason for this?) Also patch 2. Patch 4 looks like a simple bugfix (or maybe a generalization) of BRIN framework code; should also be committable right away. Needs a closer look of course. Patch 3 is a problem. That code is there because the union proc is only used in a corner case in Minmax, so if we remove it, user-written Union procs are very likely to remain buggy for long. If you have a better idea to test Union in Minmax, or some other way to turn that stuff off for the range stuff, I'm all ears. Just lets make sure the support procs are tested to avoid stupid bugs. Before I introduced that, my Minmax Union proc was all wrong. Patch 7 I don't understand. Will have to look closer. Are you saying Minmax will depend on Btree opclasses? I remember thinking in doing it that way at some point, but wasn't convinced for some reason. Patch 6 seems the real meat of your own stuff. I think there should be a patch 8 also but it's not attached ... ?? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
On 02/11/2015 07:34 PM, Emre Hasegeli wrote: The current code compiles but the brin test suite fails. Now, only a test in . Yeah, there is still a test which fails in opr_sanity. Yes but they were also required by this patch. This version adds more functions and operators. I can split them appropriately after your review. Ok, sounds fine to me. This problem remains. There is also a similar problem with the range types, namely empty ranges. There should be special cases for them on some of the strategies. I tried to solve the problems in several different ways, but got a segfault one line or another. This makes me think that BRIN framework doesn't support to store different types than the indexed column in the values array. For example, brin_deform_tuple() iterates over the values array and copies them using the length of the attr on the index, not the length of the type defined by OpcInfo function. If storing another types aren't supported, why is it required to return oid's on the OpcInfo function. I am confused. I leave this to someone more knowledgable about BRIN to answer. I think I have fixed them. Looks good as far as I can tell. I have fixed different addressed families by adding another support function. I used STORAGE parameter to support the point data type. To make it work I added some operators between box and point data type. We can support all geometric types with this method. Looks to me like this should work. = New comments - Searching for the empty range is slow since the empty range matches all brin ranges. EXPLAIN ANALYZE SELECT * FROM foo WHERE r = '[1,1)'; QUERY PLAN --- Bitmap Heap Scan on foo (cost=12.01..16.02 rows=1 width=14) (actual time=47.603..47.605 rows=1 loops=1) Recheck Cond: (r = 'empty'::int4range) Rows Removed by Index Recheck: 20 Heap Blocks: lossy=1082 - Bitmap Index Scan on foo_r_idx (cost=0.00..12.01 rows=1 width=0) (actual time=0.169..0.169 rows=11000 loops=1) Index Cond: (r = 'empty'::int4range) Planning time: 0.062 ms Execution time: 47.647 ms (8 rows) - Found a typo in the docs: withing the range - Why have you removed the USE_ASSERT_CHECKING code from brin.c? - Remove redundant or not from /* includes empty element or not */. - Minor grammar gripe: Change Check that to Check if in the comments in brin_inclusion_add_value(). - Wont the code incorrectly return false if the first added element to an index page is empty? - Would it be worth optimizing the code by checking for empty ranges after checking for overlap in brin_inclusion_add_value()? I would imagine that empty ranges are rare in most use cases. - Typo in comment: If the it - If it - Typo in comment: Note that this strategies - Note that these strategies - Typo in comment: inequality strategies does not - inequality strategies do not - Typo in comment: geometric types which uses - geometric types which use - I get 'ERROR: missing strategy 7 for attribute 1 of index bar_i_idx' when running the query below. Why does this not fail in the test suite? The overlap operator works just fine. If I read your code correctly other strategies are also missing. SELECT * FROM bar WHERE i = '::1'; - I do not think this comment is true Used to determine the addresses have a common union or not. It actually checks if we can create range which contains both ranges. - Compact random spaces in select numrange(1.0, 2.0) + numrange(2.5, 3.0);-- should fail -- Andreas Karlsson -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
On Thu, Feb 12, 2015 at 3:34 AM, Emre Hasegeli e...@hasegeli.com wrote: Thank you for looking at my patch again. New version is attached with a lot of changes and point data type support. Patch is moved to next CF 2015-02 as work is still going on. -- Michael
Re: [HACKERS] BRIN range operator class
Can you please break up this patch? I think I see three patches, 1. add sql-callable functions such as inet_merge, network_overright, etc etc. These need documentation and a trivial regression test somewhere. 2. necessary changes to header files (skey.h etc) 3. the inclusion opclass itself Thanks BTW the main idea behind having opcinfo return the type oid was to tell the index what was stored in the index. If that doesn't work right now, maybe it needs some tweak to the brin framework code. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
Hi, I made a quick review for your patch, but I would like to see someone who was involved in the BRIN work comment on Emre's design issues. I will try to answer them as best as I can below. I think minimax indexes on range types seems very useful, and inet/cidr too. I have no idea about geometric types. But we need to fix the issues with empty ranges and IPv4/IPv6 for these indexes to be useful. = Review The current code compiles but the brin test suite fails. I tested the indexes a bit and they seem to work fine, except for cases where we know it to be broken like IPv4/IPv6. The new code is generally clean and readable. I think some things should be broken out in separate patches since they are unrelated to this patch. - The addition of and on inet types. - The fix in brin_minmax.c. Your brin tests seems to forget and for inet types. The tests should preferably be extended to support ipv6 and empty ranges once we have fixed support for those cases. The /* If the it is all nulls, it cannot possibly be consistent. */ comment is different from the equivalent comment in brin_minmax.c. I do not see why they should be different. In brin_inclusion_union() the if (col_b-bv_allnulls) is done after handling has_nulls, which is unlike what is done in brin_minmax_union(), which code is right? I am leaning towards the code in brin_inclusion_union() since you can have all_nulls without has_nulls. On 12/14/2014 09:04 PM, Emre Hasegeli wrote: To support more operators I needed to change amstrategies and amsupport on the catalog. It would be nice if amsupport can be set to 0 like am strategies. I think it would be nicer to get the functions from the operators with using the strategy numbers instead of adding them directly as support functions. I looked around a bit but couldn't find a sensible way to support it. Is it possible without adding them to the RelationData struct? Yes that would be nice, but I do not think the current solution is terrible. This problem remains. There is also a similar problem with the range types, namely empty ranges. There should be special cases for them on some of the strategies. I tried to solve the problems in several different ways, but got a segfault one line or another. This makes me think that BRIN framework doesn't support to store different types than the indexed column in the values array. For example, brin_deform_tuple() iterates over the values array and copies them using the length of the attr on the index, not the length of the type defined by OpcInfo function. If storing another types aren't supported, why is it required to return oid's on the OpcInfo function. I am confused. I leave this to someone more knowledgable about BRIN to answer. I didn't try to support other geometric types than box as I couldn't managed to store a different type on the values array, but it would be nice to get some feedback about the overall design. I was thinking to add a STORAGE parameter to the index to support other geometric types. I am not sure that adding the STORAGE parameter to be used by the opclass implementation is the right way. It wouldn't be the actual thing that is stored by the index, it will be an element in the values array. Maybe, data type specific opclasses is the way to go, not a generic one as I am trying. I think a STORAGE parameter sounds like a good idea. Could it also be used to solve the issue with IPv4/IPv6 by setting the storage type to custom? Or is that the wrong way to fix things? -- Andreas Karlsson -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BRIN range operator class
I thought we can do better than minmax for the inet data type, and ended up with a generalized opclass supporting both inet and range types. Patch based on minmax-v20 attached. It works well except a few small problems. I will improve the patch and add into a commitfest after BRIN framework is committed. I wanted to send a new version before the commitfest to get some feedback, but it is still work in progress. Patch attached rebased to the current HEAD. This version supports more operators and box from geometric data types. Opclasses are renamed to inclusion_ops to be more generic. The problems I mentioned remain beause I couldn't solve them without touching the BRIN framework. To support more operators I needed to change amstrategies and amsupport on the catalog. It would be nice if amsupport can be set to 0 like am strategies. I think it would be nicer to get the functions from the operators with using the strategy numbers instead of adding them directly as support functions. I looked around a bit but couldn't find a sensible way to support it. Is it possible without adding them to the RelationData struct? Inet data types accept IP version 4 and version 6. It isn't possible to represent union of addresses from different versions with a valid inet type. So, I made the union function return NULL in this case. Then, I tried to store if returned value is NULL or not, in column-values[] as boolean, but it failed on the pfree() inside brin_dtuple_initilize(). It doesn't seem right to free the values based on attr-attbyval. This problem remains. There is also a similar problem with the range types, namely empty ranges. There should be special cases for them on some of the strategies. I tried to solve the problems in several different ways, but got a segfault one line or another. This makes me think that BRIN framework doesn't support to store different types than the indexed column in the values array. For example, brin_deform_tuple() iterates over the values array and copies them using the length of the attr on the index, not the length of the type defined by OpcInfo function. If storing another types aren't supported, why is it required to return oid's on the OpcInfo function. I am confused. I didn't try to support other geometric types than box as I couldn't managed to store a different type on the values array, but it would be nice to get some feedback about the overall design. I was thinking to add a STORAGE parameter to the index to support other geometric types. I am not sure that adding the STORAGE parameter to be used by the opclass implementation is the right way. It wouldn't be the actual thing that is stored by the index, it will be an element in the values array. Maybe, data type specific opclasses is the way to go, not a generic one as I am trying. brin-inclusion-v02.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
