Re: Custom table AMs need to include heapam.h because of BulkInsertState - Mailing list pgsql-hackers

From David Rowley
Subject Re: Custom table AMs need to include heapam.h because of BulkInsertState
Date
Msg-id CAKJS1f--t+HePmii3VtbGVKCM4n3-6Ar2t_vd=HLm-hxMvaPqg@mail.gmail.com
Whole thread Raw
In response to Re: Custom table AMs need to include heapam.h because ofBulkInsertState  (Michael Paquier <michael@paquier.xyz>)
Responses Re: Custom table AMs need to include heapam.h because ofBulkInsertState  (Andres Freund <andres@anarazel.de>)
List pgsql-hackers
On Wed, 3 Jul 2019 at 19:35, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Jul 02, 2019 at 01:26:26AM +1200, David Rowley wrote:
> > I've pushed the original patch plus a small change to only call
> > table_finish_bulk_insert() for the target of the copy when we're using
> > bulk inserts.
>
> Yes, sorry for coming late at the party here.  What I meant previously
> is that I did not find the version published at [1] to be natural with
> its structure to define an executor callback which then calls a
> callback for table AMs, still I get your point that it would be better
> to try to avoid unnecessary fsync calls on partitions where no tuples
> have been redirected with a COPY.  The version 1 of the patch attached
> at [2] felt much more straight-forward and cleaner by keeping all the
> table AM callbacks within copy.c.
>
> This has been reverted as of f5db56f, still it seems to me that this
> was moving in the right direction.

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

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]. If you think any of the other ideas I
suggested are better (apart from [2]) then let me know and I can see
about writing a patch. Otherwise, I plan to just fix [2] and push.

> [1]: https://postgr.es/m/CAKJS1f95sB21LBF=1MCsEV+XLtA_JC3mtXx5kgDuHDsOGoWhKg@mail.gmail.com
> [2]: https://postgr.es/m/CAKJS1f_0t-K0_3xe+erXPQ-jgaOb6tRZayErCXF2RpGdUVMt9g@mail.gmail.com

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services



pgsql-hackers by date:

Previous
From: Michael Paquier
Date:
Subject: Re: Custom table AMs need to include heapam.h because ofBulkInsertState
Next
From: Amit Langote
Date:
Subject: Re: Problem with default partition pruning