Re: WIP: Covering + unique indexes.

On Fri, Apr 6, 2018 at 5:00 AM, Peter Geoghegan <pg@bowt.ie> wrote:

> On Thu, Apr 5, 2018 at 7:59 AM, Alexander Korotkov
> <a.korotkov@postgrespro.ru> wrote:
> >> * btree_xlog_split() still has this code:
>
> > Right, I think there is absolutely no need in this code.  It's removed in
> > the attached patchset.
>
> I'm now a bit nervous about always logging the high key, since that
> could impact performance. I think that there is a good way to only do
> it when needed. New plan:
>
> 1. Add these new fields to split record's set of xl_info fields (it
> should be placed directly after XLOG_BTREE_SPLIT_R):
>
> #define XLOG_BTREE_SPLIT_L_HIGHKEY 0x50 /* as above, include truncated
> highkey */
> #define XLOG_BTREE_SPLIT_R_HIGHKEY 0x60 /* as above, include truncated
> highkey */
>
> 2. Within _bt_split(), restore the old "leaf vs. internal" logic, so
> that the high key is only logged for internal (!isleaf) pages.
> However, only log it when needed for leaf pages -- only when the new
> highkey was *actually* truncated (or when its an internal page), since
> only then will it actually be different to the first item on the right
> page. Also, set XLOG_BTREE_SPLIT_L_HIGHKEY instead of
> XLOG_BTREE_SPLIT_L when we must log (or set XLOG_BTREE_SPLIT_R_HIGHKEY
> instead of XLOG_BTREE_SPLIT_R), so that recovery actually knows that
> it should restore the truncated highkey.
>
> (Sometimes I think it would be nice to be able to do more during
> recovery, but that's a much bigger issue.)
>
> 3. Restore all the master code within btree_xlog_split(), except
> instead of restoring the high key when !isleaf, do so when the record
> is XLOG_BTREE_SPLIT_L_HIGHKEY|XLOG_BTREE_SPLIT_R_HIGHKEY.
>
> 4. Add an assertion within btree_xlog_split(), that ensures that
> internal pages never fail to have their high key logged, since there
> is no reason why that should ever not happen with internal pages.
>
> 5. Fix this struct xl_btree_split comment, which commit 0c504a80 from
> 2017 missed when it reclaimed two xl_info status bits:
>
>  * Note: the four XLOG_BTREE_SPLIT xl_info codes all use this data record.
>  * The _L and _R variants indicate whether the inserted tuple went into the
>  * left or right split page (and thus, whether newitemoff and the new item
>  * are stored or not).  The _ROOT variants indicate that we are splitting
>  * the root page, and thus that a newroot record rather than an insert or
>  * split record should follow.  Note that a split record never carries a
>  * metapage update --- we'll do that in the parent-level update.
>
> 6. Add your own xl_btree_split comment in its place, noting the new
> usage. Basically, the _ROOT sentence with a similar _HIGHKEY sentence.
>
> 7. Don't forget about btree_desc().
>
> I'd say that there is a good change that Anastasia is correct to think
> that it isn't worth worrying about the extra WAL that her approach
> implied, and that it is in fact good enough to simply always log the
> left page's high key. However, it seems easier and lower risk all
> around to do it this way. It doesn't leave us with ambiguity. In my
> experience, *ambiguity* on design questions makes a patch miss a
> release much more frequently than bugs or regressions make that
> happen.
>
> Sorry that I didn't just say this the first time I brought up
> btree_xlog_split(). I didn't see the opportunity to avoid creating
> more WAL until now.
>

Done.  I would note that this aspect also catched my eye, but
since I didn't read very carefully the whole thread I thought that
it was already decided that small extra WAL is harmless.

> If you're talking about these assertions
> >
> > Assert(IndexRelationGetNumberOfAttributes(rel) != 0);
> > indnkeyatts = IndexRelationGetNumberOfKeyAttributes(rel);
> > Assert(indnkeyatts != 0);
>
> Actually, I was just talking about the first one,
> "Assert(IndexRelationGetNumberOfAttributes(rel) != 0)". I was unclear.
> Maybe it isn't worth getting rid of even the first one.
>

OK, "Assert(IndexRelationGetNumberOfAttributes(rel) != 0)" has been
removed.


> * Use <quote><quote/> here:
>
> > +       <para>
> > +        Indexes with columns listed in the <literal>INCLUDE</literal>
> clause
> > +        are also called "covering indexes".
> > +       </para>
>
> * Use <literal><literal/> here:
>
> > +       <para>
> > +        In <literal>UNIQUE</literal> indexes, uniqueness is only
> enforced
> > +        for key columns.  Columns listed in the
> <literal>INCLUDE</literal>
> > +        clause have no effect on uniqueness enforcement.  Other
> constraints
> > +        (PRIMARY KEY and EXCLUDE) work the same way.
> > +       </para>
>

Fixed, thanks.

* Do the regression tests pass with COPY_PARSE_PLAN_TREES?
>

I've checked.  "make check-world" do pass with COPY_PARSE_PLAN_TREES.

* Running pgindent would be nice. I see a bit of trailing whitespace,
> and things like that.
>

I've run pgindent and added relevant changes into patch.


> * Please tweak the indentation here (perhaps a new line):
>
> > @@ -927,6 +963,7 @@ _bt_buildadd(BTWriteState *wstate, BTPageState
> *state, IndexTuple itup)
> >         last_off = P_FIRSTKEY;
> >     }
> >
> > +   pageop = (BTPageOpaque) PageGetSpecialPointer(npage);
> >     /*
>

Done.


> * Does the optimizer README's PathKeys section need a sentence or two
> on this patch?
>

It definitely needs one.  I've added the sentence describing that non-key
attributes don't have corresponding PathKeys.

I'm nervous about problems within the optimizer in general, since that
> is an area that I am not particularly qualified to review. I hope that
> someone with more experience in that area can take a look at it
> specifically. I see that there are very few changes in the optimizer,
> but in my experience that's often the problem when it comes to the
> optimizer -- it lacks subtle things that it actually needs, rather
> than having the wrong things.
>

I would just note, that Postgres Pro has shipped a version of covering
indexes to customers.  The optimizer part there is pretty same as now
in the patchset.  Assuming that we didn't met optimizer issues in this
part, I expect them to be at least very rare.  I also would like to ask my
Postgres Pro colleague Alexander Kuzmenkov to take a look at optimizer
part of this patch.  If even after that we will miss something in the
optimizer then I expect it could be fixed after feature freeze.

* Does this existing build_index_pathkeys() comment need to be updated?
>
>  * The result is canonical, meaning that redundant pathkeys are removed;
>  * it may therefore have fewer entries than there are index columns.
>  *
>  * Another reason for stopping early is that we may be able to tell that
>  * an index column's sort order is uninteresting for this query.  However,
>  * that test is just based on the existence of an EquivalenceClass and not
>  * on position in pathkey lists, so it's not complete.  Caller should call
>  * truncate_useless_pathkeys() to possibly remove more pathkeys.
>

Yes, I've updated it.


> * I don't think that there is much point in having separate 0003 +
> 0004 patches. For the next revision, please squash those down into
> 0002. Actually, maybe there should be only one patch for the next
> revision. Up to you.
>

Agreed.  I've merge all the patches into one.


> * Please write commit messages for your patches. I like to make these
> part of the review process.
>

Commit message is included into the patch.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company