Re: [HACKERS] WIP: Covering + unique indexes.
> 21 янв. 2018 г., в 3:36, Peter Geoghegan написал(а): > > On Wed, Jan 17, 2018 at 12:45 AM, Andrey Borodin wrote: >> Unfortunately, amcheck_next does not work currently on HEAD (there are >> problems with AllocSetContextCreate() signature), but I've tested >> bt_index_check() before, during and after pgbench, on primary and on slave. >> Also, I've checked bt_index_parent_check() on master. > > I fixed that recently. It should be fine now. Oh, sorry, missed that I'm using patched stale amcheck_next. Thanks! Affirmative, amcheck_next works fine. I run pgbench against several covering indexes. Checking before load, during and after, both on master and slave. I do not observe any errors besides infrequent "canceling statement due to conflict with recovery", which is not a sign of any malfunction. Best regards, Andrey Borodin.
Re: [HACKERS] WIP: Covering + unique indexes.
On Wed, Jan 17, 2018 at 12:45 AM, Andrey Borodin wrote: > Unfortunately, amcheck_next does not work currently on HEAD (there are > problems with AllocSetContextCreate() signature), but I've tested > bt_index_check() before, during and after pgbench, on primary and on slave. > Also, I've checked bt_index_parent_check() on master. I fixed that recently. It should be fine now. -- Peter Geoghegan
Re: [HACKERS] WIP: Covering + unique indexes.
Hi! > 18 янв. 2018 г., в 18:57, Anastasia Lubennikova > написал(а): > > What is amcheck_next ? amcheck_next is external version of amcheck, maintained by Peter G. on his github. It checks one more thing: that every heap tuple has twin in B-tree, so called heapallindexed check. Version V3 of your patch was checked with heapallindexed and passed the test, both on master and on slave. >> During bt_index_check() test from time to time I was observing >> ERROR: canceling statement due to conflict with recovery >> DETAIL: User query might have needed to see row versions that must be >> removed. >> > > Sorry, I forgot to attach the amcheck fix to the previous message. No problem, surely I've fixed that before testing. > Now all the patches are in attachment. > Could you recheck if the error is still there? No need to do that, I was checking exactly same codebase. And that error has nothing to do with your patch, amcheck does not always can perform bt_index_parent_check() on slave when master is heavy loaded. It's OK. I reported this error just to be 100% precise about observed things. Thanks for working on this feature, hope to see it in 11. Best regards, Andrey Borodin.
Re: [HACKERS] WIP: Covering + unique indexes.
Hi! > 16 янв. 2018 г., в 21:50, Anastasia Lubennikova > написал(а): > > Updated patches are attached. > Cool, thanks! I've looked into the code, but haven't found anything broken. Since I've tried to rebase patch myself and failed on parse utils, I've spend some cycles trying to break parsing. One minor complain (no need to fix). This is fine x4mmm=# create index on pgbench_accounts (bid) include (aid,filler,upper(filler)); ERROR: expressions are not supported in included columns But why not same error here? Previous message is very descriptive. x4mmm=# create index on pgbench_accounts (bid) include (aid,filler,aid+1); ERROR: syntax error at or near "+" This works. But should not, IMHO x4mmm=# create index on pgbench_accounts (bid) include (aid,aid,aid); CREATE INDEX Do not know what's that... # create index on pgbench_accounts (bid) include (aid desc, aid asc); CREATE INDEX All these things allow foot-shooting with a small caliber, but do not break big things. Unfortunately, amcheck_next does not work currently on HEAD (there are problems with AllocSetContextCreate() signature), but I've tested bt_index_check() before, during and after pgbench, on primary and on slave. Also, I've checked bt_index_parent_check() on master. During bt_index_check() test from time to time I was observing ERROR: canceling statement due to conflict with recovery DETAIL: User query might have needed to see row versions that must be removed. [install]check[-world] passed :) From my POV, patch is in a good shape. I think it is time to make the patch ready for committer again. Best regards, Andrey Borodin.
Re: [HACKERS] WIP: Covering + unique indexes.
Hello! The patch does not apply currently. Anastasia, can you, please, rebase the patch? Best regards, Andrey Borodin.
Re: [HACKERS] WIP: Covering + unique indexes.
> 30 нояб. 2017 г., в 23:07, Andrey Borodin написал(а): > > Seems like it was not a big deal of patching, I've fixed those bits (see > attachment). > I've done only simple tests as for now, but I'm planning to do better testing > before next CF. > Thanks for mentioning "heapallindexed", I'll use it too. I've tested the patch with fixed amcheck (including "heapallindexed" feature), tests included bulk index creation, pgbenching and amcheck of index itself and WAL-replicated index. Everything worked fine. Spotted one more typo: > Since 10.0 there is an optional INCLUDE clause should be > Since 11.0 there is an optional INCLUDE clause I think that patch set (two patches + 1 amcheck diff) is ready for committer. Best regards, Andrey Borodin.
Re: [HACKERS] WIP: Covering + unique indexes.
Hi, Peter! > 29 нояб. 2017 г., в 8:45, Peter Geoghegan написал(а): > > It looks like amcheck needs to be patched -- a simple oversight. > amcheck is probably calling _bt_compare() without realizing that > internal pages don't have the extra attributes (just leaf pages, > although they should also not participate in comparisons in respect of > included/extra columns). There were changes to amcheck at one point in > the past. That must have slipped through again. I don't think it's > that complicated. > > BTW, it would probably be a good idea to use the new Github version's > "heapallindexed" verification [1] for testing this patch. Anastasia > will need to patch the externally maintained amcheck to do this, but > it's probably no extra work, because this is already needed for > contrib/amcheck, and because the heapallindexed check doesn't actually > care about index structure at all. Seems like it was not a big deal of patching, I've fixed those bits (see attachment). I've done only simple tests as for now, but I'm planning to do better testing before next CF. Thanks for mentioning "heapallindexed", I'll use it too. Best regards, Andrey Borodin. amcheck_include.diff Description: Binary data
Re: [HACKERS] WIP: Covering + unique indexes.
> 29 нояб. 2017 г., в 8:45, Peter Geoghegan написал(а):
>
> On Tue, Nov 28, 2017 at 6:16 PM, Michael Paquier
> wrote:
>> On Sun, Nov 12, 2017 at 8:40 PM, Andrey Borodin wrote:
>>> Postgres crashes:
>>> TRAP: FailedAssertion("!(((const void*)(&isNull) != ((void*)0)) &&
>>> (scankey->sk_attno) > 0)", File: "nbtsearch.c", Line: 466)
>>>
>>> May be I'm doing something wrong or amcheck support will go with different
>>> patch?
>>
>> Usually amcheck complaining is a sign of other symptoms. I am marking
>> this patch as returned with feedback for now as no updates have been
>> provided after two weeks.
>
> It looks like amcheck needs to be patched -- a simple oversight.
> amcheck is probably calling _bt_compare() without realizing that
> internal pages don't have the extra attributes (just leaf pages,
> although they should also not participate in comparisons in respect of
> included/extra columns). There were changes to amcheck at one point in
> the past. That must have slipped through again. I don't think it's
> that complicated.
>
There is no doubts that this will be fixed. Therefor I propose move to next CF
with Waiting for Author status.
Best regards, Andrey Borodin.
Re: [HACKERS] WIP: Covering + unique indexes.
On Tue, Nov 28, 2017 at 6:16 PM, Michael Paquier
wrote:
> On Sun, Nov 12, 2017 at 8:40 PM, Andrey Borodin wrote:
>> Postgres crashes:
>> TRAP: FailedAssertion("!(((const void*)(&isNull) != ((void*)0)) &&
>> (scankey->sk_attno) > 0)", File: "nbtsearch.c", Line: 466)
>>
>> May be I'm doing something wrong or amcheck support will go with different
>> patch?
>
> Usually amcheck complaining is a sign of other symptoms. I am marking
> this patch as returned with feedback for now as no updates have been
> provided after two weeks.
It looks like amcheck needs to be patched -- a simple oversight.
amcheck is probably calling _bt_compare() without realizing that
internal pages don't have the extra attributes (just leaf pages,
although they should also not participate in comparisons in respect of
included/extra columns). There were changes to amcheck at one point in
the past. That must have slipped through again. I don't think it's
that complicated.
BTW, it would probably be a good idea to use the new Github version's
"heapallindexed" verification [1] for testing this patch. Anastasia
will need to patch the externally maintained amcheck to do this, but
it's probably no extra work, because this is already needed for
contrib/amcheck, and because the heapallindexed check doesn't actually
care about index structure at all.
[1]
https://github.com/petergeoghegan/amcheck#optional-heapallindexed-verification
--
Peter Geoghegan
Re: [HACKERS] WIP: Covering + unique indexes.
On Sun, Nov 12, 2017 at 8:40 PM, Andrey Borodin wrote:
> Postgres crashes:
> TRAP: FailedAssertion("!(((const void*)(&isNull) != ((void*)0)) &&
> (scankey->sk_attno) > 0)", File: "nbtsearch.c", Line: 466)
>
> May be I'm doing something wrong or amcheck support will go with different
> patch?
Usually amcheck complaining is a sign of other symptoms. I am marking
this patch as returned with feedback for now as no updates have been
provided after two weeks.
--
Michael
