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

2019-08-14 Thread David Rowley
On Wed, 17 Jul 2019 at 06:46, Andres Freund wrote: > 2) Add a execPartition.c function that returns all the used tables from >a PartitionTupleRouting*. Here's a patch which implements it that way. I struggled a bit to think of a good name for the execPartition.c function. I ended up with

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

2019-07-17 Thread Andres Freund
Hi, On 2019-07-18 11:57:44 +1200, David Rowley wrote: > However, I spent quite a bit of time trying to make that function as > fast as possible in v12, and since #2 seems like a perfectly good > alternative, I'd rather go with that than to add pollution to > ExecFindPartition's signature. Also,

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

2019-07-17 Thread David Rowley
On Thu, 18 Jul 2019 at 11:36, Andres Freund wrote: > > Hi, > > On 2019-07-18 11:29:37 +1200, David Rowley wrote: > > On Wed, 17 Jul 2019 at 06:46, Andres Freund wrote: > > > 1) Have ExecFindPartition() return via a bool* whether the partition is > > >being accessed for the first time. In

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

2019-07-17 Thread Andres Freund
Hi, On 2019-07-18 11:29:37 +1200, David Rowley wrote: > On Wed, 17 Jul 2019 at 06:46, Andres Freund wrote: > > 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

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

2019-07-17 Thread David Rowley
On Wed, 17 Jul 2019 at 06:46, Andres Freund wrote: > 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

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

2019-07-16 Thread Michael Paquier
Hi David, On Wed, Jul 10, 2019 at 09:40:59PM +1200, David Rowley wrote: > On Wed, 3 Jul 2019 at 19:35, Michael Paquier wrote: >> This has been reverted as of f5db56f, still it seems to me that this >> was moving in the right direction. > > I've pushed this again, this time with the cleanup code

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

2019-07-10 Thread David Rowley
On Wed, 3 Jul 2019 at 19:35, Michael Paquier wrote: > This has been reverted as of f5db56f, still it seems to me that this > was moving in the right direction. I've pushed this again, this time with the cleanup code done in the right order. -- David Rowley

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

2019-07-03 Thread David Rowley
On Wed, 3 Jul 2019 at 19:35, Michael Paquier 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

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

2019-07-03 Thread Michael Paquier
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

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

2019-07-01 Thread David Rowley
On Sun, 30 Jun 2019 at 17:54, David Rowley wrote: > Any further thoughts on this Michael? > > Or Andres? Do you have a preference to which of the approaches > (mentioned upthread) I use for the fix? > > If I don't hear anything I'll probably just push the first fix. The > inefficiency does not

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

2019-06-29 Thread David Rowley
On Mon, 24 Jun 2019 at 23:12, David Rowley wrote: > > On Mon, 24 Jun 2019 at 22:16, Michael Paquier wrote: > > > > Don't take me bad, but I find the solution of defining and using a new > > callback to call the table AM callback not really elegant, and keeping > > all table AM callbacks called

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

2019-06-24 Thread David Rowley
On Mon, 24 Jun 2019 at 22:16, Michael Paquier wrote: > > On Sat, Jun 15, 2019 at 12:25:12PM +1200, David Rowley wrote: > > With the attached version I'm just calling table_finish_bulk_insert() > > once per partition at the end of CopyFrom(). We've got an array with > > all the ResultRelInfos we

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

2019-06-24 Thread Michael Paquier
On Sat, Jun 15, 2019 at 12:25:12PM +1200, David Rowley wrote: > With the attached version I'm just calling table_finish_bulk_insert() > once per partition at the end of CopyFrom(). We've got an array with > all the ResultRelInfos we touched in the proute, so it's mostly a > matter of looping over

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

2019-06-14 Thread David Rowley
On Fri, 14 Jun 2019 at 07:53, Andres Freund wrote: > > On June 12, 2019 6:42:11 PM PDT, David Rowley > wrote: > >Do you see any issue with calling table_finish_bulk_insert() when the > >partition's CopyMultiInsertBuffer is evicted from the > >CopyMultiInsertInfo rather than at the end of the

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

2019-06-13 Thread Andres Freund
Hi, On June 12, 2019 6:42:11 PM PDT, David Rowley wrote: >On Mon, 10 Jun 2019 at 11:45, David Rowley > wrote: >> >> On Sat, 8 Jun 2019 at 04:51, Andres Freund >wrote: >> > David, any opinions on how to best fix this? It's not extremely >obvious >> > how to do so best in the current setup of

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

