|From:||Teodor Sigaev <teodor(at)sigaev(dot)ru>|
|To:||David Rowley <david(dot)rowley(at)2ndquadrant(dot)com>|
|Cc:||Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: POC, WIP: OR-clause support for indexes|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
Thank you for review!
> I'd like to see comments too! but more so in the code. :) I've had a look over
> this, and it seems like a great area in which we could improve on, and your
> reported performance improvements are certainly very interesting too. However
> I'm finding the code rather hard to follow, which might be a combination of my
> lack of familiarity with the index code, but more likely it's the lack of
I've added comments, fixed a found bugs.
> comments to explain what's going on. Let's just take 1 function as an example:
> Here there's not a single comment, so I'm just going to try to work out what's
> going on based on the code.
> +static void
> +compileScanKeys(IndexScanDesc scan)
> +GISTScanOpaqueso = (GISTScanOpaque) scan->opaque;
> +stackPos = -1,
> +if (scan->numberOfKeys <= 1 || so->useExec == false)
> +Assert(scan->numberOfKeys >=3);
> Why can numberOfKeys never be 2? I looked at what calls this and I can't really
Because here they are actually an expression, expression could contain 1 or tree
or more nodes but could not two (operation AND/OR plus two arguments)
> work it out. I'm really also not sure what useExec means as there's no comment
fixed. If useExec == false then SkanKeys are implicitly ANDed and stored in just
> in that struct member, and what if numberOfKeys == 1 and useExec == false, won't
> this Assert() fail? If that's not a possible situation then why not?
> +ScanKey key = scan->keyData + i;
> Is there a reason not to use keyData[i]; ?
That's the same ScanKey key = &scan->keyData[i];
I prefer first form as more clear but I could be wrong - but there are other
places in code where pointer arithmetic is used.
> +if (stackPos >= 0 && (key->sk_flags & (SK_OR | SK_AND)))
> +Assert(stackPos >= 1 && stackPos < scan->numberOfKeys);
> stackPos >= 1? This seems unnecessary and confusing as the if test surely makes
> that impossible.
> +so->leftArgs[i] = stack[stackPos - 1];
> Something is broken here as stackPos can be 0 (going by the if() not the
> Assert()), therefore that's stack[-1].
> stackPos is initialised to -1, so this appears to always skip the first element
> of the keyData array. If that's really the intention, then wouldn't it be better
> to just make the initial condition of the for() look i = 1 ?
> I'd like to review more, but it feels like a job that's more difficult than it
> needs to be due to lack of comments.
> Would it be possible to update the patch to try and explain things a little better?
Hope, I made cleaner..
Teodor Sigaev E-mail: teodor(at)sigaev(dot)ru
|Next Message||Julien Rouhaud||2016-02-29 18:04:58||Re: Publish autovacuum informations|
|Previous Message||Fabrízio de Royes Mello||2016-02-29 18:01:17||Re: Reduce lock levels others reloptions in ALTER TABLE|