Date: Thu, 17 Dec 2020 14:44:42 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>,
	Andres Freund <andres@anarazel.de>, Luc Vlaming <luc@swarm64.com>,
	Paul Guo <guopa@vmware.com>
Subject: Re: New Table Access Methods for Multi and Single Inserts
Message-ID: <20201217204442.GX30237@telsasoft.com>
References: 
 <CALj2ACVi9eTRYR=gdca5wxtj3Kk_9q9qVccxsS1hngTGOCjPwQ@mail.gmail.com>
 <20201217050522.GU30237@telsasoft.com>
 <CALj2ACVgT1iocd5nQ+rEmqt3xcCONkR037qbc8PiojdR39Ag=w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CALj2ACVgT1iocd5nQ+rEmqt3xcCONkR037qbc8PiojdR39Ag=w@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Thu, Dec 17, 2020 at 04:35:33PM +0530, Bharath Rupireddy wrote:
> > You made the v2 insert interface a requirement for all table AMs.
> > Should it be optional, and fall back to simple inserts if not implemented ?
> 
> I tried to implement the APIs mentioned by Andreas here in [1]. I just
> used v2 table am APIs in existing table_insert places to show that it
> works. Having said that, if you notice, I moved the bulk insert
> allocation and deallocation to the new APIs table_insert_begin() and
> table_insert_end() respectively, which make them tableam specific.

I mean I think it should be optional for a tableam to support the optimized
insert routines.  Here, you've made it a requirement.

+       Assert(routine->tuple_insert_begin != NULL);
+       Assert(routine->tuple_insert_v2 != NULL);
+       Assert(routine->multi_insert_v2 != NULL);
+       Assert(routine->multi_insert_flush != NULL);
+       Assert(routine->tuple_insert_end != NULL);

+static inline void
+table_multi_insert_v2(TableInsertState *state, TupleTableSlot *slot)
+{
+       state->rel->rd_tableam->multi_insert_v2(state, slot);
+}

If multi_insert_v2 == NULL, I think table_multi_insert_v2() would just call
table_insert_v2(), and begin/flush/end would do nothing.  If
table_multi_insert_v2!=NULL, then you should assert that the other routines are
provided.

Are you thinking that TableInsertState would eventually have additional
attributes which would apply to other tableams, but not to heap ?  Is
heap_insert_begin() really specific to heap ?  It's allocating and populating a
structure based on its arguments, but those same arguments would be passed to
every other AM's insert_begin routine, too.  Do you need a more flexible data
structure, something that would also accomodate extensions?  I'm thinking of
reloptions as a loose analogy.

-- 
Justin