public inbox for [email protected]  

help / color / mirror / Atom feed

From: Bharath Rupireddy <[email protected]>
To: Justin Pryzby <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Luc Vlaming <[email protected]>
Cc: Paul Guo <[email protected]>
Subject: Re: New Table Access Methods for Multi and Single Inserts
Date: Fri, 18 Dec 2020 07:39:14 +0530
Message-ID: <CALj2ACW3BC5kgdffZ2LD_CT2wQoXVc29kGB74SVWnGZ=UFqcAQ@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CALj2ACVi9eTRYR=gdca5wxtj3Kk_9q9qVccxsS1hngTGOCjPwQ@mail.gmail.com>
	<[email protected]>
	<CALj2ACVgT1iocd5nQ+rEmqt3xcCONkR037qbc8PiojdR39Ag=w@mail.gmail.com>
	<[email protected]>

On Fri, Dec 18, 2020 at 2:14 AM Justin Pryzby <[email protected]> wrote:
> 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);

+1 to make them optional. I will change.

> +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.

What should happen if both multi_insert_v2 and insert_v2 are NULL?
Should we error out from table_insert_v2()?

> 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.

I could not think of other tableam attributes now. But +1 to have that
kind of flexible structure for TableInsertState. So, it can have
tableam type and attributes within the union for each type.

> 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.
> Currently, the bulk insert state is outside and independent of
> tableam. I think we should not make bulk insert state allocation and
> deallocation tableam specific.

Any thoughts on the above point?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

view thread (36+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: New Table Access Methods for Multi and Single Inserts
  In-Reply-To: <CALj2ACW3BC5kgdffZ2LD_CT2wQoXVc29kGB74SVWnGZ=UFqcAQ@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox