Re: [HACKERS] Pluggable storage

[David Steele]

On 2/26/18 3:19 AM, Alexander Korotkov wrote:
> On Fri, Feb 23, 2018 at 2:20 AM, Robert Haas  > wrote:
> 
> On Fri, Feb 16, 2018 at 5:56 AM, Alexander Korotkov
> mailto:a.korot...@postgrespro.ru>> wrote:
> > BTW, EnterpriseDB announces zheap table access method (heap with undo 
> log)
> > [2].  I think this is great, and I'm looking forward for publishing 
> zheap in
> > mailing lists.  But I'm concerning about its compatibility with 
> pluggable
> > table access methods API.  Does zheap use table AM API from this 
> thread?  Or
> > does it just override current heap and needs to be adopted to use table 
> AM
> > API?  Or does it implements own API?
> 
> Right now it just hacks the code.  The plan is to adapt it to whatever
> API we settle on in this thread.
> 
> 
> Great, thank you for clarification.  I'm looking forward reviewing zheap :)
I think this entry should be moved the the next CF.  I'll do that
tomorrow unless there are objections.

Regards,
-- 
-David
da...@pgmasters.net

Re: [HACKERS] Pluggable storage

[Michael Paquier]

On Wed, Mar 28, 2018 at 12:23:56PM -0400, David Steele wrote:
> I think this entry should be moved the the next CF.  I'll do that
> tomorrow unless there are objections.

Instead of moving things to the next CF by default, perhaps it would
make more sense to mark things as reviewed with feedback as this is the
last CF?  There is a 5-month gap between this commit fest and the next
one, I am getting afraid of flooding the beginning of v12 development
cycle with entries which keep rotting over time.  If the author(s) claim
that they will be able to work on it, then that's of course fine.

Sorry for the digression, patches ignored across CFs contribute to the
bloat we see, and those eat the time of the CFM.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] Pluggable storage

[David Steele]

On 3/28/18 8:39 PM, Michael Paquier wrote:
> On Wed, Mar 28, 2018 at 12:23:56PM -0400, David Steele wrote:
>> I think this entry should be moved the the next CF.  I'll do that
>> tomorrow unless there are objections.
> 
> Instead of moving things to the next CF by default, perhaps it would
> make more sense to mark things as reviewed with feedback as this is the
> last CF?  There is a 5-month gap between this commit fest and the next
> one, I am getting afraid of flooding the beginning of v12 development
> cycle with entries which keep rotting over time.  If the author(s) claim
> that they will be able to work on it, then that's of course fine.

I agree and I do my best to return patches that have stalled, but I
don't think this patch is in that category.  It has gotten review and
has been kept up to date.  I don't think it's a good fit for v11 but I'd
like to see it in the first CF for v12.

> Sorry for the digression, patches ignored across CFs contribute to the
> bloat we see, and those eat the time of the CFM.

There's no question that bloat has become a problem.  I don't have all
the answers, but vigilance by the CFMs is certainly a good start.

Regards,
-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Pluggable storage

[Amit Kapila]

On Thu, Jun 14, 2018 at 1:50 AM, Haribabu Kommi
 wrote:
>
> On Fri, Apr 20, 2018 at 4:44 PM Haribabu Kommi 
> wrote:
>
> VACUUM:
> Not much changes are done in this apart moving the Vacuum visibility
> functions as part of the
> storage. But idea for the Vacuum was with each access method can define how
> it should perform.
>

We are planning to have a somewhat different mechanism for vacuum (for
non-delete marked indexes), so if you can provide some details or
discuss what you have in mind before implementation of same, that
would be great.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Pluggable storage

[Haribabu Kommi]

On Thu, Jun 14, 2018 at 12:25 PM Amit Kapila 
wrote:

> On Thu, Jun 14, 2018 at 1:50 AM, Haribabu Kommi
>  wrote:
> >
> > On Fri, Apr 20, 2018 at 4:44 PM Haribabu Kommi  >
> > wrote:
> >
> > VACUUM:
> > Not much changes are done in this apart moving the Vacuum visibility
> > functions as part of the
> > storage. But idea for the Vacuum was with each access method can define
> how
> > it should perform.
> >
>
> We are planning to have a somewhat different mechanism for vacuum (for
> non-delete marked indexes), so if you can provide some details or
> discuss what you have in mind before implementation of same, that
> would be great.
>

OK. Thanks for your input. We will discuss the changes before proceed to
code.

Apart from the this, the pluggable storage API contains some re-factored
changes
along with API, some of the re-factored changes are

1. Change the snapshot satisfies type from function to an enum
2. Try to return always the palloced tuple instead of a pointer to buffer
   (This change may have performance impact,so can be done later).
3. Perform a tuple visibility check at heap itself for the page mode
scenario also
4. New function ExecSlotCompare to compare two slots or tuple by storing it
a temp slot.
5. heap_fetch and heap_lock_tuple returns the palloced tuple, not the
pointer to the buffer
6. The index insertion logic decision is moved into heap itself(insert,
update), not in executor.
7. Split HeapscanDesc into two and remove it's usage outside heap of the
second split
8. Move the tuple traversing and providing the updated record to heap.

Is it fine to create these changes as separate patches and can go if the
changes are fine
and doesn't have any impact?

Any comments or additions or deletions to the above list?

Regards,
Haribabu Kommi
Fujitsu Australia

Re: [HACKERS] Pluggable storage

[Haribabu Kommi]

On Tue, Jan 9, 2018 at 11:42 PM, Haribabu Kommi 
wrote:

>
> Updated patches are attached.
>

To integrate the columnar store with the pluggable storage API, I found that
there are couple of other things also that needs to be supported.

1. Choosing the right table access method for a particular table?

I am thinking of adding a new table option to let the user select the
correct table
access method that the user wants for the table. HEAP is the default access
method. This approach may be simple and doesn't need any syntax changes.

Or Is it fine to add syntax "USING method" to CREATE TABLE similar like
CREATE INDEX?

comments?

2. As the columnar storage needs many other relations that are needs to be
created along with main relation.

As these extra relations are internal to the storage and shouldn't be
visible
directly from pg_class and these will be stored in the storage specific
catalog tables. A dependency is created for the original table as these
storage
specific tables must be created/dropped/altered whenever there is a change
with the original table.

Is it fine to add new API while creating/altering/drop the table to get the
control?
or to use only exiting processutility hook?


Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] Pluggable storage

[Alexander Korotkov]

Hi, Haribabu!

On Mon, Feb 5, 2018 at 2:22 PM, Haribabu Kommi 
wrote:

>
> On Tue, Jan 9, 2018 at 11:42 PM, Haribabu Kommi 
> wrote:
>
>>
>> Updated patches are attached.
>>
>
> To integrate the columnar store with the pluggable storage API, I found
> that
> there are couple of other things also that needs to be supported.
>
> 1. Choosing the right table access method for a particular table?
>
> I am thinking of adding a new table option to let the user select the
> correct table
> access method that the user wants for the table. HEAP is the default access
> method. This approach may be simple and doesn't need any syntax changes.
>
> Or Is it fine to add syntax "USING method" to CREATE TABLE similar like
> CREATE INDEX?
>
> comments?
>

