Re: [PATCH] Covering SPGiST index

Pavel Borisov writes: > In a v14 I forgot to add the test. PFA v15 I've committed this with a lot of mostly-cosmetic changes. The not-so-cosmetic bits had to do with confusion between the input data type and the leaf type, which isn't really your fault because it was there before :-(. One note i

Re: [PATCH] Covering SPGiST index

In a v14 I forgot to add the test. PFA v15 -- Best regards, Pavel Borisov Postgres Professional: http://postgrespro.com v15-0001-Covering-SP-GiST-index-support-for-INCLUDE-colum.patch Description: Binary data

Re: [PATCH] Covering SPGiST index

> > On further contemplation, it occurs to me that if we make the switch > to "key values are stored per normal rules", then even if there is an > index with pass-by-value keys out there someplace, it would only break > on big-endian architectures. On little-endian, the extra space > occupied by t

Re: [PATCH] Covering SPGiST index

I wrote: > I spent a little time looking at this, and realized something that may > or may not be a serious problem. This form of the patch supposes that > it can use the usual tuple form/deform logic for all columns of a leaf > tuple including the key column. However, that does not square with >

Re: [PATCH] Covering SPGiST index

David Steele writes: > Tom, do the changes as enumerated in [1] look like they are going in the > right direction? I spent a little time looking at this, and realized something that may or may not be a serious problem. This form of the patch supposes that it can use the usual tuple form/deform

Re: [PATCH] Covering SPGiST index

On 12/4/20 12:31 PM, Pavel Borisov wrote: The cfbot's still unhappy --- looks like you omitted a file from the patch? You are right, thank you. Corrected this. PFA v13 Tom, do the changes as enumerated in [1] look like they are going in the right direction? Regards, -- -David da...@

Re: [PATCH] Covering SPGiST index

> > The cfbot's still unhappy --- looks like you omitted a file from the > patch? > You are right, thank you. Corrected this. PFA v13 -- Best regards, Pavel Borisov Postgres Professional: http://postgrespro.com v13-0001-Covering-SP-GiST-index-support-for-INCLUDE-co

Re: [PATCH] Covering SPGiST index

Pavel Borisov writes: > I've noticed CI error due to the fact that MSVC doesn't allow arrays of > flexible size arrays and made a fix for the issue. > Also did some minor refinement in tuple creation. > PFA v12 of a patch. The cfbot's still unhappy --- looks like you omitted a file from the patch

Re: [PATCH] Covering SPGiST index

I've noticed CI error due to the fact that MSVC doesn't allow arrays of flexible size arrays and made a fix for the issue. Also did some minor refinement in tuple creation. PFA v12 of a patch. чт, 26 нояб. 2020 г. в 21:48, Pavel Borisov : > > The way that seems acceptable to me is to add (optiona

Re: [PATCH] Covering SPGiST index

> > > The way that seems acceptable to me is to add (optional) nulls mask into > > the end of existing style SpGistLeafTuple header and use indextuple > > routines to attach attributes after it. In this case, we can reduce the > > amount of code at the cost of adding one extra MAXALIGN size to the

Re: [PATCH] Covering SPGiST index

Pavel Borisov writes: > The way that seems acceptable to me is to add (optional) nulls mask into > the end of existing style SpGistLeafTuple header and use indextuple > routines to attach attributes after it. In this case, we can reduce the > amount of code at the cost of adding one extra MAXALIGN

Re: [PATCH] Covering SPGiST index

вт, 17 нояб. 2020 г. в 11:36, Pavel Borisov : > I've started to review this, and I've got to say that I really disagree >> with this choice: >> >> + * If there are INCLUDE columns, they are stored after a key value, each >> + * starting from its own typalign boundary. Unlike IndexTuple, first >> I

Re: [PATCH] Covering SPGiST index

Pavel Borisov writes: > [ v10-0001-Covering-SP-GiST-index-support-for-INCLUDE-colum.patch ] I've started to review this, and I've got to say that I really disagree with this choice: + * If there are INCLUDE columns, they are stored after a key value, each + * starting from its own typalign bound

Re: [PATCH] Covering SPGiST index

> > I have a wild idea of renaming nextOffset in SpGistLeafTupleData. > Or at least documenting in comments that this field is more than just an > offset. > Seems reasonable as previous changes localized mentions of nextOffset only to leaf tuple definition and access macros. So I've done this renam

Re: [PATCH] Covering SPGiST index

> 31 авг. 2020 г., в 16:57, Pavel Borisov написал(а): > > I agree with all of your proposals and integrated them into v9. I have a wild idea of renaming nextOffset in SpGistLeafTupleData. Or at least documenting in comments that this field is more than just an offset. This looks like assert

Re: [PATCH] Covering SPGiST index

> > But let's change macro a bit. When I see > SGLT_SET_OFFSET(leafTuple->nextOffset, InvalidOffsetNumber); > I expect that leafTuple->nextOffset is function argument by value and will > not be changed. > For example see ItemPointerSetOffsetNumber() - it's not exposing ip_posid. > > Also, I'd propo

Re: [PATCH] Covering SPGiST index

> 27 авг. 2020 г., в 21:03, Pavel Borisov написал(а): > > see v8 For me is the only concerning point is putting nullmask and varatt bits into tuple->nextOffset. But, probably, we can go with this. But let's change macro a bit. When I see SGLT_SET_OFFSET(leafTuple->nextOffset, InvalidOffsetN

Re: [PATCH] Covering SPGiST index

> > 3) I didn't quite get the meaning of the assertion, that is added in a few > places: > Assert(so->state.includeTupdesc->natts); > Should it be Assert(so->state.includeTupdesc->natts > 1) ? > It is rather Assert(so->state.includeTupdesc->natts > 0) as INCLUDE tuple descriptor should not be

Re: [PATCH] Covering SPGiST index

On 24.08.2020 16:19, Pavel Borisov wrote: I added some extra comments and mentions in manual to make all the things clear (see v7 patch) The patch implements the proposed functionality, passes tests, and in general looks good to me. I don't mind using a macro to differentiate tuples with and

Re: [PATCH] Covering SPGiST index

> > I'm looking into the patch. I have few notes: > > 1. I see that in src/backend/access/spgist/README you describe SP-GiST > tuple as sequence of {Value, ItemPtr to heap, Included attributes}. Is it > different from regular IndexTuple where tid is within TupleHeader? > Yes, the header of SpGist

Re: [PATCH] Covering SPGiST index

Hi! > 17 авг. 2020 г., в 21:04, Pavel Borisov написал(а): > > Postgres Professional: http://postgrespro.com > I'm looking into the patch. I have few notes: 1. I see that in src/backend/access/spgist/README you describe SP-GiST tuple as sequence of {Value, ItemPtr to heap, Included attributes

Re: [PATCH] Covering SPGiST index

With a little bugfix вт, 11 авг. 2020 г. в 22:50, Pavel Borisov : > > > вт, 11 авг. 2020 г. в 12:11, Pavel Borisov : > >> I added changes in documentation into the patch. >> >> >> -- >> Best regards, >> Pavel Borisov >> >> Postgres Professional: http://postgrespro.com >>

Re: [PATCH] Covering SPGiST index

вт, 11 авг. 2020 г. в 12:11, Pavel Borisov : > I added changes in documentation into the patch. > > > -- > Best regards, > Pavel Borisov > > Postgres Professional: http://postgrespro.com > v5-0001-Covering-SP-GiST-index-support-for-INCLUDE-column.patch Description: B

Re: [PATCH] Covering SPGiST index

I added changes in documentation into the patch. -- Best regards, Pavel Borisov Postgres Professional: http://postgrespro.com v4-0001-Covering-SP-GiST-index-support-for-INCLUDE-column.patch Description: Binary data

Re: [PATCH] Covering SPGiST index

Same code formatted as a patch. пн, 10 авг. 2020 г. в 17:45, Pavel Borisov : > Also little bit corrected code formatting. > >> Best regards, >> Pavel Borisov >> >> Postgres Professional: http://postgrespro.com >> >> > v3-0001-Covering-SpGist.patch Description: Binar

Re: [PATCH] Covering SPGiST index

Also little bit corrected code formatting. > Best regards, > Pavel Borisov > > Postgres Professional: http://postgrespro.com > spgist-covering-0003.diff Description: Binary data

Re: [PATCH] Covering SPGiST index

> > On a first glance the whole concept of non-multicolumn index with included > attributes seems...well, just difficult to understand. > But I expect for SP-GiST this must be single key with multiple included > attributes, right? > I couldn't find a test that checks impossibility of on 2-column SP

Re: [PATCH] Covering SPGiST index

> 7 авг. 2020 г., в 16:59, Pavel Borisov написал(а): > > As usual I very much appreciate your feedback Thanks for the patch! Looks interesting. On a first glance the whole concept of non-multicolumn index with included attributes seems...well, just difficult to understand. But I expect for

[PATCH] Covering SPGiST index

Hi, hackers! I'd like to propose a patch which introduces a functionality to include additional columns to SPGiST index to increase speed of queries containing them due to making the scans index only in this case. To date this functionality was available in GiSt and btree, I suppose the same is us