Re: Pluggable Storage - Andres's take

Hi,

Thanks for the patches!

On 2018-09-03 19:06:27 +1000, Haribabu Kommi wrote:
> I found couple of places where the zheap is using some extra logic in
> verifying
> whether it is zheap AM or not, based on that it used to took some extra
> decisions.
> I am analyzing all the extra code that is done, whether any callbacks can
> handle it
> or not? and how? I can come back with more details later.

Yea, I think some of them will need to stay (particularly around
integrating undo) and som other ones we'll need to abstract.

> >> And then:
> >> - lotsa cleanups
> >> - rebasing onto a newer version of the abstract slot patchset
> >> - splitting out smaller patches
> >>
> >>
> >> You'd moved the bulk insert into tableam callbacks - I don't quite get
> >> why? There's not really anything AM specific in that code?
> >>
> >
> > The main reason of adding them to AM is just to provide a control to
> > the specific AM to decide whether they can support the bulk insert or
> > not?
> >
> > Current framework doesn't support AM specific bulk insert state to be
> > passed from one function to another and it's structure is fixed. This needs
> > to be enhanced to add AM specific private members also.
> >
>
> Do you want me to work on it to make it generic to AM methods to extend
> the structure?

I think the best thing here would be to *remove* all AM abstraction for
bulk insert, until it's actuallly needed. The likelihood of us getting
the interface right and useful without an actual user seems low. Also,
this already is a huge patch...

> @@ -308,7 +308,7 @@ static void CopyFromInsertBatch(CopyState cstate, EState *estate,
> CommandId mycid, int hi_options,
> ResultRelInfo *resultRelInfo,
> BulkInsertState bistate,
> - int nBufferedTuples, TupleTableSlot **bufferedSlots,
> + int nBufferedSlots, TupleTableSlot **bufferedSlots,
> uint64 firstBufferedLineNo);
> static bool CopyReadLine(CopyState cstate);
> static bool CopyReadLineText(CopyState cstate);
> @@ -2309,11 +2309,12 @@ CopyFrom(CopyState cstate)
> void *bistate;
> uint64 processed = 0;
> bool useHeapMultiInsert;
> - int nBufferedTuples = 0;
> + int nBufferedSlots = 0;
> int prev_leaf_part_index = -1;

> -#define MAX_BUFFERED_TUPLES 1000
> +#define MAX_BUFFERED_SLOTS 1000

What's the point of these renames? We're still dealing in tuples. Just
seems to make the patch larger.

> if (useHeapMultiInsert)
> {
> + int tup_size;
> +
> /* Add this tuple to the tuple buffer */
> - if (nBufferedTuples == 0)
> + if (nBufferedSlots == 0)
> + {
> firstBufferedLineNo = cstate->cur_lineno;
> - Assert(bufferedSlots[nBufferedTuples] == myslot);
> - nBufferedTuples++;
> +
> + /*
> + * Find out the Tuple size of the first tuple in a batch and
> + * use it for the rest tuples in a batch. There may be scenarios
> + * where the first tuple is very small and rest can be large, but
> + * that's rare and this should work for majority of the scenarios.
> + */
> + tup_size = heap_compute_data_size(myslot->tts_tupleDescriptor,
> + myslot->tts_values,
> + myslot->tts_isnull);
> + }

This seems too exensive to me. I think it'd be better if we instead
used the amount of input data consumed for the tuple as a proxy. Does that
sound reasonable?

Greetings,

Andres Freund