Sure, user needs to specify particular table access method when creating a
table.  "USING method" is good for me.  I think it's better to reuse
already existing syntax rather than reinventing something new.


> 2. As the columnar storage needs many other relations that are needs to be
> created along with main relation.
>

That's an interesting point.  During experimenting on some new index access
methods I also have to define some kind of "internal" relations invisible
for user, but essential for main relation functionality.  I've made them in
hackery manner, and I think legal mechanism for them would be very good.


> As these extra relations are internal to the storage and shouldn't be
> visible
> directly from pg_class and these will be stored in the storage specific
> catalog tables. A dependency is created for the original table as these
> storage
> specific tables must be created/dropped/altered whenever there is a change
> with the original table.
>

I think that internal relations should be visible from pg_class, otherwise
it wouldn't be possible to define dependencies over them.  But they should
have different relkind, so they wouldn't be messed up with regular
relations.

Is it fine to add new API while creating/altering/drop the table to get the
> control?
> or to use only exiting processutility hook?
>

I believe that we need some generic way for defining internal relations,
not hooks.  For me it seems like useful feature for further development of
both index access methods and table access methods.

During developer meeting [1], I've proposed to reorder patches so that
refactoring patches go first and API introduction patches go afterwards.
That would make possible to commit some of refactoring patches earlier
without necessary committing API in the same release cycle.  If no
objection, I would do that reordering.

BTW, EnterpriseDB announces zheap table access method (heap with undo log)
[2].  I think this is great, and I'm looking forward for publishing zheap
in mailing lists.  But I'm concerning about its compatibility with
pluggable table access methods API.  Does zheap use table AM API from this
thread?  Or does it just override current heap and needs to be adopted to
use table AM API?  Or does it implements own API?

*Links*

1. https://wiki.postgresql.org/wiki/FOSDEM/PGDay_2018_
Developer_Meeting#Minutes
2. http://rhaas.blogspot.com.by/2018/01/do-or-undo-there-is-no-vacuum.html

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] Pluggable storage

[Robert Haas]

On Fri, Feb 16, 2018 at 5:56 AM, Alexander Korotkov
 wrote:
> BTW, EnterpriseDB announces zheap table access method (heap with undo log)
> [2].  I think this is great, and I'm looking forward for publishing zheap in
> mailing lists.  But I'm concerning about its compatibility with pluggable
> table access methods API.  Does zheap use table AM API from this thread?  Or
> does it just override current heap and needs to be adopted to use table AM
> API?  Or does it implements own API?

Right now it just hacks the code.  The plan is to adapt it to whatever
API we settle on in this thread.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] Pluggable storage

[Alexander Korotkov]

On Fri, Feb 23, 2018 at 2:20 AM, Robert Haas  wrote:

> On Fri, Feb 16, 2018 at 5:56 AM, Alexander Korotkov
>  wrote:
> > BTW, EnterpriseDB announces zheap table access method (heap with undo
> log)
> > [2].  I think this is great, and I'm looking forward for publishing
> zheap in
> > mailing lists.  But I'm concerning about its compatibility with pluggable
> > table access methods API.  Does zheap use table AM API from this
> thread?  Or
> > does it just override current heap and needs to be adopted to use table
> AM
> > API?  Or does it implements own API?
>
> Right now it just hacks the code.  The plan is to adapt it to whatever
> API we settle on in this thread.


Great, thank you for clarification.  I'm looking forward reviewing zheap :)

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] Pluggable storage

[Michael Paquier]

On Tue, Nov 7, 2017 at 6:34 PM, Haribabu Kommi  wrote:
> On Tue, Oct 31, 2017 at 8:59 PM, Haribabu Kommi 
> wrote:
>> Additional changes that are done in the patches compared to earlier
>> patches apart from rebase.
>
> Rebased patches are attached.

This set of patches needs again a... Rebase.
-- 
Michael

Re: [HACKERS] Pluggable storage

[Alvaro Herrera]

Hmm.  Am I reading it right that this discussion led to moving
essentially all code from tqual.c to heapam?  Given the hard time we've
had to get tqual.c right, it seems fundamentally misguided to me to
require that every single storage AM reimplements all the visibility
routines.

I think that changing tqual's API (such as not passing HeapTuples
anymore but some other more general representation) would be okay and
should be sufficient, but this wholesale movement of code seems
dangerous and wasteful in terms of future reimplementations that will be
necessary.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Pluggable storage

[Amit Kapila]

On Tue, Nov 14, 2017 at 4:12 PM, Alvaro Herrera  wrote:
> Hmm.  Am I reading it right that this discussion led to moving
> essentially all code from tqual.c to heapam?  Given the hard time we've
> had to get tqual.c right, it seems fundamentally misguided to me to
> require that every single storage AM reimplements all the visibility
> routines.
>
> I think that changing tqual's API (such as not passing HeapTuples
> anymore but some other more general representation) would be okay and
> should be sufficient, but this wholesale movement of code seems
> dangerous and wasteful in terms of future reimplementations that will be
> necessary.
>

I don't think the idea is to touch existing tqual.c in any significant
way.  However, some other storage engine might need a different way to
check the visibility of tuples so we need provision for that. I think
for storage engine where tuple headers no longer contain transaction
information and or the old versions of tuples are chained in separate
storage (say undo storage), current visibility routines can be used.
I think the current tqual.c is quite tightly coupled with the
HeapTuple representation, so changing that or adding more code to it
for another storage engine with different tuple format won't be of
much use.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Pluggable storage

[Michael Paquier]

On Tue, Nov 14, 2017 at 5:09 PM, Michael Paquier
 wrote:
> On Tue, Nov 7, 2017 at 6:34 PM, Haribabu Kommi  
> wrote:
>> On Tue, Oct 31, 2017 at 8:59 PM, Haribabu Kommi 
>> wrote:
>>> Additional changes that are done in the patches compared to earlier
>>> patches apart from rebase.
>>
>> Rebased patches are attached.
>
> This set of patches needs again a... Rebase.

No rebased versions have showed up for two weeks. For now I am marking
this patch as returned with feedback.
-- 
Michael

Re: [HACKERS] Pluggable storage

[Alexander Korotkov]

Hi!

On Wed, Dec 27, 2017 at 6:54 AM, Haribabu Kommi 
wrote:

>
> On Tue, Dec 12, 2017 at 3:06 PM, Haribabu Kommi 
> wrote:
>
>>
>> I restructured that patch files to avoid showing unnecessary
>> modifications,
>> and also it will be easy for adding of new API's based on the all the
>> functions
>> that are exposed by heapam module easily compared earlier.
>>
>> Attached are the latest set of patches. I will work on the remaining
>> pending
>> items.
>>
>
> Apart from rebase to the latest master code, following are the additional
> changes,
>
> 1. Added API for bulk insert and rewrite functionality(Logical rewrite is
> not touched yet)
> 2. Tuple lock API interface redesign to remove the traversal logic from
> executor module.
>

Great, thank you!


> The tuple lock API interface changes are from "Alexander Korotkov" from
> "PostgresPro".
> Thanks Alexander. Currently we both are doing joint development for faster
> closure of
> open items that are pending to bring the "pluggable storage API" into a
> good shape.
>

Thank you for announcing this.  Yes, pluggable storage API requires a lot
of work to get into committable shape.  This is why I've decided to join
the development.