2019-06-12 Thread David Rowley
On Mon, 10 Jun 2019 at 11:45, David Rowley wrote: > > On Sat, 8 Jun 2019 at 04:51, Andres Freund wrote: > > David, any opinions on how to best fix this? It's not extremely obvious > > how to do so best in the current setup of the partition actually being > > hidden somewhere a few calls away

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

2019-06-11 Thread Robert Haas
On Fri, Jun 7, 2019 at 12:51 PM Andres Freund wrote: > > As far as I can see, any on-disk, row-oriented, block-based AM is > > likely to want the same implementation as the heap. > > I'm pretty doubtful about that. It'd e.g. would make a ton of sense to > keep the VM pinned, even for heap. You

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

2019-06-09 Thread David Rowley
On Sat, 8 Jun 2019 at 04:51, Andres Freund wrote: > > On 2019-06-07 09:48:29 -0400, Robert Haas wrote: > > However, it looks to me as though copy.c can create a bunch of > > BulkInsertStates but only call finish_bulk_insert() once, so unless > > that's a bug in need of fixing I don't quite see

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

2019-06-07 Thread Michael Paquier
On Fri, Jun 07, 2019 at 08:55:36AM -0400, Robert Haas wrote: > Are you talking about the call to ReleaseBulkInsertStatePin, or something > else? Yes, I was referring to ReleaseBulkInsertStatePin() -- Michael signature.asc Description: PGP signature

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

2019-06-07 Thread Andres Freund
Hi, (David, see bottom if you're otherwise not interested). On 2019-06-07 09:48:29 -0400, Robert Haas wrote: > On Sat, Jun 1, 2019 at 3:20 PM Andres Freund wrote: > > > I'd like to think that the best way to deal with that and reduce the > > > confusion would be to move anything related to bulk

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

2019-06-07 Thread Robert Haas
On Sat, Jun 1, 2019 at 3:20 PM Andres Freund wrote: > > I'd like to think that the best way to deal with that and reduce the > > confusion would be to move anything related to bulk inserts into their > > own header/file, meaning the following set: > > - ReleaseBulkInsertStatePin > > -

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

2019-06-07 Thread Robert Haas
On Thu, Jun 6, 2019 at 10:29 PM Michael Paquier wrote: > One thing which is a bit tricky is that for example with COPY FROM we > have a code path which is able to release a buffer held by the bulk > insert state. Are you talking about the call to ReleaseBulkInsertStatePin, or something else? --

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

2019-06-06 Thread Michael Paquier
On Tue, Jun 04, 2019 at 10:18:03AM -0400, Robert Haas wrote: > On Sat, Jun 1, 2019 at 3:09 PM Michael Paquier wrote: >> I am fine to live with the dependency with vacuum.h as it is not that >> strange. However for BulkInsertState we get a hard dependency with a >> heap-related area and it seems

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

2019-06-04 Thread Robert Haas
On Sat, Jun 1, 2019 at 3:09 PM Michael Paquier wrote: > I am fine to live with the dependency with vacuum.h as it is not that > strange. However for BulkInsertState we get a hard dependency with a > heap-related area and it seems to me that we had better move that part > out of heapam.c, as we

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

2019-06-01 Thread Michael Paquier
On Sat, Jun 01, 2019 at 12:19:43PM -0700, Andres Freund wrote: > Yea, I think we should do that at some point. But I'm not sure this is > the right design. Bulk insert probably needs to rather be something > that's allocated inside the AM. Yeah, actually you may be right that I am not taking the

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

2019-06-01 Thread Andres Freund
Hi, On 2019-06-01 15:09:24 -0400, Michael Paquier wrote: > I have been playing lately with the table AM API to do some stuff, and > I got surprised that in the minimum set of headers which needs to be > included for a table AM we have a hard dependency with heapam.h for > BulkInsertState and

Custom table AMs need to include heapam.h because of BulkInsertState

2019-06-01 Thread Michael Paquier
Hi all, I have been playing lately with the table AM API to do some stuff, and I got surprised that in the minimum set of headers which needs to be included for a table AM we have a hard dependency with heapam.h for BulkInsertState and vacuum.h for VacuumParams. I am fine to live with the