Re: Table AM Interface Enhancements

Hi, Alexander!

On Wed, 10 Apr 2024 at 16:20, Alexander Korotkov <aekorotkov(at)gmail(dot)com>
wrote:

> On Mon, Apr 8, 2024 at 9:54 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Mon, Apr 8, 2024 at 12:33 PM Alexander Korotkov <aekorotkov(at)gmail(dot)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.
>
> The patch to revert is attached. Given that revert touches the work
> done in 041b96802e, I think it needs some feedback before push.
>
> > 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.
>
> I agree with the facts. But I have a different interpretation on
> this. The patch was committed as 11470f544e on March 23, 2023, then
> reverted on April 3. I've proposed the revised version, but Andres
> complained that this is the new API design days before FF. Then the
> patch with this design was published in the thread for the year with
> periodical rebases. So, I think I expressed my intention with that
> design before 2023 FF, nobody prevented me from expressing objections
> or other feedback during the year. Then I realized that 2024 FF is
> approaching and decided to give this another try for pg18.
>
> But I don't yet see it's wrong with this patch. I waited a year for
> feedback. I waited 2 days after saying "I will push this if no
> objections". Given your feedback now, I get that it would be better to
> do another attempt to commit this earlier.
>
> I admit my mistake with dd1f6b0c17. I get rushed trying to fix the
> things actually making things worse. I apologise for this. But if
> I'm forced to revert 87985cc925 without even hearing any reasonable
> critics besides imperfection of timing, I feel like this is the
> punishment for my mistake with dd1f6b0c17. Pretty unreasonable
> punishment in my view.
>
> > - 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.
>
> I have to repeat again, that I admit my mistake with dd1f6b0c17,
> apologize for that, and make my own conclusions to not repeat this.
> But dd1f6b0c17 seems to be the only one that has a link to the message
> with complains. I went through the list of commits above, it seems
> that others have just linked to the first message of the thread.
> Probably, there is a lack of consensus for some of them. But I never
> heard about a policy to link not just the discussion start, but also
> exact messages expressing agreeing. And I didn't see others doing
> that.
>
> > - 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.
>
> 1) 9bd99f4c26 comprises the reworked patch after working with notes
> from Jeff Davis. I agree it would be better to wait for him to
> express explicit agreement. Before reverting this, I would prefer to
> hear his opinion.
> 2) One of the issues here is that table AM API doesn't have
> documentation, it has just a very brief page which doesn't go deep
> explaining particular API methods. I have heard a lot of complains
> about that from users attempting to write table access methods. It's
> now too late to complain about that (but if I had a wisdom of now back
> during pg12 development I would definitely object against table AM API
> being committed at that shape). I understand I could be more
> proactive and propose a patch with that documentation.
>
> > 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.
>
> Do you propose a ban from March 1 to the end of any year? I think the
> first doesn't make sense, because it leaves only 2 months a year for
> the work. That would create a potential rush during these 2 month and
> could serve exactly opposite to the intention. So, I guess this means
> a ban from March 1 to the FF of any year. The situation now is quite
> unpleasant for me. So I'm far from repeating this next year.
> However, if there should be a formal ban, it should be specified.
> Does it relate to the patches I've pushed, all patches in this thread,
> all similar patches, all table AM patches, or other API patches?
>
> Surely, I'm an interested party and can't be impartial. But I think
> it would be nice if we introduce some general rules based on this
> experience. Could we have some API freeze date some time before the
> feature freeze?
>
> > 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.
>
> This thread had a lot of patches for table AM API. My intention for
> pg17 was to commit the easiest and least contradictory of them. I
> understand there should be more consensus for some of them and
> committing dd1f6b0c17 instead of reverting 27bc1772fc was a mistake.
> But I don't feel good about reverting everything in a row without
> clear feedback.
>
> ------
> Regards,
> Alexander Korotkov
>

In my view, the actual list of what has raised discussion is:
dd1f6b0c17 Provide a way block-level table AMs could re-use
acquire_sample_rows()
27bc1772fc Generalize relation analyze in table AM interface

Proposals to revert the other patches in a wholesale way look to me like an
ill-performed continuation of a discussion [1]. I can't believe that "Let's
select which commits close to FF looks worse than the others" based on
whereabouts, not patch contents is a good and productive way for the
community to use.

At the same time if Andres, who is the most experienced person in the scope
of access methods is willing to give his post-commit re-review of any of
the committed patches and will recommend some of them reverted, it would be
a good sensible input to act accordingly.
patch

[1]
https://www.postgresql.org/message-id/flat/39b1e953-6397-44ba-bb18-d3fdd61839c1%40joeconway.com#e5457f348b8ca90150cb9666aea94547