Let me explain the idea behind new tuple lock API and further patches I
plan to send.  As I noted upthread, I consider possibility of alternative
MVCC implementations as vital property of pluggable storage API.  These
include undo log option when tuple is updated in-place while old version of
tuple is displaced to some special area.  In this case, new version of
tuple would reside on same TID as old version of tuple.  This is an
important point, because TID is not really tuple identifier anymore.
Naturally, TID becomes a row identifier while tuple may be identified by
pair (tid, snapshot).  For current heap, snapshot is redundant and can be
used just for assert checking (tuple on given tid is really visible using
given snapshot).  For heap with undo log, appropriate tuple could be found
by snapshot in the undo chain associated with given tid.

One of consequences of above is that we cannot use fact that tid isn't
changed after update as sign that tuple was deleted.  This is why I've
introduced HTSU_Result  HeapTupleDeleted.  Another consequence is that our
tid traverse logic in the executor layer is not valid anymore.  For
instance, this traversal from older tuple to latter tuple doesn't make any
sense for heap with undo log where latter tuple is more easily accessible
than older tuple.  This is why I decided to hide this logic in storage
layer and provide TUPLE_LOCK_FLAG_FIND_LAST_VERSION flag which indicates
that lock_tuple() have to find latest updated version and lock it.  I've
also changed follow_updates bool to more explicit
TUPLE_LOCK_FLAG_LOCK_UPDATE_IN_PROGRESS flag in order to not mess it with
previous flag which also kind of follow updates.  Third consequence is that
we have to pass snapshot to tuple_update() and tuple_delete() methods to
let them check if row was concurrently updated while residing on the same
TID.  I'm going to provide this change as separate patch.

Also, I appreciate that now tuple_insert() and tuple_update() methods are
responsible for inserting index tuples.  This unleash pluggable storages to
implement another way of interaction with indexes.  However, I didn't get
the point of passing InsertIndexTuples IndexFunc to them.  Now, we're
always passing ExecInsertIndexTuples() to this argument.  As I understood
storage is free to either call ExecInsertIndexTuples() or implement its own
logic of interaction with indexes.  But, I don't understand why do we need
a callback when tuple_insert() and tuple_update() can call
ExecInsertIndexTuples() directly if needed.  Another thing is that
tuple_delete() could also interact with indexes (especially when we will
enhance index access method API), and we need to pass meta-information
about indexes to tuple_delete() too.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] Pluggable storage

[Alexander Korotkov]

On Wed, Jan 3, 2018 at 10:08 AM, Haribabu Kommi 
wrote:

>
> On Wed, Dec 27, 2017 at 11:33 PM, Alexander Korotkov <
> a.korot...@postgrespro.ru> wrote:
>
>>
>> Also, I appreciate that now tuple_insert() and tuple_update() methods are
>> responsible for inserting index tuples.  This unleash pluggable storages to
>> implement another way of interaction with indexes.  However, I didn't get
>> the point of passing InsertIndexTuples IndexFunc to them.  Now, we're
>> always passing ExecInsertIndexTuples() to this argument.  As I
>> understood storage is free to either call ExecInsertIndexTuples() or
>> implement its own logic of interaction with indexes.  But, I don't
>> understand why do we need a callback when tuple_insert() and tuple_update()
>> can call ExecInsertIndexTuples() directly if needed.  Another thing is that
>> tuple_delete() could also interact with indexes (especially when we will
>> enhance index access method API), and we need to pass meta-information
>> about indexes to tuple_delete() too.
>>
>
> The main reason for which I added the callback function to not to
> introduce the
> dependency of storage on executor functions. This way storage can call the
> function that is passed to it without any knowledge. I added the function
> pointer
> for tuple_delete also in the new patches, currently it is passed as NULL
> for heap.
> These API's can be enhanced later.
>

Understood, but in order to implement alternative behavior with indexes
(for example,
insert index tuples to only some of indexes), storage am will still have to
call executor
functions.  So, yes this needs to be enhanced.  Probably, we just need to
implement
nicer executor API for storage am.


> Apart from rebase, Added storage shared memory API, currently this API is
> used
> only by the syncscan. And also all the exposed functions of syncscan usage
> is
> removed outside the heap.
>

This makes me uneasy.  You introduce two new hooks for size estimation and
initialization
of shared memory needed by storage am's.  But if storage am is implemented
in shared library,
then this shared library can use our generic method for allocation of
shared memory
(including memory needed by storage am).  If storage am is builtin, then
hooks are also not
needed, because we know all our builtin storage am's in advance.  For me,
it would be
nice to encapsulate heap am requirements in shared memory into functions
like
HeapAmShmemSize() and HeapAmShmemInit(), and don't explicitly show outside
that
this memory is needed for synchronized scan.  But separate hooks don't look
justified for me.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] Pluggable storage

[Haribabu Kommi]

On Thu, Jan 4, 2018 at 10:00 AM, Alexander Korotkov <
a.korot...@postgrespro.ru> wrote:

> On Wed, Jan 3, 2018 at 10:08 AM, Haribabu Kommi 
> wrote:
>
>>
>> On Wed, Dec 27, 2017 at 11:33 PM, Alexander Korotkov <
>> a.korot...@postgrespro.ru> wrote:
>>
>>>
>>> Also, I appreciate that now tuple_insert() and tuple_update() methods
>>> are responsible for inserting index tuples.  This unleash pluggable
>>> storages to implement another way of interaction with indexes.  However, I
>>> didn't get the point of passing InsertIndexTuples IndexFunc to them.  Now,
>>> we're always passing ExecInsertIndexTuples() to this argument.  As I
>>> understood storage is free to either call ExecInsertIndexTuples() or
>>> implement its own logic of interaction with indexes.  But, I don't
>>> understand why do we need a callback when tuple_insert() and tuple_update()
>>> can call ExecInsertIndexTuples() directly if needed.  Another thing is that
>>> tuple_delete() could also interact with indexes (especially when we will
>>> enhance index access method API), and we need to pass meta-information
>>> about indexes to tuple_delete() too.
>>>
>>
>> The main reason for which I added the callback function to not to
>> introduce the
>> dependency of storage on executor functions. This way storage can call the
>> function that is passed to it without any knowledge. I added the function
>> pointer
>> for tuple_delete also in the new patches, currently it is passed as NULL
>> for heap.
>> These API's can be enhanced later.
>>
>
> Understood, but in order to implement alternative behavior with indexes
> (for example,
> insert index tuples to only some of indexes), storage am will still have
> to call executor
> functions.  So, yes this needs to be enhanced.  Probably, we just need to
> implement
> nicer executor API for storage am.
>

OK.


> Apart from rebase, Added storage shared memory API, currently this API is
>> used
>> only by the syncscan. And also all the exposed functions of syncscan
>> usage is
>> removed outside the heap.
>>
>
> This makes me uneasy.  You introduce two new hooks for size estimation and
> initialization
> of shared memory needed by storage am's.  But if storage am is implemented
> in shared library,
> then this shared library can use our generic method for allocation of
> shared memory
> (including memory needed by storage am).  If storage am is builtin, then
> hooks are also not
> needed, because we know all our builtin storage am's in advance.  For me,
> it would be
> nice to encapsulate heap am requirements in shared memory into functions
> like
> HeapAmShmemSize() and HeapAmShmemInit(), and don't explicitly show outside
> that
> this memory is needed for synchronized scan.  But separate hooks don't
> look justified for me.
>

