From: | David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> |
---|---|
To: | Michael Paquier <michael(at)paquier(dot)xyz> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, Robert Haas <robertmhaas(at)gmail(dot)com>, Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Custom table AMs need to include heapam.h because of BulkInsertState |
Date: | 2019-07-03 07:46:06 |
Message-ID: | CAKJS1f--t+HePmii3VtbGVKCM4n3-6Ar2t_vd=HLm-hxMvaPqg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Wed, 3 Jul 2019 at 19:35, Michael Paquier <michael(at)paquier(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Langote | 2019-07-03 08:13:00 | Re: Problem with default partition pruning |
Previous Message | Michael Paquier | 2019-07-03 07:34:54 | Re: Custom table AMs need to include heapam.h because of BulkInsertState |