On Sat, Oct 15, 2022 at 1:47 AM Amit Langote <amitlangote09@gmail.com> wrote:
> I have merged your incremental patch into 0003.
Note that if someone goes to commit 0003, they would have no idea that
I contributed to the effort. You should probably try to keep a running
list of co-authors, reviewers, or other people that need to be
acknowledged in your draft commit messages. On that note, I think that
the commit messages for 0001 and to some extent 0002 need some more
work. In particular, it seems like the commit message for 0001 is
entirely concerned with what the patch does and says nothing about why
it's a good idea. In my opinion, a good commit message needs to do
both, ideally but not always in less space than this patch takes to do
only one of those things. 0002 has the same problem to a lesser
degree, since it is perhaps not so hard to infer that the reason for
avoiding the SQL query is performance.
I am wondering if the ordering for this patch series needs to be
rethought. The commit message for 0004 reads as if it is fixing a bug
introduced by earlier patches in the series. If that is not correct,
maybe it can be made clearer. If it is correct, then that's not good,
because we don't want to commit buggy patches and then make follow-up
commits to remove the bugs. If a planned commit needs new
infrastructure to avoid being buggy, the commits adding that
infrastructure should happen first.
But I think the bigger problem for this patch set is that the
design-level feedback from
https://www.postgresql.org/message-id/CA%2BTgmoaiTNj4DgQy42OT9JmTTP1NWcMV%2Bke0i%3D%2Ba7%3DVgnzqGXw%40mail.gmail.com
hasn't really been addressed, AFAICS. ri_LookupKeyInPkRelPlanIsValid
is still trivial in v7, and that still seems wrong to me. And I still
don't know how we're going to avoid changing the semantics in ways
that are undesirable, or even knowing precisely what we did change. If
we don't have answers to those questions, then I suspect that this
patch set isn't going anywhere.
--
Robert Haas
EDB: http://www.enterprisedb.com