Yes, I agree that for the builtin storage's there is no need of hooks. But
in future,
if we want to support multiple storage's in an instance, we may need hooks
for shared memory
registration. I am fine to change it.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] Pluggable storage

[Alexander Korotkov]

On Thu, Jan 4, 2018 at 8:03 AM, Haribabu Kommi 
wrote:

> On Thu, Jan 4, 2018 at 10:00 AM, Alexander Korotkov <
> a.korot...@postgrespro.ru> wrote:
>
>> On Wed, Jan 3, 2018 at 10:08 AM, Haribabu Kommi > > wrote:
>>
>>> Apart from rebase, Added storage shared memory API, currently this API
>>> is used
>>>
>> only by the syncscan. And also all the exposed functions of syncscan
>>> usage is
>>> removed outside the heap.
>>>
>>
>> This makes me uneasy.  You introduce two new hooks for size estimation
>> and initialization
>> of shared memory needed by storage am's.  But if storage am is
>> implemented in shared library,
>> then this shared library can use our generic method for allocation of
>> shared memory
>> (including memory needed by storage am).  If storage am is builtin, then
>> hooks are also not
>> needed, because we know all our builtin storage am's in advance.  For me,
>> it would be
>> nice to encapsulate heap am requirements in shared memory into functions
>> like
>> HeapAmShmemSize() and HeapAmShmemInit(), and don't explicitly show
>> outside that
>> this memory is needed for synchronized scan.  But separate hooks don't
>> look justified for me.
>>
>
> Yes, I agree that for the builtin storage's there is no need of hooks. But
> in future,
> if we want to support multiple storage's in an instance, we may need hooks
> for shared memory
> registration. I am fine to change it.
>

Yes, but we already have hooks for shared memory registration in shared
modules.  I don't see the point for another hooks for the same purpose.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] Pluggable storage

[Haribabu Kommi]

On Fri, Jan 5, 2018 at 9:55 AM, Alexander Korotkov <
a.korot...@postgrespro.ru> wrote:

> On Thu, Jan 4, 2018 at 8:03 AM, Haribabu Kommi 
> wrote:
>
>> On Thu, Jan 4, 2018 at 10:00 AM, Alexander Korotkov <
>> a.korot...@postgrespro.ru> wrote:
>>
>>> On Wed, Jan 3, 2018 at 10:08 AM, Haribabu Kommi <
>>> kommi.harib...@gmail.com> wrote:
>>>
 Apart from rebase, Added storage shared memory API, currently this API
 is used

>>> only by the syncscan. And also all the exposed functions of syncscan
 usage is
 removed outside the heap.

>>>
>>> This makes me uneasy.  You introduce two new hooks for size estimation
>>> and initialization
>>> of shared memory needed by storage am's.  But if storage am is
>>> implemented in shared library,
>>> then this shared library can use our generic method for allocation of
>>> shared memory
>>> (including memory needed by storage am).  If storage am is builtin, then
>>> hooks are also not
>>> needed, because we know all our builtin storage am's in advance.  For
>>> me, it would be
>>> nice to encapsulate heap am requirements in shared memory into functions
>>> like
>>> HeapAmShmemSize() and HeapAmShmemInit(), and don't explicitly show
>>> outside that
>>> this memory is needed for synchronized scan.  But separate hooks don't
>>> look justified for me.
>>>
>>
>> Yes, I agree that for the builtin storage's there is no need of hooks.
>> But in future,
>> if we want to support multiple storage's in an instance, we may need
>> hooks for shared memory
>> registration. I am fine to change it.
>>
>
> Yes, but we already have hooks for shared memory registration in shared
> modules.  I don't see the point for another hooks for the same purpose.
>

Oh, yes, I missed it. I will update the patch and share it later.


Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] Pluggable storage

[Robert Haas]

I do not like the way that this patch set uses the word "storage".  In
current usage, storage is a thing that certain kinds of relations
have.  Plain relations (a.k.a. heap tables) have storage, indexes have
storage, materialized views have storage, TOAST tables have storage,
and sequences have storage.  This patch set wants to use "storage AM"
to mean a replacement for a plain heap table, but I think that's going
to create a lot of confusion, because it overlaps heavily with the
existing meaning yet is different.  My suggestion is to call these
"table access methods" rather than "storage access methods".  Then,
the default table AM can be heap.  This has the nice property that you
create an index using CREATE INDEX and the support functions arrive
via an IndexAmRoutine, so correspondingly you would create a table
using CREATE TABLE and the support functions would arrive via a
TableAmRoutine -- so all the names match.

An alternative would be to call the new thing a "heap AM" with
HeapAmRoutine as the support function structure.  That's also not
unreasonable.  In that case, we're deciding that "heap" is not just
the name of the current implementation, but the name of the kind of
thing that backs a table at the storage level.  I don't like that
quite as well because then we've got a class of things called a heap
of which the current and only implementation is called heap, which is
a bit confusing in my view.  But we could probably make it work.

If we adopt the first proposal, it leads to another nice parallel: we
can have src/backend/access/table for those things which are generic
to table AMs, just as we have src/backend/access/index for things
which are generic to index AMs, and then src/backend/access/
for things which are specific to a particular AM.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] Pluggable storage

[Alexander Korotkov]

On Fri, Jan 5, 2018 at 7:20 PM, Robert Haas  wrote:

> I do not like the way that this patch set uses the word "storage".  In
> current usage, storage is a thing that certain kinds of relations
> have.  Plain relations (a.k.a. heap tables) have storage, indexes have
> storage, materialized views have storage, TOAST tables have storage,
> and sequences have storage.  This patch set wants to use "storage AM"
> to mean a replacement for a plain heap table, but I think that's going
> to create a lot of confusion, because it overlaps heavily with the
> existing meaning yet is different.


Good point, thank you for noticing that.  Name "storage" is really confusing
for this purpose.


> My suggestion is to call these
> "table access methods" rather than "storage access methods".  Then,
> the default table AM can be heap.  This has the nice property that you
> create an index using CREATE INDEX and the support functions arrive
> via an IndexAmRoutine, so correspondingly you would create a table
> using CREATE TABLE and the support functions would arrive via a
> TableAmRoutine -- so all the names match.
>
> An alternative would be to call the new thing a "heap AM" with
> HeapAmRoutine as the support function structure.  That's also not
> unreasonable.  In that case, we're deciding that "heap" is not just
> the name of the current implementation, but the name of the kind of
> thing that backs a table at the storage level.  I don't like that
> quite as well because then we've got a class of things called a heap
> of which the current and only implementation is called heap, which is
> a bit confusing in my view.  But we could probably make it work.
>
> If we adopt the first proposal, it leads to another nice parallel: we
> can have src/backend/access/table for those things which are generic
> to table AMs, just as we have src/backend/access/index for things
> which are generic to index AMs, and then src/backend/access/
> for things which are specific to a particular AM.


I would vote for the first proposal: table AM.  Because we eventually
might get index-organized tables whose don't have something like heap.
So, it would be nice to avoid hardcoding "heap" name.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] Pluggable storage

