Re: Custom table AMs need to include heapam.h because ofBulkInsertState

Hi,

Sorry for not chiming in again earlier, I was a bit exhausted...


On 2019-07-03 19:46:06 +1200, David Rowley wrote:
> I think the only objection to doing it the way [2] did was, if there
> are more than MAX_PARTITION_BUFFERS partitions then we may end up
> evicting the CopyMultiInsertBuffer out of the CopyMultiInsertInfo and
> thus cause a call to table_finish_bulk_insert() before we're done with
> the copy.

Right.


> It's not impossible that this could happen many times for a
> given partition.  I agree that a working version of [2] is cleaner
> than [1] but it's just the thought of those needless calls.

I think it's fairly important to optimize this. E.g. emitting
unnecessary fsyncs as it'd happen for heap is a pretty huge constant to
add to bulk loading.


> For [1], I wasn't very happy with the way it turned out which is why I
> ended up suggesting a few other ideas. I just don't really like either
> of them any better than [1], so I didn't chase those up, and that's
> why I ended up going for [2].

Yea, I don't like [1] either - they all seems too tied to copy.c's
usage.  Ideas:

1) Have ExecFindPartition() return via a bool* whether the partition is
   being accessed for the first time. In copy.c push the partition onto
   a list of to-be-bulk-finished tables.
2) Add a execPartition.c function that returns all the used tables from
   a PartitionTupleRouting*.

both seem cleaner to me than your proposals in [1], albeit not perfect
either. I think knowing which partitions are referenced is a reasonable
thing to want from the partition machinery. But using bulk-insert etc
seems outside of execPartition.c's remit, so doing that in copy.c seems
to make sense.


Greetings,

Andres Freund