On Mon, Apr 8, 2024 at 12:33 PM Alexander Korotkov <aekorot...@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