[Robert Haas]

On Fri, Feb 16, 2018 at 5:56 AM, Alexander Korotkov
 wrote:
> BTW, EnterpriseDB announces zheap table access method (heap with undo log)
> [2].  I think this is great, and I'm looking forward for publishing zheap in
> mailing lists.  But I'm concerning about its compatibility with pluggable
> table access methods API.  Does zheap use table AM API from this thread?  Or
> does it just override current heap and needs to be adopted to use table AM
> API?  Or does it implements own API?

Right now it just hacks the code.  The plan is to adapt it to whatever
API we settle on in this thread.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] Pluggable storage

[Alexander Korotkov]

On Fri, Feb 23, 2018 at 2:20 AM, Robert Haas  wrote:

> On Fri, Feb 16, 2018 at 5:56 AM, Alexander Korotkov
>  wrote:
> > BTW, EnterpriseDB announces zheap table access method (heap with undo
> log)
> > [2].  I think this is great, and I'm looking forward for publishing
> zheap in
> > mailing lists.  But I'm concerning about its compatibility with pluggable
> > table access methods API.  Does zheap use table AM API from this
> thread?  Or
> > does it just override current heap and needs to be adopted to use table
> AM
> > API?  Or does it implements own API?
>
> Right now it just hacks the code.  The plan is to adapt it to whatever
> API we settle on in this thread.


Great, thank you for clarification.  I'm looking forward reviewing zheap :)

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] Pluggable storage

[David Steele]

On 2/26/18 3:19 AM, Alexander Korotkov wrote:
> On Fri, Feb 23, 2018 at 2:20 AM, Robert Haas  > wrote:
> 
> On Fri, Feb 16, 2018 at 5:56 AM, Alexander Korotkov
> mailto:a.korot...@postgrespro.ru>> wrote:
> > BTW, EnterpriseDB announces zheap table access method (heap with undo 
> log)
> > [2].  I think this is great, and I'm looking forward for publishing 
> zheap in
> > mailing lists.  But I'm concerning about its compatibility with 
> pluggable
> > table access methods API.  Does zheap use table AM API from this 
> thread?  Or
> > does it just override current heap and needs to be adopted to use table 
> AM
> > API?  Or does it implements own API?
> 
> Right now it just hacks the code.  The plan is to adapt it to whatever
> API we settle on in this thread.
> 
> 
> Great, thank you for clarification.  I'm looking forward reviewing zheap :)
I think this entry should be moved the the next CF.  I'll do that
tomorrow unless there are objections.

Regards,
-- 
-David
da...@pgmasters.net

Re: [HACKERS] Pluggable storage

[Alexander Korotkov]

On Wed, Jan 3, 2018 at 10:08 AM, Haribabu Kommi 
wrote:

>
> On Wed, Dec 27, 2017 at 11:33 PM, Alexander Korotkov <
> a.korot...@postgrespro.ru> wrote:
>
>>
>> Also, I appreciate that now tuple_insert() and tuple_update() methods are
>> responsible for inserting index tuples.  This unleash pluggable storages to
>> implement another way of interaction with indexes.  However, I didn't get
>> the point of passing InsertIndexTuples IndexFunc to them.  Now, we're
>> always passing ExecInsertIndexTuples() to this argument.  As I
>> understood storage is free to either call ExecInsertIndexTuples() or
>> implement its own logic of interaction with indexes.  But, I don't
>> understand why do we need a callback when tuple_insert() and tuple_update()
>> can call ExecInsertIndexTuples() directly if needed.  Another thing is that
>> tuple_delete() could also interact with indexes (especially when we will
>> enhance index access method API), and we need to pass meta-information
>> about indexes to tuple_delete() too.
>>
>
> The main reason for which I added the callback function to not to
> introduce the
> dependency of storage on executor functions. This way storage can call the
> function that is passed to it without any knowledge. I added the function
> pointer
> for tuple_delete also in the new patches, currently it is passed as NULL
> for heap.
> These API's can be enhanced later.
>

Understood, but in order to implement alternative behavior with indexes
(for example,
insert index tuples to only some of indexes), storage am will still have to
call executor
functions.  So, yes this needs to be enhanced.  Probably, we just need to
implement
nicer executor API for storage am.


> Apart from rebase, Added storage shared memory API, currently this API is
> used
> only by the syncscan. And also all the exposed functions of syncscan usage
> is
> removed outside the heap.
>

This makes me uneasy.  You introduce two new hooks for size estimation and
initialization
of shared memory needed by storage am's.  But if storage am is implemented
in shared library,
then this shared library can use our generic method for allocation of
shared memory
(including memory needed by storage am).  If storage am is builtin, then
hooks are also not
needed, because we know all our builtin storage am's in advance.  For me,
it would be
nice to encapsulate heap am requirements in shared memory into functions
like
HeapAmShmemSize() and HeapAmShmemInit(), and don't explicitly show outside
that
this memory is needed for synchronized scan.  But separate hooks don't look
justified for me.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] Pluggable storage

[Haribabu Kommi]

On Thu, Jan 4, 2018 at 10:00 AM, Alexander Korotkov <
a.korot...@postgrespro.ru> wrote:

> On Wed, Jan 3, 2018 at 10:08 AM, Haribabu Kommi 
> wrote:
>
>>
>> On Wed, Dec 27, 2017 at 11:33 PM, Alexander Korotkov <
>> a.korot...@postgrespro.ru> wrote:
>>
>>>
>>> Also, I appreciate that now tuple_insert() and tuple_update() methods
>>> are responsible for inserting index tuples.  This unleash pluggable
>>> storages to implement another way of interaction with indexes.  However, I
>>> didn't get the point of passing InsertIndexTuples IndexFunc to them.  Now,
>>> we're always passing ExecInsertIndexTuples() to this argument.  As I
>>> understood storage is free to either call ExecInsertIndexTuples() or
>>> implement its own logic of interaction with indexes.  But, I don't
>>> understand why do we need a callback when tuple_insert() and tuple_update()
>>> can call ExecInsertIndexTuples() directly if needed.  Another thing is that
>>> tuple_delete() could also interact with indexes (especially when we will
>>> enhance index access method API), and we need to pass meta-information
>>> about indexes to tuple_delete() too.
>>>
>>
>> The main reason for which I added the callback function to not to
>> introduce the
>> dependency of storage on executor functions. This way storage can call the
>> function that is passed to it without any knowledge. I added the function
>> pointer
>> for tuple_delete also in the new patches, currently it is passed as NULL
>> for heap.
>> These API's can be enhanced later.
>>
>
> Understood, but in order to implement alternative behavior with indexes
> (for example,
> insert index tuples to only some of indexes), storage am will still have
> to call executor
> functions.  So, yes this needs to be enhanced.  Probably, we just need to
> implement
> nicer executor API for storage am.
>

OK.


