Re: [PATCH] Keeps tracking the uniqueness with UniqueKey

On Wed, Oct 7, 2020 at 9:55 PM Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:

> > On Wed, Sep 09, 2020 at 07:51:12AM +0800, Andy Fan wrote:
> >
> > Thank you Michael for checking it, I can reproduce the same locally after
> > rebasing to the latest master. The attached v11 has fixed it and includes
> > the fix Floris found.
> >
> > The status of this patch is we are still in discussion about which data
> > type should
> > UniqueKey->expr use. Both David [1] and I [2] shared some thinking about
> > EquivalenceClasses, but neither of us have decided on it. So I still
> didn't
> > change
> > anything about that now. I can change it once we have decided on it.
> >
> > [1]
> >
> https://www.postgresql.org/message-id/CAApHDvoDMyw%3DhTuW-258yqNK4bhW6CpguJU_GZBh4x%2Brnoem3w%40mail.gmail.com
> >
> > [2]
> >
> https://www.postgresql.org/message-id/CAKU4AWqy3Uv67%3DPR8RXG6LVoO-cMEwfW_LMwTxHdGrnu%2Bcf%2BdA%40mail.gmail.com
>
> Hi,
>
> In the Index Skip Scan thread Peter mentioned couple of issues that I
> believe need to be addressed here. In fact one about valgrind errors was
> already fixed as far as I see (nkeycolumns instead of ncolumns), another
> one was:
>
>
> /code/postgresql/patch/build/../source/src/backend/optimizer/path/uniquekeys.c:
> In function ‘populate_baserel_uniquekeys’:
>
> /code/postgresql/patch/build/../source/src/backend/optimizer/path/uniquekeys.c:797:13:
> warning: ‘expr’ may be used uninitialized in this function
> [-Wmaybe-uninitialized]
> 797 | else if (!list_member(unique_index->rel->reltarget->exprs, expr))
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>
I can fix this warning in the next version, thanks for reporting it. It
can be
fixed like below or just adjust the if-elseif-else pattern.

--- a/src/backend/optimizer/path/uniquekeys.c
+++ b/src/backend/optimizer/path/uniquekeys.c
@@ -760,6 +760,7 @@ get_exprs_from_uniqueindex(IndexOptInfo *unique_index,
{
/* Index on system column is not supported */
Assert(false);
+ expr = NULL; /* make compiler happy */
}

> Other than that I wanted to ask what are the plans to proceed with this
> patch? It's been a while since the question was raised in which format
> to keep unique key expressions, and as far as I can see no detailed
> suggestions or patch changes were proposed as a follow up. Obviously I
> would love to see the first two preparation patches committed to avoid
> dependencies between patches, and want to suggest an incremental
> approach with simple format for start (what we have right now) with the
> idea how to extend it in the future to cover more cases.
>

I think the hardest part of this series is commit 2, it probably needs
lots of
dedicated time to review which would be the hardest part for the reviewers.
I don't have a good suggestion, however.

--
Best Regards
Andy Fan