Hi, I've pushed up a new version to https://github.com/anarazel/postgres-pluggable-storage which now passes all the tests.
Besides a lot of bugfixes, I've rebased the tree, moved TriggerData to be primarily slot based (with a conversion roundtrip when calling trigger functions), and a lot of other small things. On 2018-07-04 20:11:21 +1000, Haribabu Kommi wrote: > On Tue, Jul 3, 2018 at 5:06 PM Andres Freund <and...@anarazel.de> wrote: > > The current state of my version of the patch is *NOT* ready for proper > > review (it doesn't even pass all tests, there's FIXME / elog()s). But I > > think it's getting close enough to it's eventual shape that more eyes, > > and potentially more hands on keyboards, can be useful. > > > > I will try to update it to make sure that it passes all the tests and also > try to > reduce the FIXME's. Cool. Alexander, Haribabu, if you give me (privately) your github accounts, I'll give you write access to that repository. > > The most fundamental issues I had with Haribabu's last version from [2] > > are the following: > > > > - The use of TableTuple, a typedef from void *, is bad from multiple > > fronts. For one it reduces just about all type safety. There were > > numerous bugs in the patch where things were just cast from HeapTuple > > to TableTuple to HeapTuple (and even to TupleTableSlot). I think it's > > a really, really bad idea to introduce a vague type like this for > > development purposes alone, it makes it way too hard to refactor - > > essentially throwing the biggest benefit of type safe languages out of > > the window. > > > > My earlier intention was to remove the HeapTuple usage entirely and replace > it with slot everywhere outside the tableam. But it ended up with TableTuple > before it reach to the stage because of heavy use of HeapTuple. I don't think that's necessary - a lot of the system catalog accesses are going to continue to be heap tuple accesses. And the conversions you did significantly continue to access TableTuples as heap tuples - it was just that the compiler didn't warn about it anymore. A prime example of that is the way the rewriteheap / cluster integreation was done. Cluster continued to sort tuples as heap tuples - even though that's likely incompatible with other tuple formats which need different state. > > Additionally I think it's also the wrong approach architecturally. We > > shouldn't assume that a tuple can efficiently be represented as a > > single palloc'ed chunk. In fact, we should move *away* from relying on > > that so much. > > > > I've thus removed the TableTuple type entirely. > > > > Thanks for the changes, I didn't check the code yet, so for now whenever the > HeapTuple is required it will be generated from slot? Pretty much. > > - the heap tableam did a heap_copytuple() nearly everywhere. Leading to > > a higher memory usage, because the resulting tuples weren't freed or > > anything. There might be a reason for doing such a change - we've > > certainly discussed that before - but I'm *vehemently* against doing > > that at the same time we introduce pluggable storage. Analyzing the > > performance effects will be hard enough without changes like this. > > > > How about using of slot instead of tuple and reusing of it? I don't know > yet whether it is possible everywhere. Not quite sure what you mean? > > Tasks / Questions: > > > > - split up patch > > > > How about generating refactoring changes as patches first based on > the code in your repo as discussed here[1]? Sure - it was just at the moment too much work ;) > > - Change heap table AM to not allocate handler function for each table, > > instead allocate it statically. Avoids a significant amount of data > > duplication, and allows for a few more compiler optimizations. > > > > Some kind of static variable handlers for each tableam, but need to check > how can we access that static handler from the relation. I'm not sure what you mean by "how can we access"? We can just return a pointer from the constant data from the current handler? Except that adding a bunch of consts would be good, the external interface wouldn't need to change? > > - change scan level slot creation to use tableam function for doing so > > - get rid of slot->tts_tid, tts_tupleOid and potentially tts_tableOid > > > > so with this there shouldn't be a way from slot to tid mapping or there > should be some other way. I'm not sure I follow? > - bitmap index scans probably need a new tableam.h callback, abstracting > > bitgetpage() > > > > OK. Any chance you could try to tackle this? I'm going to be mostly out this week, so we'd probably not run across each others feet... Greetings, Andres Freund
