Re: Table AM Interface Enhancements

On Mon, Apr 8, 2024 at 12:33 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> Yes, it was my mistake. I got rushing trying to fit this to FF, even doing significant changes just before commit.
> I'll revert this later today.

Alexander,

Exactly how much is getting reverted here? I see these, all since March 23rd:

dd1f6b0c17 Provide a way block-level table AMs could re-use
acquire_sample_rows()
9bd99f4c26 Custom reloptions for table AM
97ce821e3e Fix the parameters order for
TableAmRoutine.relation_copy_for_cluster()
867cc7b6dd Revert "Custom reloptions for table AM"
b1484a3f19 Let table AM insertion methods control index insertion
c95c25f9af Custom reloptions for table AM
27bc1772fc Generalize relation analyze in table AM interface
87985cc925 Allow locking updated tuples in tuple_update() and tuple_delete()
c35a3fb5e0 Allow table AM tuple_insert() method to return the different slot
02eb07ea89 Allow table AM to store complex data structures in rd_amcache

I'm not really feeling very good about all of this, because:

- 87985cc925 was previously committed as 11470f544e on March 23, 2023,
and almost immediately reverted. Now you tried again on March 26,
2024. I know there was a bunch of rework in the middle, but there are
times in the year that things can be committed other than right before
the feature freeze. Like, don't wait a whole year for the next attempt
and then again do it right before the cutoff.

- The Discussion links in the commit messages do not seem to stand for
the proposition that these particular patches ought to be committed in
this form. Some of them are just links to the messages where the patch
was originally posted, which is probably not against policy or
anything, but it'd be nicer to see links to versions of the patch with
which people are, in nearby emails, agreeing. Even worse, some of
these are links to emails where somebody said, "hey, some earlier
commit does not look good." In particular,
dd1f6b0c172a643a73d6b71259fa2d10378b39eb has a discussion link where
Andres complains about 27bc1772fc814946918a5ac8ccb9b5c5ad0380aa, but
it's not clear how that justifies the new commit.

- The commit message for 867cc7b6dd says "This reverts commit
c95c25f9af4bc77f2f66a587735c50da08c12b37 due to multiple design issues
spotted after commit." That's not a very good justification for then
trying again 6 days later with 9bd99f4c26, and it's *definitely* not a
good justification for there being no meaningful discussion links in
the commit message for 9bd99f4c26. They're just the same links you had
in the previous attempt, so it's pretty hard for anybody to understand
what got fixed and whether all of the concerns were really addressed.
Just looking over the commit, it's pretty hard to understand what is
being changed and why: there's not a lot of comment updates, there's
no documentation changes, and there's not a lot of explanation in the
commit message either. Even if this feature is great and all the code
is perfect now, it's going to be hard for anyone to figure out how to
use it.

97ce821e3e looks like a clear bug fix to me, but I wonder if the rest
of this should all just be reverted, with a ban on ever trying it
again after March 1 of any year. I'd like to believe that there are
only bookkeeping problems here, and that there was in fact clear
agreement that all of these changes should be made in this form, and
that the commit messages simply failed to reference the most relevant
emails. But what I fear, especially in view of Andres's remarks, is
that these commits were done in haste without adequate consensus, and
I think that's a serious problem.

--
Robert Haas
EDB: http://www.enterprisedb.com