> Apart from rebase, Added storage shared memory API, currently this API is
>> used
>> only by the syncscan. And also all the exposed functions of syncscan
>> usage is
>> removed outside the heap.
>>
>
> This makes me uneasy.  You introduce two new hooks for size estimation and
> initialization
> of shared memory needed by storage am's.  But if storage am is implemented
> in shared library,
> then this shared library can use our generic method for allocation of
> shared memory
> (including memory needed by storage am).  If storage am is builtin, then
> hooks are also not
> needed, because we know all our builtin storage am's in advance.  For me,
> it would be
> nice to encapsulate heap am requirements in shared memory into functions
> like
> HeapAmShmemSize() and HeapAmShmemInit(), and don't explicitly show outside
> that
> this memory is needed for synchronized scan.  But separate hooks don't
> look justified for me.
>

Yes, I agree that for the builtin storage's there is no need of hooks. But
in future,
if we want to support multiple storage's in an instance, we may need hooks
for shared memory
registration. I am fine to change it.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] Pluggable storage

[Alexander Korotkov]

On Thu, Jan 4, 2018 at 8:03 AM, Haribabu Kommi 
wrote:

> On Thu, Jan 4, 2018 at 10:00 AM, Alexander Korotkov <
> a.korot...@postgrespro.ru> wrote:
>
>> On Wed, Jan 3, 2018 at 10:08 AM, Haribabu Kommi > > wrote:
>>
>>> Apart from rebase, Added storage shared memory API, currently this API
>>> is used
>>>
>> only by the syncscan. And also all the exposed functions of syncscan
>>> usage is
>>> removed outside the heap.
>>>
>>
>> This makes me uneasy.  You introduce two new hooks for size estimation
>> and initialization
>> of shared memory needed by storage am's.  But if storage am is
>> implemented in shared library,
>> then this shared library can use our generic method for allocation of
>> shared memory
>> (including memory needed by storage am).  If storage am is builtin, then
>> hooks are also not
>> needed, because we know all our builtin storage am's in advance.  For me,
>> it would be
>> nice to encapsulate heap am requirements in shared memory into functions
>> like
>> HeapAmShmemSize() and HeapAmShmemInit(), and don't explicitly show
>> outside that
>> this memory is needed for synchronized scan.  But separate hooks don't
>> look justified for me.
>>
>
> Yes, I agree that for the builtin storage's there is no need of hooks. But
> in future,
> if we want to support multiple storage's in an instance, we may need hooks
> for shared memory
> registration. I am fine to change it.
>

Yes, but we already have hooks for shared memory registration in shared
modules.  I don't see the point for another hooks for the same purpose.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] Pluggable storage

[Haribabu Kommi]

On Fri, Jan 5, 2018 at 9:55 AM, Alexander Korotkov <
a.korot...@postgrespro.ru> wrote:

> On Thu, Jan 4, 2018 at 8:03 AM, Haribabu Kommi 
> wrote:
>
>> On Thu, Jan 4, 2018 at 10:00 AM, Alexander Korotkov <
>> a.korot...@postgrespro.ru> wrote:
>>
>>> On Wed, Jan 3, 2018 at 10:08 AM, Haribabu Kommi <
>>> kommi.harib...@gmail.com> wrote:
>>>
 Apart from rebase, Added storage shared memory API, currently this API
 is used

>>> only by the syncscan. And also all the exposed functions of syncscan
 usage is
 removed outside the heap.

>>>
>>> This makes me uneasy.  You introduce two new hooks for size estimation
>>> and initialization
>>> of shared memory needed by storage am's.  But if storage am is
>>> implemented in shared library,
>>> then this shared library can use our generic method for allocation of
>>> shared memory
>>> (including memory needed by storage am).  If storage am is builtin, then
>>> hooks are also not
>>> needed, because we know all our builtin storage am's in advance.  For
>>> me, it would be
>>> nice to encapsulate heap am requirements in shared memory into functions
>>> like
>>> HeapAmShmemSize() and HeapAmShmemInit(), and don't explicitly show
>>> outside that
>>> this memory is needed for synchronized scan.  But separate hooks don't
>>> look justified for me.
>>>
>>
>> Yes, I agree that for the builtin storage's there is no need of hooks.
>> But in future,
>> if we want to support multiple storage's in an instance, we may need
>> hooks for shared memory
>> registration. I am fine to change it.
>>
>
> Yes, but we already have hooks for shared memory registration in shared
> modules.  I don't see the point for another hooks for the same purpose.
>

Oh, yes, I missed it. I will update the patch and share it later.


Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] Pluggable storage

[Robert Haas]

I do not like the way that this patch set uses the word "storage".  In
current usage, storage is a thing that certain kinds of relations
have.  Plain relations (a.k.a. heap tables) have storage, indexes have
storage, materialized views have storage, TOAST tables have storage,
and sequences have storage.  This patch set wants to use "storage AM"
to mean a replacement for a plain heap table, but I think that's going
to create a lot of confusion, because it overlaps heavily with the
existing meaning yet is different.  My suggestion is to call these
"table access methods" rather than "storage access methods".  Then,
the default table AM can be heap.  This has the nice property that you
create an index using CREATE INDEX and the support functions arrive
via an IndexAmRoutine, so correspondingly you would create a table
using CREATE TABLE and the support functions would arrive via a
TableAmRoutine -- so all the names match.

An alternative would be to call the new thing a "heap AM" with
HeapAmRoutine as the support function structure.  That's also not
unreasonable.  In that case, we're deciding that "heap" is not just
the name of the current implementation, but the name of the kind of
thing that backs a table at the storage level.  I don't like that
quite as well because then we've got a class of things called a heap
of which the current and only implementation is called heap, which is
a bit confusing in my view.  But we could probably make it work.

If we adopt the first proposal, it leads to another nice parallel: we
can have src/backend/access/table for those things which are generic
to table AMs, just as we have src/backend/access/index for things
which are generic to index AMs, and then src/backend/access/
for things which are specific to a particular AM.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] Pluggable storage

[Alexander Korotkov]

On Fri, Jan 5, 2018 at 7:20 PM, Robert Haas  wrote:

> I do not like the way that this patch set uses the word "storage".  In
> current usage, storage is a thing that certain kinds of relations
> have.  Plain relations (a.k.a. heap tables) have storage, indexes have
> storage, materialized views have storage, TOAST tables have storage,
> and sequences have storage.  This patch set wants to use "storage AM"
> to mean a replacement for a plain heap table, but I think that's going
> to create a lot of confusion, because it overlaps heavily with the
> existing meaning yet is different.


Good point, thank you for noticing that.  Name "storage" is really confusing
for this purpose.


> My suggestion is to call these
> "table access methods" rather than "storage access methods".  Then,
> the default table AM can be heap.  This has the nice property that you
> create an index using CREATE INDEX and the support functions arrive
> via an IndexAmRoutine, so correspondingly you would create a table
> using CREATE TABLE and the support functions would arrive via a
> TableAmRoutine -- so all the names match.
>
> An alternative would be to call the new thing a "heap AM" with
> HeapAmRoutine as the support function structure.  That's also not
> unreasonable.  In that case, we're deciding that "heap" is not just
> the name of the current implementation, but the name of the kind of
> thing that backs a table at the storage level.  I don't like that
> quite as well because then we've got a class of things called a heap
> of which the current and only implementation is called heap, which is
> a bit confusing in my view.  But we could probably make it work.
>
> If we adopt the first proposal, it leads to another nice parallel: we
> can have src/backend/access/table for those things which are generic
> to table AMs, just as we have src/backend/access/index for things
> which are generic to index AMs, and then src/backend/access/
> for things which are specific to a particular AM.


