Re: Table AM Interface Enhancements - Mailing list pgsql-hackers

On Mon, Apr 8, 2024 at 9:54 PM Robert Haas <robertmhaas@gmail.com> wrote:
> 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.

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