Re: remove_useless_groupby_columns does not need to record constraint dependencies

On Thu, 16 Apr 2020 at 03:24, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> David Rowley <dgrowleyml(at)gmail(dot)com> writes:
> > Over in [1], Tom and I had a discussion in response to some confusion
> > about why remove_useless_groupby_columns() goes to the trouble of
> > recording a dependency on the PRIMARY KEY constraint when removing
> > surplus columns from the GROUP BY clause.
>
> > The outcome was that we don't need to do this since
> > remove_useless_groupby_columns() is used only as a plan-time
> > optimisation, we don't need to record any dependency.
>
> Right. I think it would be good for the comments to emphasize that
> a relcache inval will be forced if the *index* underlying the pkey
> constraint is dropped; the code doesn't care so much about the constraint
> as such. (This is also why it'd be safe to use a plain unique index
> for the same optimization, assuming you can independently verify
> non-nullness of the columns.

I've reworded the comment in the attached version.

> Maybe we should trash the existing coding
> and just have it look for unique indexes + attnotnull flags.)

I'd like to, but the timing seems off. Perhaps after we branch for PG14.

> > To prevent future confusion, I'd like to remove dependency recording
> > code from remove_useless_groupby_columns() and update the misleading
> > comment. Likely this should also be backpatched to 9.6.
>
> +1 for removing the dependency and improving the comments in HEAD.
> Minus quite a lot for back-patching: this is not a bug fix, and
> there's a nonzero risk that we've overlooked something. I'd rather
> find that out in beta testing than from bug reports against stable
> branches.

That seems fair.

David