I would vote for the first proposal: table AM.  Because we eventually
might get index-organized tables whose don't have something like heap.
So, it would be nice to avoid hardcoding "heap" name.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] Pluggable storage

[Haribabu Kommi]

On Tue, Jan 9, 2018 at 11:42 PM, Haribabu Kommi 
wrote:

>
> Updated patches are attached.
>

To integrate the columnar store with the pluggable storage API, I found that
there are couple of other things also that needs to be supported.

1. Choosing the right table access method for a particular table?

I am thinking of adding a new table option to let the user select the
correct table
access method that the user wants for the table. HEAP is the default access
method. This approach may be simple and doesn't need any syntax changes.

Or Is it fine to add syntax "USING method" to CREATE TABLE similar like
CREATE INDEX?

comments?

2. As the columnar storage needs many other relations that are needs to be
created along with main relation.

As these extra relations are internal to the storage and shouldn't be
visible
directly from pg_class and these will be stored in the storage specific
catalog tables. A dependency is created for the original table as these
storage
specific tables must be created/dropped/altered whenever there is a change
with the original table.

Is it fine to add new API while creating/altering/drop the table to get the
control?
or to use only exiting processutility hook?


Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] Pluggable storage

[Alexander Korotkov]

Hi, Haribabu!

On Mon, Feb 5, 2018 at 2:22 PM, Haribabu Kommi 
wrote:

>
> On Tue, Jan 9, 2018 at 11:42 PM, Haribabu Kommi 
> wrote:
>
>>
>> Updated patches are attached.
>>
>
> To integrate the columnar store with the pluggable storage API, I found
> that
> there are couple of other things also that needs to be supported.
>
> 1. Choosing the right table access method for a particular table?
>
> I am thinking of adding a new table option to let the user select the
> correct table
> access method that the user wants for the table. HEAP is the default access
> method. This approach may be simple and doesn't need any syntax changes.
>
> Or Is it fine to add syntax "USING method" to CREATE TABLE similar like
> CREATE INDEX?
>
> comments?
>

Sure, user needs to specify particular table access method when creating a
table.  "USING method" is good for me.  I think it's better to reuse
already existing syntax rather than reinventing something new.


> 2. As the columnar storage needs many other relations that are needs to be
> created along with main relation.
>

That's an interesting point.  During experimenting on some new index access
methods I also have to define some kind of "internal" relations invisible
for user, but essential for main relation functionality.  I've made them in
hackery manner, and I think legal mechanism for them would be very good.


> As these extra relations are internal to the storage and shouldn't be
> visible
> directly from pg_class and these will be stored in the storage specific
> catalog tables. A dependency is created for the original table as these
> storage
> specific tables must be created/dropped/altered whenever there is a change
> with the original table.
>

I think that internal relations should be visible from pg_class, otherwise
it wouldn't be possible to define dependencies over them.  But they should
have different relkind, so they wouldn't be messed up with regular
relations.

Is it fine to add new API while creating/altering/drop the table to get the
> control?
> or to use only exiting processutility hook?
>

I believe that we need some generic way for defining internal relations,
not hooks.  For me it seems like useful feature for further development of
both index access methods and table access methods.

During developer meeting [1], I've proposed to reorder patches so that
refactoring patches go first and API introduction patches go afterwards.
That would make possible to commit some of refactoring patches earlier
without necessary committing API in the same release cycle.  If no
objection, I would do that reordering.

BTW, EnterpriseDB announces zheap table access method (heap with undo log)
[2].  I think this is great, and I'm looking forward for publishing zheap
in mailing lists.  But I'm concerning about its compatibility with
pluggable table access methods API.  Does zheap use table AM API from this
thread?  Or does it just override current heap and needs to be adopted to
use table AM API?  Or does it implements own API?

*Links*

1. https://wiki.postgresql.org/wiki/FOSDEM/PGDay_2018_
Developer_Meeting#Minutes
2. http://rhaas.blogspot.com.by/2018/01/do-or-undo-there-is-no-vacuum.html

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] Pluggable storage

[Michael Paquier]

On Wed, Mar 28, 2018 at 12:23:56PM -0400, David Steele wrote:
> I think this entry should be moved the the next CF.  I'll do that
> tomorrow unless there are objections.

Instead of moving things to the next CF by default, perhaps it would
make more sense to mark things as reviewed with feedback as this is the
last CF?  There is a 5-month gap between this commit fest and the next
one, I am getting afraid of flooding the beginning of v12 development
cycle with entries which keep rotting over time.  If the author(s) claim
that they will be able to work on it, then that's of course fine.

Sorry for the digression, patches ignored across CFs contribute to the
bloat we see, and those eat the time of the CFM.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] Pluggable storage

[David Steele]

On 3/28/18 8:39 PM, Michael Paquier wrote:
> On Wed, Mar 28, 2018 at 12:23:56PM -0400, David Steele wrote:
>> I think this entry should be moved the the next CF.  I'll do that
>> tomorrow unless there are objections.
> 
> Instead of moving things to the next CF by default, perhaps it would
> make more sense to mark things as reviewed with feedback as this is the
> last CF?  There is a 5-month gap between this commit fest and the next
> one, I am getting afraid of flooding the beginning of v12 development
> cycle with entries which keep rotting over time.  If the author(s) claim
> that they will be able to work on it, then that's of course fine.

I agree and I do my best to return patches that have stalled, but I
don't think this patch is in that category.  It has gotten review and
has been kept up to date.  I don't think it's a good fit for v11 but I'd
like to see it in the first CF for v12.

> Sorry for the digression, patches ignored across CFs contribute to the
> bloat we see, and those eat the time of the CFM.

There's no question that bloat has become a problem.  I don't have all
the answers, but vigilance by the CFMs is certainly a good start.

Regards,
-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Pluggable storage

[Amit Kapila]

On Thu, Jun 14, 2018 at 1:50 AM, Haribabu Kommi
 wrote:
>
> On Fri, Apr 20, 2018 at 4:44 PM Haribabu Kommi 
> wrote:
>
> VACUUM:
> Not much changes are done in this apart moving the Vacuum visibility
> functions as part of the
> storage. But idea for the Vacuum was with each access method can define how
> it should perform.
>

We are planning to have a somewhat different mechanism for vacuum (for
non-delete marked indexes), so if you can provide some details or
discuss what you have in mind before implementation of same, that
would be great.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Pluggable storage

[Haribabu Kommi]

On Thu, Jun 14, 2018 at 12:25 PM Amit Kapila 
wrote:

> On Thu, Jun 14, 2018 at 1:50 AM, Haribabu Kommi
>  wrote:
> >
> > On Fri, Apr 20, 2018 at 4:44 PM Haribabu Kommi  >
> > wrote:
> >
> > VACUUM:
> > Not much changes are done in this apart moving the Vacuum visibility
> > functions as part of the
> > storage. But idea for the Vacuum was with each access method can define
> how
> > it should perform.
> >
>
> We are planning to have a somewhat different mechanism for vacuum (for
> non-delete marked indexes), so if you can provide some details or
> discuss what you have in mind before implementation of same, that
> would be great.
>

OK. Thanks for your input. We will discuss the changes before proceed to
code.

