Hi, Alexander! On Tue, 19 Mar 2024 at 03:34, Alexander Korotkov <aekorot...@gmail.com> wrote:
> On Sun, Mar 3, 2024 at 1:50 PM Alexander Korotkov <aekorot...@gmail.com> > wrote: > > On Mon, Nov 27, 2023 at 10:18 PM Mark Dilger > > <mark.dil...@enterprisedb.com> wrote: > > > > > > > On Nov 25, 2023, at 9:47 AM, Alexander Korotkov < > aekorot...@gmail.com> wrote: > > > > > > > >> Should the patch at least document which parts of the EState are > expected to be in which states, and which parts should be viewed as > undefined? If the implementors of table AMs rely on any/all aspects of > EState, doesn't that prevent future changes to how that structure is used? > > > > > > > > New tuple tuple_insert_with_arbiter() table AM API method needs > EState > > > > argument to call executor functions: ExecCheckIndexConstraints(), > > > > ExecUpdateLockMode(), and ExecInsertIndexTuples(). I think we > > > > probably need to invent some opaque way to call this executor > function > > > > without revealing EState to table AM. Do you think this could work? > > > > > > We're clearly not accessing all of the EState, just some specific > fields, such as es_per_tuple_exprcontext. I think you could at least > refactor to pass the minimum amount of state information through the table > AM API. > > > > Yes, the table AM doesn't need the full EState, just the ability to do > > specific manipulation with tuples. I'll refactor the patch to make a > > better isolation for this. > > Please find the revised patchset attached. The changes are following: > 1. Patchset is rebase. to the current master. > 2. Patchset is reordered. I tried to put less debatable patches to the > top. > 3. tuple_is_current() method is moved from the Table AM API to the > slot as proposed by Matthias van de Meent. > 4. Assert added to the table_free_rd_amcache() as proposed by Pavel > Borisov. > Patches 0001-0002 are unchanged compared to the last version in thread [1]. In my opinion, it's still ready to be committed, which was not done for time were too close to feature freeze one year ago. 0003 - Assert added from previous version. I still have a strong opinion that allowing multi-chunked data structures instead of single chunks is completely safe and makes natural process of Postgres improvement that is self-justified. The patch is simple enough and ready to be pushed. 0004 (previously 0007) - Have not changed, and there is consensus that this is reasonable. I've re-checked the current code. Looks safe considering returning a different slot, which I doubted before. So consider this patch also ready. 0005 (previously 0004) - Unused argument in the is_current_xact_tuple() signature is removed. Also comparing to v1 the code shifted from tableam methods to TTS's level. I'd propose to remove Assert(!TTS_EMPTY(slot)) for tts_minimal_is_current_xact_tuple() and tts_virtual_is_current_xact_tuple() as these are only error reporting functions that don't use slot actually. Comment similar to: +/* + * VirtualTupleTableSlots never have a storage tuples. We generally + * shouldn't get here, but provide a user-friendly message if we do. + */ also applies to tts_minimal_is_current_xact_tuple() I'd propose changes for clarity of this comment: %s/a storage tuples/storage tuples/g %s/generally//g Otherwise patch 0005 also looks good to me. I'm planning to review the remaining patches. Meanwhile think pushing what is now ready and uncontroversial is a good intention. Thank you for the work done on this patchset! Regards, Pavel Borisov, Supabase. [1]. https://www.postgresql.org/message-id/CAPpHfdua-YFw3XTprfutzGp28xXLigFtzNbuFY8yPhqeq6X5kg%40mail.gmail.com