Hi Andres, On Wed, Apr 10, 2024 at 7:52 PM Andres Freund <and...@anarazel.de> wrote: > On 2024-04-08 14:54:46 -0400, Robert Haas wrote: > > Exactly how much is getting reverted here? I see these, all since March > > 23rd: > > IMO: > > > > dd1f6b0c17 Provide a way block-level table AMs could re-use > > acquire_sample_rows() > > Should be reverted. > > > > 9bd99f4c26 Custom reloptions for table AM > > Hm. There are some oddities here: > > - It doesn't seem great that relcache.c now needs to know about the default > values for all kinds of reloptions. > > - why is there table_reloptions() and tableam_reloptions()? > > - Why does extractRelOptions() need a TableAmRoutine parameter, extracted by a > caller, instead of doing that work itself? > > > > > 97ce821e3e Fix the parameters order for > > TableAmRoutine.relation_copy_for_cluster() > > Shouldn't be, this is a clear fix. > > > > b1484a3f19 Let table AM insertion methods control index insertion > > I'm not sure. I'm not convinced this is right, nor the opposite. If the > tableam takes control of index insertion, shouldn't nodeModifyTuple know this > earlier, so it doesn't prepare a bunch of index insertion state? Also, > there's pretty much no motivating explanation in the commit. > > > > 27bc1772fc Generalize relation analyze in table AM interface > > Should be reverted. > > > > 87985cc925 Allow locking updated tuples in tuple_update() and tuple_delete() > > Strongly suspect this should be reverted. The last time this was committed it > was far from ready. It's very easy to cause corruption due to subtle bugs in > this area. > > > > c35a3fb5e0 Allow table AM tuple_insert() method to return the different slot > > If the AM returns a different slot, who is responsible for cleaning it up? And > how is creating a new slot for every insert not going to be a measurable > overhead? > > > > 02eb07ea89 Allow table AM to store complex data structures in rd_amcache > > I am doubtful this is right. Is it really sufficient to have a callback for > freeing? What happens when relcache entries are swapped as part of a rebuild? > That works for "flat" caches, but I don't immediately see how it works for > more complicated datastructures. At least from the commit message it's hard > to evaluate how this actually intended to be used.
Thank you for your feedback. I've reverted all of above. ------ Regards, Alexander Korotkov