Apart from the this, the pluggable storage API contains some re-factored
changes
along with API, some of the re-factored changes are

1. Change the snapshot satisfies type from function to an enum
2. Try to return always the palloced tuple instead of a pointer to buffer
   (This change may have performance impact,so can be done later).
3. Perform a tuple visibility check at heap itself for the page mode
scenario also
4. New function ExecSlotCompare to compare two slots or tuple by storing it
a temp slot.
5. heap_fetch and heap_lock_tuple returns the palloced tuple, not the
pointer to the buffer
6. The index insertion logic decision is moved into heap itself(insert,
update), not in executor.
7. Split HeapscanDesc into two and remove it's usage outside heap of the
second split
8. Move the tuple traversing and providing the updated record to heap.

Is it fine to create these changes as separate patches and can go if the
changes are fine
and doesn't have any impact?

Any comments or additions or deletions to the above list?

Regards,
Haribabu Kommi
Fujitsu Australia

Re: [HACKERS] Pluggable storage

[Michael Paquier]

On Tue, Nov 7, 2017 at 6:34 PM, Haribabu Kommi  wrote:
> On Tue, Oct 31, 2017 at 8:59 PM, Haribabu Kommi 
> wrote:
>> Additional changes that are done in the patches compared to earlier
>> patches apart from rebase.
>
> Rebased patches are attached.

This set of patches needs again a... Rebase.
-- 
Michael

Re: [HACKERS] Pluggable storage

[Alvaro Herrera]

Hmm.  Am I reading it right that this discussion led to moving
essentially all code from tqual.c to heapam?  Given the hard time we've
had to get tqual.c right, it seems fundamentally misguided to me to
require that every single storage AM reimplements all the visibility
routines.

I think that changing tqual's API (such as not passing HeapTuples
anymore but some other more general representation) would be okay and
should be sufficient, but this wholesale movement of code seems
dangerous and wasteful in terms of future reimplementations that will be
necessary.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Pluggable storage

[Amit Kapila]

On Tue, Nov 14, 2017 at 4:12 PM, Alvaro Herrera  wrote:
> Hmm.  Am I reading it right that this discussion led to moving
> essentially all code from tqual.c to heapam?  Given the hard time we've
> had to get tqual.c right, it seems fundamentally misguided to me to
> require that every single storage AM reimplements all the visibility
> routines.
>
> I think that changing tqual's API (such as not passing HeapTuples
> anymore but some other more general representation) would be okay and
> should be sufficient, but this wholesale movement of code seems
> dangerous and wasteful in terms of future reimplementations that will be
> necessary.
>

I don't think the idea is to touch existing tqual.c in any significant
way.  However, some other storage engine might need a different way to
check the visibility of tuples so we need provision for that. I think
for storage engine where tuple headers no longer contain transaction
information and or the old versions of tuples are chained in separate
storage (say undo storage), current visibility routines can be used.
I think the current tqual.c is quite tightly coupled with the
HeapTuple representation, so changing that or adding more code to it
for another storage engine with different tuple format won't be of
much use.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Pluggable storage

[Michael Paquier]

On Tue, Nov 14, 2017 at 5:09 PM, Michael Paquier
 wrote:
> On Tue, Nov 7, 2017 at 6:34 PM, Haribabu Kommi  
> wrote:
>> On Tue, Oct 31, 2017 at 8:59 PM, Haribabu Kommi 
>> wrote:
>>> Additional changes that are done in the patches compared to earlier
>>> patches apart from rebase.
>>
>> Rebased patches are attached.
>
> This set of patches needs again a... Rebase.

No rebased versions have showed up for two weeks. For now I am marking
this patch as returned with feedback.
-- 
Michael

Re: [HACKERS] Pluggable storage

[Alexander Korotkov]

Hi!

On Wed, Dec 27, 2017 at 6:54 AM, Haribabu Kommi 
wrote:

>
> On Tue, Dec 12, 2017 at 3:06 PM, Haribabu Kommi 
> wrote:
>
>>
>> I restructured that patch files to avoid showing unnecessary
>> modifications,
>> and also it will be easy for adding of new API's based on the all the
>> functions
>> that are exposed by heapam module easily compared earlier.
>>
>> Attached are the latest set of patches. I will work on the remaining
>> pending
>> items.
>>
>
> Apart from rebase to the latest master code, following are the additional
> changes,
>
> 1. Added API for bulk insert and rewrite functionality(Logical rewrite is
> not touched yet)
> 2. Tuple lock API interface redesign to remove the traversal logic from
> executor module.
>

Great, thank you!


> The tuple lock API interface changes are from "Alexander Korotkov" from
> "PostgresPro".
> Thanks Alexander. Currently we both are doing joint development for faster
> closure of
> open items that are pending to bring the "pluggable storage API" into a
> good shape.
>

Thank you for announcing this.  Yes, pluggable storage API requires a lot
of work to get into committable shape.  This is why I've decided to join
the development.

Let me explain the idea behind new tuple lock API and further patches I
plan to send.  As I noted upthread, I consider possibility of alternative
MVCC implementations as vital property of pluggable storage API.  These
include undo log option when tuple is updated in-place while old version of
tuple is displaced to some special area.  In this case, new version of
tuple would reside on same TID as old version of tuple.  This is an
important point, because TID is not really tuple identifier anymore.
Naturally, TID becomes a row identifier while tuple may be identified by
pair (tid, snapshot).  For current heap, snapshot is redundant and can be
used just for assert checking (tuple on given tid is really visible using
given snapshot).  For heap with undo log, appropriate tuple could be found
by snapshot in the undo chain associated with given tid.

One of consequences of above is that we cannot use fact that tid isn't
changed after update as sign that tuple was deleted.  This is why I've
introduced HTSU_Result  HeapTupleDeleted.  Another consequence is that our
tid traverse logic in the executor layer is not valid anymore.  For
instance, this traversal from older tuple to latter tuple doesn't make any
sense for heap with undo log where latter tuple is more easily accessible
than older tuple.  This is why I decided to hide this logic in storage
layer and provide TUPLE_LOCK_FLAG_FIND_LAST_VERSION flag which indicates
that lock_tuple() have to find latest updated version and lock it.  I've
also changed follow_updates bool to more explicit
TUPLE_LOCK_FLAG_LOCK_UPDATE_IN_PROGRESS flag in order to not mess it with
previous flag which also kind of follow updates.  Third consequence is that
we have to pass snapshot to tuple_update() and tuple_delete() methods to
let them check if row was concurrently updated while residing on the same
TID.  I'm going to provide this change as separate patch.

Also, I appreciate that now tuple_insert() and tuple_update() methods are
responsible for inserting index tuples.  This unleash pluggable storages to
implement another way of interaction with indexes.  However, I didn't get
the point of passing InsertIndexTuples IndexFunc to them.  Now, we're
always passing ExecInsertIndexTuples() to this argument.  As I understood
storage is free to either call ExecInsertIndexTuples() or implement its own
logic of interaction with indexes.  But, I don't understand why do we need
a callback when tuple_insert() and tuple_update() can call
ExecInsertIndexTuples() directly if needed.  Another thing is that
tuple_delete() could also interact with indexes (especially when we will
enhance index access method API), and we need to pass meta-information
about indexes to tuple_delete() too.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company