Re: Table AM Interface Enhancements

Hi, Alexander!

On Tue, 19 Mar 2024 at 03:34, Alexander Korotkov <aekorotkov(at)gmail(dot)com>
wrote:

> On Sun, Mar 3, 2024 at 1:50 PM Alexander Korotkov <aekorotkov(at)gmail(dot)com>
> wrote:
> > On Mon, Nov 27, 2023 at 10:18 PM Mark Dilger
> > <mark(dot)dilger(at)enterprisedb(dot)com> wrote:
> > >
> > > > On Nov 25, 2023, at 9:47 AM, Alexander Korotkov <
> aekorotkov(at)gmail(dot)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