Re: [HACKERS] WIP: Access method extendability

[Emre Hasegeli]

I think the variable "magick" should be "magic".  Patch attached.


bloom-magick.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Aleksander Alekseev]

Hello

> Fixed.

Thanks. I don't have any other suggestions at the moment. Let see whats
committers opinion on this.

-- 
Best regards,
Aleksander Alekseev
http://eax.me/


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Alexander Korotkov]

Hi!

On Fri, Apr 1, 2016 at 12:45 PM, Aleksander Alekseev <
a.aleks...@postgrespro.ru> wrote:

> Code looks much better now, thanks. Still I believe it could be improved.
>
> I don't think that using srand() / rand() in signValue procedure the
> way you did is such a good idea. You create a side affect (changing
> current randseed) which could cause problems in some cases. And there
> is no real need for that. For instance you could use following formula
> instead:
>
> hash(attno || hashVal || j)
>

I've discussed this with Teodor privately.  Extra hash calculation could
cause performance regression.  He proposed to use own random generator
instead.  Implemented in attached version of patch.


> And a few more things.
>
> > + memset(opaque, 0, sizeof(BloomPageOpaqueData));
> > + opaque->maxoff = 0;
>


> This looks a bit redundant.
>

Fixed.


> > + for (my $i = 1; $i <= 10; $i++)
>
> More idiomatic Perl would be `for my $i (1..10)`.
>

Fixed.


> > + UnlockReleaseBuffer(buffer);
> > + ReleaseBuffer(metaBuffer);
> > + goto away;
>
> In general I don't have anything against goto. But are you sure that
> using it here is really justified?


Fixed with small code duplication which seems to be better than goto.

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


0003-bloom-contrib.16.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Aleksander Alekseev]

Hello, Alexander

> Hi!
> 
> New revision of patches is attached.

Code looks much better now, thanks. Still I believe it could be improved.

I don't think that using srand() / rand() in signValue procedure the
way you did is such a good idea. You create a side affect (changing
current randseed) which could cause problems in some cases. And there
is no real need for that. For instance you could use following formula
instead:

hash(attno || hashVal || j)

And a few more things.

> + memset(opaque, 0, sizeof(BloomPageOpaqueData));
> + opaque->maxoff = 0;

This looks a bit redundant.

> + for (my $i = 1; $i <= 10; $i++)

More idiomatic Perl would be `for my $i (1..10)`.

> + UnlockReleaseBuffer(buffer);
> + ReleaseBuffer(metaBuffer);
> + goto away;

In general I don't have anything against goto. But are you sure that
using it here is really justified?

-- 
Best regards,
Aleksander Alekseev
http://eax.me/


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Markus Nullmeier]

On 03/31/16 17:29, Alexander Korotkov wrote:

> New revision of patches is attached.

> 1) API of generic xlog was changed: now there is no static variables,
>GenericXLogStart() returns palloc'd struct.

Attached are two trivial documentation editing fixes for this, as an
incremental patch.

-- 
Markus Nullmeierhttp://www.g-vo.org
German Astrophysical Virtual Observatory (GAVO)

diff --git a/doc/src/sgml/generic-wal.sgml b/doc/src/sgml/generic-wal.sgml
index 6655f22..b3388ba 100644
--- a/doc/src/sgml/generic-wal.sgml
+++ b/doc/src/sgml/generic-wal.sgml
@@ -43,7 +43,7 @@
 
 
  
-  GenericXLogAbort(state)  finish construction of
+  GenericXLogFinish(state)  finish construction of
   a generic xlog record.
  
 
@@ -52,7 +52,7 @@
 
   
The xlog record construction can be canceled between any of the above
-   steps by calling GenericXLogAbort().  This will discard all
+   steps by calling GenericXLogAbort(state).  This will discard all
changes to the page image copies.
   
 

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Alexander Korotkov]

Hi!

New revision of patches is attached.

Changes are following:
1) API of generic xlog was changed: now there is no static variables,
GenericXLogStart()
returns palloc'd struct.
2) Generic xlog use elog instead ereport since it reports internal errors
which shouldn't happen normally.
3) Error messages doesn't contains name of the function.
4) Bloom contrib was pgindented.
5) More comments for bloomb.
6) One more assert was added to bloom.
7) makeDefaultBloomOptions was renamed to adjustBloomOptions.  Now it only
modifies its parameter. palloc is done outside of this function.
8) BloomBuildState is explicitly zeroed.

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


0002-generic-xlog.15.patch
Description: Binary data


0003-bloom-contrib.15.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Markus Nullmeier]

Alexander Korotkov  wrote:
> I heard no objections.  There is revision of patch where generic WAL
> interface description was moved to documentation.  This description
> contains improvements by Petr Jelinek, Alvaro Herrera and Markus Nullmeier

Attached are a few more small fixes as an incremental patch (typos / etc.).

-- 
Markus Nullmeierhttp://www.g-vo.org
German Astrophysical Virtual Observatory (GAVO)

diff --git a/doc/src/sgml/generic-wal.sgml b/doc/src/sgml/generic-wal.sgml
index a00c03c..d756e33 100644
--- a/doc/src/sgml/generic-wal.sgml
+++ b/doc/src/sgml/generic-wal.sgml
@@ -4,15 +4,15 @@
  Generic WAL records
 
   
-   Despite all built in access methods and WAL-logged modules have their own
-   types of WAL records, there is also generic WAL record type which describes
+   Despite all built-in access methods and WAL-logged modules having their own
+   types of WAL records, there is also a generic WAL record type, which describes
changes to pages in a generic way.  This is useful for extensions that
provide custom access methods, because they cannot register their own
WAL redo routines.
   
 
   
-   API for contructing generic WAL records is defined in
+   The API for contructing generic WAL records is defined in
generic_xlog.h and implemented in generic_xlog.c.
Each generic WAL record must be constructed by following these steps:
 
diff --git a/src/backend/access/transam/generic_xlog.c b/src/backend/access/transam/generic_xlog.c
index 32c2648..1a720fa 100644
--- a/src/backend/access/transam/generic_xlog.c
+++ b/src/backend/access/transam/generic_xlog.c
@@ -19,7 +19,7 @@
 #include "utils/memutils.h"
 
 /*-
- * Internally, a delta between pages consists of set of fragments.  Each
+ * Internally, a delta between pages consists of a set of fragments.  Each
  * fragment represents changes made in a given region of a page.  A fragment
  * is made up as follows:
  *
@@ -29,7 +29,7 @@
  *
  * Unchanged regions of a page are not represented in its delta.  As a
  * result, a delta can be more compact than the full page image.  But having
- * an unchanged region in the middle to two fragments that is smaller than
+ * an unchanged region in the middle of two fragments that is smaller than
  * the fragment header (offset and length) does not pay off in terms of the
  * overall size of the delta. For this reason, we break fragments only if
  * the unchanged region is bigger than MATCH_THRESHOLD.
@@ -422,7 +422,7 @@ generic_redo(XLogReaderState *record)
 
 	Assert(record->max_block_id < MAX_GENERIC_XLOG_PAGES);
 
-	/* Interate over blocks */
+	/* Iterate over blocks */
 	for (block_id = 0; block_id <= record->max_block_id; block_id++)
 	{
 		XLogRedoAction action;

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Aleksander Alekseev]

> > http://www.postgresql.org/message-id/56eff347.20...@anastigmatix.net
> That discussion is about SQL-level types which could be stored on
> disk, not about in-memory structs

I must respectfully disagree. That discussion is also about memory
sanitizers and using them on buildfarms. Lets say you initialize a
structure like this:

st->f1 = 111;
st->f2 = 222;

... without using memset, so there could be a "hole" with uninitialized
data somewhere in between of f1 and f2.

Than some code calculates a hash of this structure or does memcpy - and
1) You get unreproducible behavior - hash is always different for the
same structure, thus it is stored in different hash buckets, etc, and as
a result you got bugs that sometimes reproduce and sometimes do not
2) There is one more place where sanitizers could report accesses to
uninitialized values and thus they still can't be used on buildfarms
where they could find a lot of serious bugs automatically. I believe
MemorySanitizer is smart enough to recognize trivial memcpy case, but
it could be confused in more complicated cases.

Anyway I suggest continue discussion of whether we should make
PostgreSQL sanitizers-friendly or not in a corresponding thread. So far
no one spoke against this idea. Thus I don't think that new patches
should complicate implementing it. Especially considering that it's
very simple to do and even is considered a good practice according to
PostgreSQL documentation.

-- 
Best regards,
Aleksander Alekseev
http://eax.me/


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Teodor Sigaev]

as we discussed recently [1] you should avoid leaving "holes" with
uninitialized data in structures. Please fix this or provide a comment
that describes why things are done here the way they are done.
[1] http://www.postgresql.org/message-id/56eff347.20...@anastigmatix.net
That discussion is about SQL-level types which could be stored on disk, not 
about in-memory structs



--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Teodor Sigaev]

GenericXLogStart(Relation relation)
{
...
if (genericXlogStatus != GXLOG_NOT_STARTED)
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
 errmsg("GenericXLogStart: generic xlog is already started")));


Hmm, seems, generic wal whiil be in incorrect state if exception occurs between 
GenericXLogStart() and GenericXLogFinish() calls because static variable 
genericXlogStatus will contain GXLOG_LOGGED/GXLOG_UNLOGGED status.


Suppose, it could be solved by different ways
- remove all static variable, so, GenericXLogStart() will return an struct
  (object) which incapsulated all data needed to generic wal work. As I can
  see, in case of exception there isn't ane needing to extra cleanup. Also,
  it would allow to use generic wal for two or more relations at the same time,
  although I don't know any useful example for such feature.
- add callback via RegisterResourceReleaseCallback() which will cleanup state
  of genericXlogStatus variable

--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Jose Luis Tallon]

Referenced by commit commit 473b93287040b20017cc25a157cffdc5b978c254 ("Support 
CREATE ACCESS METHOD"), commited by alvherre

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Aleksander Alekseev]

Hello

I did a brief review of bloom contrib and I don't think I like it much.
Here are some issues I believe should be fixed before committing it to
PostgreSQL.

1) Most of the code is not commented. Every procedure should at least
have a breif description of what it does, what arguments it receives
and what it returns. Same for structures and enums.

2) There are only 3 Asserts. For sure there are much more invariants in
this contrib.

3) I don't like this code (blinsert.c):

```
typedef struct
{
BloomState  blstate;
MemoryContext   tmpCtx;
chardata[BLCKSZ];
int64   count;
} BloomBuildState;

/* ... */

/*
 * (Re)initialize cached page in BloomBuildState.
 */
static void
initCachedPage(BloomBuildState *buildstate)
{
memset(buildstate->data, 0, BLCKSZ);
BloomInitPage(buildstate->data, 0); 
buildstate->count = 0;
}
```

It looks wrong because 1) blkstate and tmpCtx are left uninitialized 2)
as we discussed recently [1] you should avoid leaving "holes" with
uninitialized data in structures. Please fix this or provide a comment
that describes why things are done here the way they are done.

Perhaps there are also other places like this that I missed.

4) I don't think I like such a code either:

```
/* blutuls.c */

static BloomOptions *
makeDefaultBloomOptions(BloomOptions *opts)
{
int i;

if (!opts)
opts = palloc0(sizeof(BloomOptions));

/* ... */

/* see also blvacuum.c and other places I could miss */
``` 

Sometimes we create a new zero-initialized structure and sometimes we
use a provided one filled with some data. If I'll use this procedure
multiple times in a loop memory will leak. Thus sometimes we need
pfree() returned value manually and sometimes we don't. Such a code is
hard to reason about. You could do it much simpler choosing only one
thing to do --- either allocate a new structure or use a provided one.

5) Code is not pgindent'ed and has many trailing spaces.

[1] http://www.postgresql.org/message-id/56eff347.20...@anastigmatix.net

-- 
Best regards,
Aleksander Alekseev
http://eax.me/


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Alexander Korotkov]

On Tue, Mar 29, 2016 at 8:34 PM, Alexander Korotkov <
a.korot...@postgrespro.ru> wrote:

> On Tue, Mar 29, 2016 at 8:29 PM, Teodor Sigaev  wrote:
>
>> I incorporated your changes and did some additional refinements on top of
>>> them
>>> still.
>>>
>>> Attached is delta against v12, that should cause less issues when
>>> merging for
>>> Teodor.
>>>
>>
>> But last version is 13th?
>>
>> BTW, it would be cool to add odcs in VII. Internals chapter, description
>> should be similar to index's interface description, i.e. how to use generic
>> WAL interface.
>
>
> We already have generic WAL interface description in comments to
> generic_xlog.c.  I'm not fan of duplicating things.  What about moving
> interface description from comments to docs completely?
>

I heard no objections.  There is revision of patch where generic WAL
interface description was moved to documentation.  This description
contains improvements by Petr Jelinek, Alvaro Herrera and Markus Nullmeier
(who sent it to me privately).

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


0002-generic-xlog.14.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Alexander Korotkov]

On Tue, Mar 29, 2016 at 8:29 PM, Teodor Sigaev  wrote:

> I incorporated your changes and did some additional refinements on top of
>> them
>> still.
>>
>> Attached is delta against v12, that should cause less issues when merging
>> for
>> Teodor.
>>
>
> But last version is 13th?
>
> BTW, it would be cool to add odcs in VII. Internals chapter, description
> should be similar to index's interface description, i.e. how to use generic
> WAL interface.


We already have generic WAL interface description in comments to
generic_xlog.c.  I'm not fan of duplicating things.  What about moving
interface description from comments to docs completely?

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

Re: [HACKERS] WIP: Access method extendability

[Petr Jelinek]

On 29/03/16 19:29, Teodor Sigaev wrote:

I incorporated your changes and did some additional refinements on top
of them
still.

Attached is delta against v12, that should cause less issues when
merging for
Teodor.


But last version is 13th?


No, 12 is last version from Alexander afaics, I named my merge 13, but 
somehow the diff was broken so now I just sent diff against Alexander's 
work with mine + Alvaro's changes, sorry for confusion.


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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Teodor Sigaev]

I incorporated your changes and did some additional refinements on top of them
still.

Attached is delta against v12, that should cause less issues when merging for
Teodor.


But last version is 13th?

BTW, it would be cool to add odcs in VII. Internals chapter, description should 
be similar to index's interface description, i.e. how to use generic WAL interface.



--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Petr Jelinek]

On 29/03/16 18:25, Alvaro Herrera wrote:

+ /*-
>+  * API for construction of generic xlog records
>+  *
>+  * This API allows user to construct generic xlog records which describe
>+  * difference between pages in a generic way.  This is useful for
>+  * extensions which provide custom access methods because they can't
>+  * register their own WAL redo routines.
>+  *
>+  * Each record must be constructed by following these steps:
>+  * 1) GenericXLogStart(relation) - start construction of a generic xlog
>+  *  record for the given relation.
>+  * 2) GenericXLogRegister(buffer, isNew) - register one or more buffers
>+  *  for the record.  This function returns a copy of the page
>+  *  image where modifications can be performed.  The second argument
>+  *  indicates if the block is new (i.e. a full page image should be 
taken).
>+  * 3) Apply modification of page images obtained in the previous step.
>+  * 4) GenericXLogFinish() - finish construction of generic xlog record.
>+  *
>+  * The xlog record construction can be canceled at any step by calling
>+  * GenericXLogAbort().  All changes made to page images copies will be
>+  * discarded.
>+  *
>+  * Please, note the following points when constructing generic xlog records.
>+  * - No direct modifications of page images are allowed!  All modifications
>+  * must be done in the copies returned by GenericXLogRegister().  In 
other
>+  * words the code which makes generic xlog records must never call
>+  * BufferGetPage().
>+  * - Registrations of buffers (step 2) and modifications of page images
>+  * (step 3) can be mixed in any sequence.  The only restriction is 
that
>+  * you can only modify page image after registration of corresponding
>+  * buffer.
>+  * - After registration, the buffer also can be unregistered by calling
>+  * GenericXLogUnregister(buffer).  In this case the changes made in
>+  * that particular page image copy will be discarded.
>+  * - Generic xlog assumes that pages are using standard layout, i.e., all
>+  * data between pd_lower and pd_upper will be discarded.
>+  * - Maximum number of buffers simultaneously registered for a generic xlog
>+  * record is MAX_GENERIC_XLOG_PAGES.  An error will be thrown if 
this limit
>+  * is exceeded.
>+  * - Since you modify copies of page images, GenericXLogStart() doesn't
>+  * start a critical section.  Thus, you can do memory allocation, 
error
>+  * throwing etc between GenericXLogStart() and GenericXLogFinish().
>+  * The actual critical section is present inside GenericXLogFinish().
>+  * - GenericXLogFinish() takes care of marking buffers dirty and setting 
their
>+  * LSNs.  You don't need to do this explicitly.
>+  * - For unlogged relations, everything works the same except there is no
>+  * WAL record produced.  Thus, you typically don't need to do any 
explicit
>+  * checks for unlogged relations.
>+  * - If registered buffer isn't new, generic xlog record contains delta
>+  * between old and new page images.  This delta is produced by per 
byte
>+  * comparison.  This current delta mechanism is not effective for 
data shifts
>+  * inside the page and may be improved in the future.
>+  * - Generic xlog redo function will acquire exclusive locks on buffers
>+  * in the same order they were registered.  After redo of all 
changes,
>+  * the locks will be released in the same order.
>+  *
>+  *
>+  * Internally, delta between pages consists of set of fragments.  Each
>+  * fragment represents changes made in given region of page.  A fragment is
>+  * described as follows:
>+  *
>+  * - offset of page region (OffsetNumber)
>+  * - length of page region (OffsetNumber)
>+  * - data - the data to place into described region ('length' number of 
bytes)
>+  *
>+  * Unchanged regions of page are not represented in the delta.  As a result,
>+  * the delta can be more compact than full page image.  But if the unchanged 
region
>+  * of the page is less than fragment header (offset and length) the delta
>+  * would be bigger than the full page image. For this reason we break into 
fragments
>+  * only if the unchanged region is bigger than MATCH_THRESHOLD.
>+  *
>+  * The worst case for delta size is when we didn't find any unchanged region
>+  * in the page. Then size of delta would be size of page plus size of 
fragment
>+  * header.
>+  */
>+ #define FRAGMENT_HEADER_SIZE  (2 * sizeof(OffsetNumber))
>+ #define MATCH_THRESHOLD   FRAGMENT_HEADER_SIZE
>+ #define MAX_DELTA_SIZEBLCKSZ + FRAGMENT_HEADER_SIZE




I incorporated your changes and did some additional refinements on top 
of them still.


Attached is delta against v12, that should cause less issues when 
merging for 

Re: [HACKERS] WIP: Access method extendability

[Alvaro Herrera]

Petr Jelinek wrote:

> And here it is. It's not perfect but it's better (I am not native speaker
> either). It's same as v12, just changed comments somewhat.

I think this can still be improved a bit more, in particular this large
comment, which I edit inline for expediency.

> + /*-
> +  * API for construction of generic xlog records
> +  *
> +  * This API allows user to construct generic xlog records which describe
> +  * difference between pages in a generic way.  This is useful for
> +  * extensions which provide custom access methods because they can't
> +  * register their own WAL redo routines.
> +  *
> +  * Each record must be constructed by following these steps:
> +  * 1) GenericXLogStart(relation) - start construction of a generic xlog
> +  *record for the given relation.
> +  * 2) GenericXLogRegister(buffer, isNew) - register one or more buffers
> +  *for the record.  This function returns a copy of the page
> +  *image where modifications can be performed.  The second argument
> +  *indicates if the block is new (i.e. a full page image should be 
> taken).
> +  * 3) Apply modification of page images obtained in the previous step.
> +  * 4) GenericXLogFinish() - finish construction of generic xlog record.
> +  *
> +  * The xlog record construction can be canceled at any step by calling
> +  * GenericXLogAbort().  All changes made to page images copies will be
> +  * discarded.
> +  *
> +  * Please, note the following points when constructing generic xlog records.
> +  * - No direct modifications of page images are allowed!  All modifications
> +  *   must be done in the copies returned by GenericXLogRegister().  In other
> +  *   words the code which makes generic xlog records must never call
> +  *   BufferGetPage().
> +  * - Registrations of buffers (step 2) and modifications of page images
> +  *   (step 3) can be mixed in any sequence.  The only restriction is that
> +  *   you can only modify page image after registration of corresponding
> +  *   buffer.
> +  * - After registration, the buffer also can be unregistered by calling
> +  *   GenericXLogUnregister(buffer).  In this case the changes made in
> +  *   that particular page image copy will be discarded.
> +  * - Generic xlog assumes that pages are using standard layout, i.e., all
> +  *   data between pd_lower and pd_upper will be discarded.
> +  * - Maximum number of buffers simultaneously registered for a generic xlog
> +  *   record is MAX_GENERIC_XLOG_PAGES.  An error will be thrown if this 
> limit
> +  *   is exceeded.
> +  * - Since you modify copies of page images, GenericXLogStart() doesn't
> +  *   start a critical section.  Thus, you can do memory allocation, error
> +  *   throwing etc between GenericXLogStart() and GenericXLogFinish().
> +  *   The actual critical section is present inside GenericXLogFinish().
> +  * - GenericXLogFinish() takes care of marking buffers dirty and setting 
> their
> +  *   LSNs.  You don't need to do this explicitly.
> +  * - For unlogged relations, everything works the same except there is no
> +  *   WAL record produced.  Thus, you typically don't need to do any explicit
> +  *   checks for unlogged relations.
> +  * - If registered buffer isn't new, generic xlog record contains delta
> +  *   between old and new page images.  This delta is produced by per byte
> +  *   comparison.  This current delta mechanism is not effective for data 
> shifts
> +  *   inside the page and may be improved in the future.
> +  * - Generic xlog redo function will acquire exclusive locks on buffers
> +  *   in the same order they were registered.  After redo of all changes,
> +  *   the locks will be released in the same order.
> +  *
> +  *
> +  * Internally, delta between pages consists of set of fragments.  Each
> +  * fragment represents changes made in given region of page.  A fragment is
> +  * described as follows:
> +  *
> +  * - offset of page region (OffsetNumber)
> +  * - length of page region (OffsetNumber)
> +  * - data - the data to place into described region ('length' number of 
> bytes)
> +  *
> +  * Unchanged regions of page are not represented in the delta.  As a result,
> +  * the delta can be more compact than full page image.  But if the 
> unchanged region
> +  * of the page is less than fragment header (offset and length) the delta
> +  * would be bigger than the full page image. For this reason we break into 
> fragments
> +  * only if the unchanged region is bigger than MATCH_THRESHOLD.
> +  *
> +  * The worst case for delta size is when we didn't find any unchanged region
> +  * in the page. Then size of delta would be size of page plus size of 
> fragment
> +  * header.
> +  */
> + #define FRAGMENT_HEADER_SIZE(2 * sizeof(OffsetNumber))
> + #define MATCH_THRESHOLD FRAGMENT_HEADER_SIZE
> + #define MAX_DELTA_SIZE  BLCKSZ + FRAGMENT_HEADER_SIZE

Re: [HACKERS] WIP: Access method extendability

[Teodor Sigaev]

And here it is. It's not perfect but it's better (I am not native speaker
either). It's same as v12, just changed comments somewhat.


Thank you, but I have a problems with applying:

% patch -p1 < ~/Downloads/0002-generic-xlog.13.patch
Hmm...  Looks like a new-style context diff to me...
...
|diff --git a/src/backend/access/transam/generic_xlog.c 
b/src/backend/access/transam/generic_xlog.c

|new file mode 100644
|index ...7ca03bf
|*** a/src/backend/access/transam/generic_xlog.c
|--- b/src/backend/access/transam/generic_xlog.c
--
(Creating file src/backend/access/transam/generic_xlog.c...)
Patching file src/backend/access/transam/generic_xlog.c using Plan A...
patch:  malformed patch at line 634: diff --git 
a/src/backend/access/transam/rmgr.c b/src/backend/access/transam/rmgr.c


--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Petr Jelinek]

On 29/03/16 17:33, Teodor Sigaev wrote:

Does that mean this patch should be closed or is there more remaining
to commit?


Petr promises to check english in comments/docs in generic-wal patch at
this week, bloom patch depends on it. Bloom patch is ready from my point
of view.



And here it is. It's not perfect but it's better (I am not native 
speaker either). It's same as v12, just changed comments somewhat.


--
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/src/backend/access/rmgrdesc/Makefile 
b/src/backend/access/rmgrdesc/Makefile
new file mode 100644
index c72a1f2..c0e38fd
*** a/src/backend/access/rmgrdesc/Makefile
--- b/src/backend/access/rmgrdesc/Makefile
*** subdir = src/backend/access/rmgrdesc
*** 8,16 
  top_builddir = ../../../..
  include $(top_builddir)/src/Makefile.global
  
! OBJS = brindesc.o clogdesc.o committsdesc.o dbasedesc.o gindesc.o gistdesc.o \
!  hashdesc.o heapdesc.o mxactdesc.o nbtdesc.o relmapdesc.o \
!  replorigindesc.o seqdesc.o smgrdesc.o spgdesc.o \
   standbydesc.o tblspcdesc.o xactdesc.o xlogdesc.o
  
  include $(top_srcdir)/src/backend/common.mk
--- 8,16 
  top_builddir = ../../../..
  include $(top_builddir)/src/Makefile.global
  
! OBJS = brindesc.o clogdesc.o committsdesc.o dbasedesc.o genericdesc.o \
!  gindesc.o gistdesc.o hashdesc.o heapdesc.o mxactdesc.o nbtdesc.o \
!  relmapdesc.o replorigindesc.o seqdesc.o smgrdesc.o spgdesc.o \
   standbydesc.o tblspcdesc.o xactdesc.o xlogdesc.o
  
  include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/access/rmgrdesc/genericdesc.c 
b/src/backend/access/rmgrdesc/genericdesc.c
new file mode 100644
index ...3d035c2
*** a/src/backend/access/rmgrdesc/genericdesc.c
--- b/src/backend/access/rmgrdesc/genericdesc.c
***
*** 0 
--- 1,58 
+ /*-
+  *
+  * genericdesc.c
+  *  rmgr descriptor routines for access/transam/generic_xlog.c
+  *
+  *
+  * Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group
+  * Portions Copyright (c) 1994, Regents of the University of California
+  *
+  * src/backend/access/rmgrdesc/genericdesc.c
+  *
+  *-
+  */
+ #include "postgres.h"
+ 
+ #include "access/generic_xlog.h"
+ #include "lib/stringinfo.h"
+ #include "storage/relfilenode.h"
+ 
+ /*
+  * Description of generic xlog record: write page regions which this record
+  * overrides.
+  */
+ void
+ generic_desc(StringInfo buf, XLogReaderState *record)
+ {
+   Pointer ptr = XLogRecGetData(record),
+   end = ptr + XLogRecGetDataLen(record);
+ 
+   while (ptr < end)
+   {
+   OffsetNumberoffset,
+   length;
+ 
+   memcpy(, ptr, sizeof(offset));
+   ptr += sizeof(offset);
+   memcpy(, ptr, sizeof(length));
+   ptr += sizeof(length);
+   ptr += length;
+ 
+   if (ptr < end)
+   appendStringInfo(buf, "offset %u, length %u; ", offset, 
length);
+   else
+   appendStringInfo(buf, "offset %u, length %u", offset, 
length);
+   }
+ 
+   return;
+ }
+ 
+ /*
+  * Identification of generic xlog record: we don't distinguish any subtypes
+  * inside generic xlog records.
+  */
+ const char *
+ generic_identify(uint8 info)
+ {
+   return "Generic";
+ }
diff --git a/src/backend/access/transam/Makefile 
b/src/backend/access/transam/Makefile
new file mode 100644
index 94455b2..16fbe47
*** a/src/backend/access/transam/Makefile
--- b/src/backend/access/transam/Makefile
*** subdir = src/backend/access/transam
*** 12,19 
  top_builddir = ../../../..
  include $(top_builddir)/src/Makefile.global
  
! OBJS = clog.o commit_ts.o multixact.o parallel.o rmgr.o slru.o subtrans.o \
!   timeline.o transam.o twophase.o twophase_rmgr.o varsup.o \
xact.o xlog.o xlogarchive.o xlogfuncs.o \
xloginsert.o xlogreader.o xlogutils.o
  
--- 12,19 
  top_builddir = ../../../..
  include $(top_builddir)/src/Makefile.global
  
! OBJS = clog.o commit_ts.o generic_xlog.o multixact.o parallel.o rmgr.o slru.o 
\
!   subtrans.o timeline.o transam.o twophase.o twophase_rmgr.o varsup.o \
xact.o xlog.o xlogarchive.o xlogfuncs.o \
xloginsert.o xlogreader.o xlogutils.o
  
diff --git a/src/backend/access/transam/generic_xlog.c 
b/src/backend/access/transam/generic_xlog.c
new file mode 100644
index ...7ca03bf
*** a/src/backend/access/transam/generic_xlog.c
--- b/src/backend/access/transam/generic_xlog.c
***
*** 0 
--- 1,510 
+ /*-
+  *
+  * generic_xlog.c
+  * 

Re: [HACKERS] WIP: Access method extendability

[Teodor Sigaev]

Does that mean this patch should be closed or is there more remaining to commit?


Petr promises to check english in comments/docs in generic-wal patch at this 
week, bloom patch depends on it. Bloom patch is ready from my point of view.




--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Alexander Korotkov]

On Tue, Mar 29, 2016 at 6:20 PM, David Steele  wrote:

> On 3/28/16 1:26 PM, Alvaro Herrera wrote:
>
>> Alexander Korotkov wrote:
>>
>>> On Thu, Mar 24, 2016 at 6:19 PM, Alvaro Herrera <
>>> alvhe...@2ndquadrant.com>
>>> wrote:
>>>
>>> .. Oh crap.  I just noticed I forgot to update a comment in pg_dump's
 getAccessMethods.  And we're missing psql tab-complete support for the
 new commands.

>>>
>>> Attached patches fix both these issues.
>>>
>>
>> I see Teodor just pushed these two fixes.  Thanks!
>>
>
> Does that mean this patch should be closed or is there more remaining to
> commit?
>

There are still two pending patches:
 * Generic WAL records
 * Bloom filter contrib

Teodor is going to commit them.  But he is waiting Petr Jelinek to review
the English of the first one.

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

Re: [HACKERS] WIP: Access method extendability

[David Steele]

On 3/28/16 1:26 PM, Alvaro Herrera wrote:

Alexander Korotkov wrote:

On Thu, Mar 24, 2016 at 6:19 PM, Alvaro Herrera 
wrote:


.. Oh crap.  I just noticed I forgot to update a comment in pg_dump's
getAccessMethods.  And we're missing psql tab-complete support for the
new commands.


Attached patches fix both these issues.


I see Teodor just pushed these two fixes.  Thanks!


Does that mean this patch should be closed or is there more remaining to 
commit?


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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Alvaro Herrera]

Alexander Korotkov wrote:
> On Thu, Mar 24, 2016 at 6:19 PM, Alvaro Herrera 
> wrote:
> 
> > .. Oh crap.  I just noticed I forgot to update a comment in pg_dump's
> > getAccessMethods.  And we're missing psql tab-complete support for the
> > new commands.
> 
> Attached patches fix both these issues.

I see Teodor just pushed these two fixes.  Thanks!

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Alexander Korotkov]

Hi, Petr!

On Thu, Mar 17, 2016 at 10:56 AM, Petr Jelinek  wrote:

> On 16/03/16 15:31, Teodor Sigaev wrote:
>
>> Good catch, thanks! Tests were added.
>>>
>>
>> I don't see any objection, is consensus reached? I'm close to comiit
>> that...
>>
>>
> I did only cursory review on the bloom contrib module and don't really
> have complaints there, but I know you can review that one. I'd like the
> English of the generic_xlog.c description improved but I won't get to it
> before weekend.


What is your plans about English of the generic_xlog.c?

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

Re: [HACKERS] WIP: Access method extendability

[Alexander Korotkov]

On Thu, Mar 24, 2016 at 6:19 PM, Alvaro Herrera 
wrote:

> .. Oh crap.  I just noticed I forgot to update a comment in pg_dump's
> getAccessMethods.  And we're missing psql tab-complete support for the
> new commands.


Attached patches fix both these issues.

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


fix-pg_dump-comment.patch
Description: Binary data


fix-am-tab-complete.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Alvaro Herrera]

Alvaro Herrera wrote:
> Teodor Sigaev wrote:
> 
> > So, per patch status:
> > create-am
> > ready
> 
> Teodor agreed to me committing this one instead of him; thanks.  I just
> pushed it after some mostly cosmetic adjustments.

This was maybe too laconic.  I actually changed the code a good bit.
Some of the changes:

* pg_dump support got changed to use selectDumpableAccessMethod to
compare the OID to FirstNormalOid rather than embedding "1" in the
SQL query.  This is in line with what we do for other
no-schema-qualified object types.

* I changed DROP ACCESS METHOD to use the generic DropStmt instead of
creating a new production.  I find that most of the DropFooStmt
productions are useless -- we should remove them and instead merge
everything into DropStmt.

* I changed get_object_address to use get_object_address_unqualified,
just like all other object types which use no-schema-qualified object
names.  This removes a screenful of code.  I had to revert get_am_oid to
its original state instead of adding the "amtype" argument.  I added
get_index_am_oid.

* In SGML docs (and psql help) I changed the "TYPE INDEX" literal with
"TYPE access_method_type".

* I moved OCLASS_AM to a more sensible place rather than just stuffing
it at the end of the list

* I removed more than half the includes in the new file amcmds; there
were not necessary.

* I changed this:

+errmsg("function %s must return type 
\"index_am_handler\"",
+   NameListToString(handler_name)));

to this:

+errmsg("function %s must return type \"%s\"",
+   NameListToString(handler_name),
+   "index_am_handler")));

This eases the job of translators: 1) there's no point in presenting the
type name to translate, since it cannot be translated, and 2) all the
equivalent sentences share a single translation instead of needing a
dozen separate translations that only differ in a word that cannot be
translated anyway.  In doing this I noticed that most other uses do not
do this, so I'm going to change them too.


.. Oh crap.  I just noticed I forgot to update a comment in pg_dump's
getAccessMethods.  And we're missing psql tab-complete support for the
new commands.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Alvaro Herrera]

Teodor Sigaev wrote:

> So, per patch status:
> create-am
>   ready

Teodor agreed to me committing this one instead of him; thanks.  I just
pushed it after some mostly cosmetic adjustments.

I pass the baton back to Teodor for the remaining patches in this
series.

Thanks,

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Alvaro Herrera]

I don't quite see how this is supposed to work:

+ #ifdef WAL_DEBUG
+   /*
+* If xlog debug is enabled then check produced delta.  Result of delta
+* application to saved image should be the same as current page state.
+*/
+   if (XLOG_DEBUG)
+   {
+   chartmp[BLCKSZ];
+   memcpy(tmp, image, BLCKSZ);
+   applyPageRedo(tmp, pageData->data, pageData->dataLen);
+   elog(ERROR, "result of generic xlog apply doesn't match");
+   }
+ #endif

I suppose the elog(ERROR) call should be conditional ...

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Alexander Korotkov]

On Tue, Mar 22, 2016 at 11:53 PM, Alvaro Herrera 
wrote:

> Alexander Korotkov wrote:
> > On Tue, Mar 22, 2016 at 1:26 AM, Alvaro Herrera <
> alvhe...@2ndquadrant.com>
> > wrote:
>
> > > I noticed this state of affairs because I started reading the complete
> > > thread in order to verify whether a consensus had been reached
> regarding
> > > the move of IndexQualInfo and GenericCosts to selfuncs.h.  But I only
> > > see that Jim Nasby mentioned it and Alexander said "yes they move";
> > > nothing else.  The reason for raising this is that, according to
> > > Alexander, new index AMs will want to use those structs; but current
> > > index AMs only include index_selfuncs.h, and none of them includes
> > > selfuncs.h, so it seems completely wrong to be importing all the
> planner
> > > knowledge embodied in selfuncs.h into extension index AMs.
> > >
> > > One observation is that the bloom AM patch (0003 of this series) uses
> > > GenericCosts but not IndexQualInfo.  But btcostestimate() and
> > > gincostestimate() both use IndexQualInfo, so other AMs might well want
> > > to use it too.
> > >
> > > We could move GenericCosts and the prototype for genericcostestimate()
> > > to index_selfuncs.h; that much is pretty reasonable.  But I'm not sure
> > > what to do about IndexQualInfo.  We could just add forward struct
> > > declarations for RestrictInfo and Node to index_selfuncs.h.  But then
> > > the extension code is going to have to import the includes were those
> > > are defined anyway.  Certainly it seems that deconstruct_indexquals()
> > > (which returns a list of IndexQualInfo) is going to be needed by
> > > extension index AMs, at least ...
> >
> > Thank you for highlighting these issues.
>
> After thinking some more about this, I think it's all right to move both
> IndexQualInfo and GenericCosts to selfuncs.h.  The separation that we
> want is this: the selectivity functions themselves need access to a lot
> of planner knowledge, and so selfuncs.h knows a lot about planner.  But
> the code that _calls_ the selectivity function doesn't; and
> index_selfuncs.h is about the latter.  Properly architected extension
> index AMs are going to have their selectivity functions in a separate .c
> file which knows a lot about planner, and which includes selfuncs.h (and
> maybe index_selfuncs.h if it wants to piggyback on the existing
> selecticity functions, but that's unlikely); only the prototype for the
> selfunc is going to be needed elsewhere, and the rest of the index code
> is not going to know anything about planner stuff so it will not need to
> include selfuncs.h nor index_selfuncs.h.


Right, the purposes of headers are:
 * selfuncs.h – for those who going to estimate selectivity,
 * index_selfuncs.h – for those who going to call selectivity estimation
functions of built-in AMs.

>From this point for view we should keep both IndexQualInfo and GenericCosts
in selfuncs.h.

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

Re: [HACKERS] WIP: Access method extendability

[Alvaro Herrera]

Robert Haas wrote:
> On Tue, Mar 22, 2016 at 5:50 AM, Alexander Korotkov
>  wrote:
> >> And then Alexander posted this version, without any discussion that
> >> evidenced that those old objections were overridden.  What happened
> >> here?  Did somebody discuss this in unarchived meetings?  If so, it
> >> would be good to know about that in this thread.
> >
> > After that Tom Lane committed very huge patch about rework AM API.  One of
> > aims of this patch was support for CREATE ACCESS METHOD command.
> > http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=65c5fcd353a859da9e61bfb2b92a99f12937de3b
> > I've initially positioned this patch as refactoring for support CREATE
> > ACCESS METHOD command.
> > http://www.postgresql.org/message-id/capphfdtlisxmxk2b4tw+4+no_1-t0rao5foyszho6+sn2om...@mail.gmail.com
> > During discussing Tom mentioned future CREATE ACCESS METHOD command.
> > http://www.postgresql.org/message-id/16164.1439222...@sss.pgh.pa.us
> > I don't think Tom would put so much efforts in this direction if he would
> > remain opposed to this command.
> 
> Yeah, I rather thought we had consensus on that.

I vaguely remembered so too, but was startled to see that this thread
didn't have any evidence of it -- I thought I was misremembering.  I'm
glad we're all in the same page.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Robert Haas]

On Tue, Mar 22, 2016 at 5:50 AM, Alexander Korotkov
 wrote:
>> And then Alexander posted this version, without any discussion that
>> evidenced that those old objections were overridden.  What happened
>> here?  Did somebody discuss this in unarchived meetings?  If so, it
>> would be good to know about that in this thread.
>
> After that Tom Lane committed very huge patch about rework AM API.  One of
> aims of this patch was support for CREATE ACCESS METHOD command.
> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=65c5fcd353a859da9e61bfb2b92a99f12937de3b
> I've initially positioned this patch as refactoring for support CREATE
> ACCESS METHOD command.
> http://www.postgresql.org/message-id/capphfdtlisxmxk2b4tw+4+no_1-t0rao5foyszho6+sn2om...@mail.gmail.com
> During discussing Tom mentioned future CREATE ACCESS METHOD command.
> http://www.postgresql.org/message-id/16164.1439222...@sss.pgh.pa.us
> I don't think Tom would put so much efforts in this direction if he would
> remain opposed to this command.

Yeah, I rather thought we had consensus on that.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Alvaro Herrera]

Alexander Korotkov wrote:
> On Tue, Mar 22, 2016 at 1:26 AM, Alvaro Herrera 
> wrote:

> > I noticed this state of affairs because I started reading the complete
> > thread in order to verify whether a consensus had been reached regarding
> > the move of IndexQualInfo and GenericCosts to selfuncs.h.  But I only
> > see that Jim Nasby mentioned it and Alexander said "yes they move";
> > nothing else.  The reason for raising this is that, according to
> > Alexander, new index AMs will want to use those structs; but current
> > index AMs only include index_selfuncs.h, and none of them includes
> > selfuncs.h, so it seems completely wrong to be importing all the planner
> > knowledge embodied in selfuncs.h into extension index AMs.
> >
> > One observation is that the bloom AM patch (0003 of this series) uses
> > GenericCosts but not IndexQualInfo.  But btcostestimate() and
> > gincostestimate() both use IndexQualInfo, so other AMs might well want
> > to use it too.
> >
> > We could move GenericCosts and the prototype for genericcostestimate()
> > to index_selfuncs.h; that much is pretty reasonable.  But I'm not sure
> > what to do about IndexQualInfo.  We could just add forward struct
> > declarations for RestrictInfo and Node to index_selfuncs.h.  But then
> > the extension code is going to have to import the includes were those
> > are defined anyway.  Certainly it seems that deconstruct_indexquals()
> > (which returns a list of IndexQualInfo) is going to be needed by
> > extension index AMs, at least ...
> 
> Thank you for highlighting these issues.

After thinking some more about this, I think it's all right to move both
IndexQualInfo and GenericCosts to selfuncs.h.  The separation that we
want is this: the selectivity functions themselves need access to a lot
of planner knowledge, and so selfuncs.h knows a lot about planner.  But
the code that _calls_ the selectivity function doesn't; and
index_selfuncs.h is about the latter.  Properly architected extension
index AMs are going to have their selectivity functions in a separate .c
file which knows a lot about planner, and which includes selfuncs.h (and
maybe index_selfuncs.h if it wants to piggyback on the existing
selecticity functions, but that's unlikely); only the prototype for the
selfunc is going to be needed elsewhere, and the rest of the index code
is not going to know anything about planner stuff so it will not need to
include selfuncs.h nor index_selfuncs.h.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Alexander Korotkov]

On Tue, Mar 22, 2016 at 1:26 AM, Alvaro Herrera 
wrote:

> So.  Before this version of the patch was posted in Nov 4th 2015, both
> Tom and Heikki had said essentially "CREATE ACCESS METHOD is worthless,
> let's pursue this stuff without those commands".
> http://www.postgresql.org/message-id/54730dfd.2060...@vmware.com (Nov
> 2014)
>

Yes, that's it. In this email Heikki asserts that INSERTs into pg_am tables
in extensions would survive pg_upgrade, because pg_dump will dump CREATE
EXTENSION command which execute extension script.
In my response I noticed that it's not correct.  pg_dump in binary upgrade
mode works so that we will miss records in pg_am.  I haven't any answer
here.
http://www.postgresql.org/message-id/capphfdsbkntlchypngr0ffu9wlhh__nukdp13qlqukgtnv_...@mail.gmail.com
And, as Petr Jelinek mentioned, there is also problem of dependencies in
DROP EXTENSION.


> http://www.postgresql.org/message-id/26822.1414516...@sss.pgh.pa.us (Oct
> 2014)
>

> And then Alexander posted this version, without any discussion that
> evidenced that those old objections were overridden.  What happened
> here?  Did somebody discuss this in unarchived meetings?  If so, it
> would be good to know about that in this thread.


After that Tom Lane committed very huge patch about rework AM API.  One of
aims of this patch was support for CREATE ACCESS METHOD command.
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=65c5fcd353a859da9e61bfb2b92a99f12937de3b
I've initially positioned this patch as refactoring for support CREATE
ACCESS METHOD command.
http://www.postgresql.org/message-id/capphfdtlisxmxk2b4tw+4+no_1-t0rao5foyszho6+sn2om...@mail.gmail.com
During discussing Tom mentioned future CREATE ACCESS METHOD command.
http://www.postgresql.org/message-id/16164.1439222...@sss.pgh.pa.us
I don't think Tom would put so much efforts in this direction if he would
remain opposed to this command.

I noticed this state of affairs because I started reading the complete
> thread in order to verify whether a consensus had been reached regarding
> the move of IndexQualInfo and GenericCosts to selfuncs.h.  But I only
> see that Jim Nasby mentioned it and Alexander said "yes they move";
> nothing else.  The reason for raising this is that, according to
> Alexander, new index AMs will want to use those structs; but current
> index AMs only include index_selfuncs.h, and none of them includes
> selfuncs.h, so it seems completely wrong to be importing all the planner
> knowledge embodied in selfuncs.h into extension index AMs.
>
> One observation is that the bloom AM patch (0003 of this series) uses
> GenericCosts but not IndexQualInfo.  But btcostestimate() and
> gincostestimate() both use IndexQualInfo, so other AMs might well want
> to use it too.
>
> We could move GenericCosts and the prototype for genericcostestimate()
> to index_selfuncs.h; that much is pretty reasonable.  But I'm not sure
> what to do about IndexQualInfo.  We could just add forward struct
> declarations for RestrictInfo and Node to index_selfuncs.h.  But then
> the extension code is going to have to import the includes were those
> are defined anyway.  Certainly it seems that deconstruct_indexquals()
> (which returns a list of IndexQualInfo) is going to be needed by
> extension index AMs, at least ...
>

Thank you for highlighting these issues.


> I've made a few edits to the patch already, but I need to do some more
> testing.


Did you already fix the issues above or do you need me to fix them?


> Particularly I would like to understand the relcache issues to
> figure out whether the current one is right.


Good point.  I'll also try to find relcache issues.

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

Re: [HACKERS] WIP: Access method extendability

[Petr Jelinek]

On 21/03/16 23:26, Alvaro Herrera wrote:

Alexander Korotkov wrote:

Hi!

Thank you for review!


So.  Before this version of the patch was posted in Nov 4th 2015, both
Tom and Heikki had said essentially "CREATE ACCESS METHOD is worthless,
let's pursue this stuff without those commands".
http://www.postgresql.org/message-id/54730dfd.2060...@vmware.com (Nov 2014)
http://www.postgresql.org/message-id/26822.1414516...@sss.pgh.pa.us (Oct 2014)



And in sequence am thread Robert said the opposite.


And then Alexander posted this version, without any discussion that
evidenced that those old objections were overridden.  What happened
here?  Did somebody discuss this in unarchived meetings?  If so, it
would be good to know about that in this thread.


Well there are two main reasons for having DDL, one is dependency 
tracking and the other is binary upgrade. We can solve part of 
dependency tracking by recoding dependency between opclass and amhandler 
instead of opclass and access method, that would work fine. I don't know 
how to clean pg_am on DROP EXTENSION though without the dependency support.


I also am not sure what is good way of solving binary upgrade without 
any kind of DDL. Adding another pg_catalog.binary_upgrade_ 
function would be potential solution if we really think DDL is bad idea 
for access methods in general. Actually thinking of this, we might 
actually need function like in any case if we are recoding dependencies 
on access methods (which means it would probably be better to record 
dependency of opclass on amhandler as mentioned before, since this is 
already solved for functions and if the oid of am is not referenced 
anywhere it does not need special handling for binary upgrade).


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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Alvaro Herrera]

Alexander Korotkov wrote:
> Hi!
> 
> Thank you for review!

So.  Before this version of the patch was posted in Nov 4th 2015, both
Tom and Heikki had said essentially "CREATE ACCESS METHOD is worthless,
let's pursue this stuff without those commands".
http://www.postgresql.org/message-id/54730dfd.2060...@vmware.com (Nov 2014)
http://www.postgresql.org/message-id/26822.1414516...@sss.pgh.pa.us (Oct 2014)

And then Alexander posted this version, without any discussion that
evidenced that those old objections were overridden.  What happened
here?  Did somebody discuss this in unarchived meetings?  If so, it
would be good to know about that in this thread.

I noticed this state of affairs because I started reading the complete
thread in order to verify whether a consensus had been reached regarding
the move of IndexQualInfo and GenericCosts to selfuncs.h.  But I only
see that Jim Nasby mentioned it and Alexander said "yes they move";
nothing else.  The reason for raising this is that, according to
Alexander, new index AMs will want to use those structs; but current
index AMs only include index_selfuncs.h, and none of them includes
selfuncs.h, so it seems completely wrong to be importing all the planner
knowledge embodied in selfuncs.h into extension index AMs.

One observation is that the bloom AM patch (0003 of this series) uses
GenericCosts but not IndexQualInfo.  But btcostestimate() and
gincostestimate() both use IndexQualInfo, so other AMs might well want
to use it too.

We could move GenericCosts and the prototype for genericcostestimate()
to index_selfuncs.h; that much is pretty reasonable.  But I'm not sure
what to do about IndexQualInfo.  We could just add forward struct
declarations for RestrictInfo and Node to index_selfuncs.h.  But then
the extension code is going to have to import the includes were those
are defined anyway.  Certainly it seems that deconstruct_indexquals()
(which returns a list of IndexQualInfo) is going to be needed by
extension index AMs, at least ...

I've made a few edits to the patch already, but I need to do some more
testing.  Particularly I would like to understand the relcache issues to
figure out whether the current one is right.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Teodor Sigaev]

I don't see any objection, is consensus reached? I'm close to comiit
that...



I did only cursory review on the bloom contrib module and don't really have
complaints there, but I know you can review that one. I'd like the English of
the generic_xlog.c description improved but I won't get to it before weekend.


So, per patch status:
create-am
ready
generic-xlog
need to fix comments/docs
bloom-contrib
need review. I'm an original author of this module and of course
I can do some review of it but, seems, it's needed more eyes.
--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Alvaro Herrera]

Teodor Sigaev wrote:
> 
> >>I don't see any objection, is consensus reached? I'm close to comiit
> >>that...
> >>
> >
> >I did only cursory review on the bloom contrib module and don't really have
> >complaints there, but I know you can review that one. I'd like the English of
> >the generic_xlog.c description improved but I won't get to it before weekend.
> 
> So, per patch status:
> create-am
>   ready

Please wait a bit on this one -- I think more review is warranted.
I will try to review it early next week.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Petr Jelinek]

On 16/03/16 15:31, Teodor Sigaev wrote:

Good catch, thanks! Tests were added.


I don't see any objection, is consensus reached? I'm close to comiit
that...



I did only cursory review on the bloom contrib module and don't really 
have complaints there, but I know you can review that one. I'd like the 
English of the generic_xlog.c description improved but I won't get to it 
before weekend.


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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Teodor Sigaev]

Good catch, thanks! Tests were added.


I don't see any objection, is consensus reached? I'm close to comiit that...

--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Oleg Bartunov]

On Wed, Mar 9, 2016 at 8:31 PM, Alexander Korotkov <
a.korot...@postgrespro.ru> wrote:

> Hi!
>
> On Wed, Mar 9, 2016 at 3:27 PM, Alvaro Herrera 
> wrote:
>
>> Hi.  As I just said to Tomas Vondra: since your patch creates a new
>> object type, please make sure to add a case to cover it in the
>> object_address.sql test.  That verifies some things such as
>> pg_identify_object and related.
>>
>
> Good catch, thanks! Tests were added.
> I also introduced numbering into patch names to make evident the order to
> their application.
>
>
Nice to see progress ! I hope to see Alexander' work in 9.6.

I and Teodor will show at PGCon new GIN AM as an extension, optimized for
full text search (millisecond FTS) , which we gave up to push into core.


> --
> Alexander Korotkov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

Re: [HACKERS] WIP: Access method extendability

[Alvaro Herrera]

Hi.  As I just said to Tomas Vondra: since your patch creates a new
object type, please make sure to add a case to cover it in the
object_address.sql test.  That verifies some things such as
pg_identify_object and related.

Thanks,

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Petr Jelinek]

Hi,

I went over the latest version of this, here are my notes.

create-am.9:

+   case DO_ACCESS_METHOD:
+   snprintf(buf, bufsize,
+"");
+   return;

Missing the actual description.

+   appendPQExpBuffer(q, "CREATE ACCESS METHOD %s HANDLER %s;\n",
+ qamname, aminfo->amhandler);

Generates invalid syntax (missing TYPE).

I don't like that you hardcode 'i' everywhere in the code as am type 
index. It would be better to define it in pg_am.h and use that.


I was also debating in my head if amtype is correct name as it maps to 
relkind so amkind might be better name for the column, otoh then it 
won't map to the syntax we have agreed on for CREATE ACCESS METHOD so I 
guess there is no ideal name here.


object_type_map record is missing, as is getObjectTypeDescription and 
getObjectIdentityParts support


(you can check what I sent as part of sequence access methods where I 
fixed these things, otherwise it reuses most of your code)



generic-xlog.9:
Not much to say here, the api makes sense to me, it will have 
performance impact but don't see how we can do this generically without 
callbacks to extension in any better way.


One thing I don't understand is why there is support for unlogged 
relations. Shouldn't that be handled by whoever is using this to 
actually not call this at all? It's just call to RelationNeedsWAL() 
nothing to hard and hidden magic like not doing anything with WAL for 
the unlogged tables is seldomly good idea.


Another small thing is that we put the API explanation comments into .c 
file not .h file.


Didn't look at the bloom index too deeply yet.


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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Michael Paquier]

On Mon, Feb 29, 2016 at 7:42 PM, Alexander Korotkov
 wrote:
> On Fri, Feb 19, 2016 at 4:08 AM, Michael Paquier 
> wrote:
>> This is basically a copy of RewindTest.pm. This is far from generic.
>> If this patch gets committed first I would suggest to wait for the
>> more-generic routines that would be added in PostgresNode.pm and then
>> come back to it.
>
>
> Yes, that's it. Now, with committed changes to PostgresNode.pm, I get rid of
> separate WALTest.pm.

The test is cleaner in this shape, thanks.

+ # Run few queries on both master and stanbdy and check their results match.
s/stanbdy/standby
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Michael Paquier]

On Fri, Feb 19, 2016 at 12:51 AM, Alexander Korotkov wrote:
>> 11 I'd really like to see regression tests (TAP-tests?) for replication
>> with generic xlog.
>
>
> TAP test for replication added to bloom contrib. This test run on additional
> make target wal-check.

Just putting my eyes on that...

diff --git a/contrib/bloom/WALTest.pm b/contrib/bloom/WALTest.pm
new file mode 100644
index ...b2daf8b
*** a/contrib/bloom/WALTest.pm
--- b/contrib/bloom/WALTest.pm

This is basically a copy of RewindTest.pm. This is far from generic.
If this patch gets committed first I would suggest to wait for the
more-generic routines that would be added in PostgresNode.pm and then
come back to it.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Michael Paquier]

On Wed, Feb 17, 2016 at 11:56 PM, Teodor Sigaev  wrote:
> 11 I'd really like to see regression tests (TAP-tests?) for replication with
> generic xlog.

The recovery test suite can help to accomplish that.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Teodor Sigaev]

Next version of patch is attached:
  * Documentation for CREATE ACCESS METHOD command is added.
  * Refactoring and comments for bloom contrib.


Cool work, nice to see.

Some comments
1 create-am.7.patch: function lookup_index_am_handler_func() why doesn't it emit 
error in case of emtpy input? If it checks signature of function and
empty handler is not allowed then, seems, all checks about handler have to be 
moved in lookup_index_am_handler_func().


2 create-am.7.patch: Comment near get_am_name() incorrectly mentions  get_am_oid 
function


3 create-am.7.patch: get_am_name(Oid amOid, char amtype). Seems, amtype argument 
is overengineering.  get_am_name() is used only in error messages and additional 
check in it will do nothing or even confuse user.


4 create-am.7.patch: Inconsistentcy with psql help. As I can see other code, 
it's forbidden to create access method without handler

postgres=# \h create access method
Command: CREATE ACCESS METHOD
Description: define a new access method
Syntax:
CREATE ACCESS METHOD name
TYPE INDEX
[ HANDLER handler_function | NO HANDLER ]

postgres=# create access method yyy type index no handler;
ERROR:  syntax error at or near "no"
LINE 1: create access method yyy type index no handler;

5 create-am.7.patch: file src/bin/pg_dump/pg_dump.h. Extra comma near DO_POLICY:
*** 77,83 
DO_POST_DATA_BOUNDARY,
DO_EVENT_TRIGGER,
DO_REFRESH_MATVIEW,
!   DO_POLICY
  } DumpableObjectType;

  typedef struct _dumpableObject
--- 78,84 
DO_POST_DATA_BOUNDARY,
DO_EVENT_TRIGGER,
DO_REFRESH_MATVIEW,
!   DO_POLICY,
  } DumpableObjectType;

6 generic-xlog.7.patch: writeDelta() Seems, even under USE_ASSERT_CHECKING 
define checking diff by its applying is to expensive. May be, let we use here 
GENERIC_WAL_DEBUG macros?


7 generic-xlog.7.patch: src/backend/access/transam/generic_xlog.c It's unclear 
for me, what does MATCH_THRESHOLD do or intended for? Pls, add comments here.


8 generic-xlog.7.patch: src/backend/access/transam/generic_xlog.c. Again, it's 
unclear for me, what is motivation of size of PageData.data?


9 generic-xlog.7.patch: GenericXLogRegister(), Seems, emitting an error if 
caller tries to register buffer which is already registered isn't good idea. In 
practice, say, SP-GIST, buffer variable is used and page could be easily get 
from buffer. Suppose, GenericXLogRegister() should not emit an error and ignore 
isnew flag if case of second registration of the same buffer.


10 bloom-contrib.7.patch:
blutils.c: In function 'initBloomState':
blutils.c:128:20: warning: variable 'opaque' set but not used 
[-Wunused-but-set-variable]

   BloomPageOpaque  opaque;

11 I'd really like to see regression tests (TAP-tests?) for replication with 
generic xlog.





--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Alvaro Herrera]

Alexander Korotkov wrote:
> Hi!
> 
> Patches was rebased against master.
> 
> In the attached version of patch access methods get support of pg_dump.

Thanks.  This patch came in just as the commitfest was ending, so I'm
moving it to the next one.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Jim Nasby]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, failed
Spec compliant:   not tested
Documentation:not tested

There are currently no docs or unit tests. I suspect this patch is still WIP?

create-am.5.patch:
General notes:
Needs catversion bump.

IndexQualInfo and GenericCosts have been moved to src/include/utils/selfuncs.h.

METHOD becomes an unreserved keyword.

generic-xlog.5.patch:
generic_xlog.c: At least needs a bunch of explanatory comments, if not info in 
a README. Since I don't really understand what the design here is my review is 
just cursory.

static memoryMove() - seems like an overly-generic function name to me...

writeCopyFlagment(), writeMoveFlagment(): s/Fl/Fr/?

bloom-control.5:
README:
+ CREATE INDEX bloomidx ON tbloom(i1,i2,i3) 
+WITH (length=5, col1=2, col2=2, col3=4);
+ 
+ Here, we create bloom index with signature length 80 bits and attributes
+ i1, i2  mapped to 2 bits, attribute i3 - to 4 bits.

It's not clear to me where 80 bits is coming from...
bloom.h:
+ #define BITBYTE   (8)
ISTR seeing this defined somewhere in the Postgres headers; dunno if it's worth 
using that definition instead.

Testing:
I ran the SELECT INTO from the README, as well as CREATE INDEX bloomidx. I then 
ran

insert into tbloom select
(generate_series(1,1000)*random())::int as i1,
(generate_series(1,1000)*random())::int as i2,
(generate_series(1,1000)*random())::int as i3,
(generate_series(1,1000)*random())::int as i4,
(generate_series(1,1000)*random())::int as i5,
(generate_series(1,1000)*random())::int as i6,
(generate_series(1,1000)*random())::int as i7,
(generate_series(1,1000)*random())::int as i8,
(generate_series(1,1000)*random())::int as i9,
(generate_series(1,1000)*random())::int as i10,
(generate_series(1,1000)*random())::int as i11,
(generate_series(1,1000)*random())::int as i12,
(generate_series(1,1000)*random())::int as i13
 from generate_series(1,999);

and kill -9'd the backend. After restart I did VACUUM VERBOSE without issue. I 
ran the INSERT INTO, kill -9 and VACUUM VERBOSE sequence again. This time I got 
an assert:

TRAP: FailedAssertion("!(((bool) (((const void*)((ItemPointer) left) != 
((void*)0)) && (((ItemPointer) left)->ip_posid != 0", File: "vacuumlazy.c", 
Line: 1823)

That line is

lblk = ItemPointerGetBlockNumber((ItemPointer) left);

which does

AssertMacro(ItemPointerIsValid(pointer)), \

which is the assert that's failing.

I've squirreled this install away for now, in case you can't repro this failure.
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Alexander Korotkov]

Hi!

Thank you for review!

On Mon, Sep 7, 2015 at 6:41 PM, Teodor Sigaev  wrote:

> Some notices:
>
> 1) create-am.3.patch.gz
>   As I understand, you didn't add schema name to access method. Why?
> Suppose, if we implement SQL-like interface for AM screation/dropping then
> we should provide a schema qualification for them
>

Per subsequent discussion it's unclear why we need to make access methods
schema qualified.


> 2) create-am.3.patch.gz get_object_address_am()
> +   switch (list_length(objname))
> ...
> +   case 2:
> +   catalogname = strVal(linitial(objname));
> +   amname = strVal(lthird(objname));
> ^^ seems, it should be
> lsecond()
>

Fixed.


> 3) create-am.3.patch.gz
>  Suppose, RM_GENERIC_ID is part of generic-xlog.3.patch.gz
>

Fixed.

4) Makefile(s)
> As I can see, object files are lexicographically ordered
>

Fixed.

5) gindesc.c -> genericdesc.c in file header
>

Fixed.


> 6) generic-xlog.3.patch.gz
> I don't like an idea to split START_CRIT_SECTION and END_CRIT_SECTION to
> GenericXLogStart and GenericXLogFinish. User's code could throw a error or
> allocate memory between them and error will become a panic.


Fixed. Now, critical section is only inside GenericXLogFinish.
GenericXLogRegister returns pointer to the page copies which could be
modified. GenericXLogFinish swaps the data between page copy and page
itself. That allow us to avoid critical section in used code.

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


generic-xlog.4.patch.gz
Description: GNU Zip compressed data


create-am.4.patch.gz
Description: GNU Zip compressed data


bloom-contrib.4.patch.gz
Description: GNU Zip compressed data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Robert Haas]

On Mon, Sep 7, 2015 at 3:32 PM, Teodor Sigaev  wrote:
> Petr Jelinek wrote:
>>
>> On 2015-09-07 17:41, Teodor Sigaev wrote:
>>>
>>> Some notices:
>>>
>>> 1) create-am.3.patch.gz
>>>As I understand, you didn't add schema name to access method. Why?
>>> Suppose, if we implement SQL-like interface for AM screation/dropping
>>> then we should provide a schema qualification for them
>>
>>
>> Why would access methods need to be schema qualified?
>
>
> Opfamily and opclass could be schema-qualified.

So what?

Making access methods schema-qualified seems like a completely
separate project, and an unnecessary one.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Petr Jelinek]

On 2015-09-07 17:41, Teodor Sigaev wrote:

Some notices:

1) create-am.3.patch.gz
   As I understand, you didn't add schema name to access method. Why?
Suppose, if we implement SQL-like interface for AM screation/dropping
then we should provide a schema qualification for them


Why would access methods need to be schema qualified?

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Teodor Sigaev]

Some notices:

1) create-am.3.patch.gz
  As I understand, you didn't add schema name to access method. Why? Suppose, 
if we implement SQL-like interface for AM screation/dropping then we should 
provide a schema qualification for them


2) create-am.3.patch.gz get_object_address_am()
+   switch (list_length(objname))
...
+   case 2:
+   catalogname = strVal(linitial(objname));
+   amname = strVal(lthird(objname));
^^ seems, it should be lsecond()
3) create-am.3.patch.gz
 Suppose, RM_GENERIC_ID is part of generic-xlog.3.patch.gz

4) Makefile(s)
As I can see, object files are lexicographically ordered

5) gindesc.c -> genericdesc.c in file header

6) generic-xlog.3.patch.gz
I don't like an idea to split START_CRIT_SECTION and END_CRIT_SECTION to 
GenericXLogStart and GenericXLogFinish. User's code could throw a error or 
allocate memory between them and error will become a panic.



--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Alexander Korotkov]

On Tue, Sep 1, 2015 at 6:50 PM, Teodor Sigaev  wrote:

> In general pattern of generic WAL usage is following.
>>
>> 1) Start using generic WAL: specify relation
>>
>
> M-m, what about extensions which wants to use WAL but WAL record doesn't
> connected to any relation? For example, transaction manager or kind of FDW.
>
>
>> GenericXLogStart(index);
>>
>> 2) Register buffers
>>
>> GenericXLogRegister(0, buffer1, false);
>> GenericXLogRegister(1, buffer2, true);
>>
>> first argument is a slot number, second is the buffer, third is flag
>> indicating
>> new buffer
>>
>
> Why do we need a slot number? to replace already registered buffer?


For further simplification slot number could be omitted. In the attached
patches, generic xlog replay applies changes in the same order
GenericXLogRegister was called.
Patches was rebased against latest version of am interface rework patch.
http://www.postgresql.org/message-id/CAPpHfduGY=KZSBPZN5+USTXev-9M2PAUp3Yi=syfdo2n244...@mail.gmail.com

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


create-am.3.patch.gz
Description: GNU Zip compressed data


generic-xlog.3.patch.gz
Description: GNU Zip compressed data


bloom-contrib.3.patch.gz
Description: GNU Zip compressed data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Teodor Sigaev]

Petr Jelinek wrote:

On 2015-09-07 17:41, Teodor Sigaev wrote:

Some notices:

1) create-am.3.patch.gz
   As I understand, you didn't add schema name to access method. Why?
Suppose, if we implement SQL-like interface for AM screation/dropping
then we should provide a schema qualification for them


Why would access methods need to be schema qualified?


Opfamily and opclass could be schema-qualified.

--
Teodor Sigaev  E-mail: teo...@sigaev.ru
  WWW: http://www.sigaev.ru/


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Teodor Sigaev]

In general pattern of generic WAL usage is following.

1) Start using generic WAL: specify relation


M-m, what about extensions which wants to use WAL but WAL record doesn't 
connected to any relation? For example, transaction manager or kind of FDW.




GenericXLogStart(index);

2) Register buffers

GenericXLogRegister(0, buffer1, false);
GenericXLogRegister(1, buffer2, true);

first argument is a slot number, second is the buffer, third is flag indicating
new buffer


Why do we need a slot number? to replace already registered buffer?



--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Alexander Korotkov]

Hackers,

there is next revision of patches providing access method extendability.
Now it's based on another patch which reworks access method interface.
http://www.postgresql.org/message-id/capphfdsxwzmojm6dx+tjnpyk27kt4o7ri6x_4oswcbyu1rm...@mail.gmail.com

Besides access method interface, major change is generic xlog interface.
Now, generic xlog interface is more user friendly. Generic xlog compares
initial and changed versions of page by itself. The only thing it can't do
is to find data moves inside page, because it would be too high overhead.
So in order to get compact WAL records one should use
GenericXLogMemmove(dst, src, size) in order to move data inside page. If
this function wasn't used then WAL records would just not so compact.

In general pattern of generic WAL usage is following.

1) Start using generic WAL: specify relation

GenericXLogStart(index);

2) Register buffers

GenericXLogRegister(0, buffer1, false);
GenericXLogRegister(1, buffer2, true);

first argument is a slot number, second is the buffer, third is flag
indicating new buffer

3) Do changes in the pages. Use GenericXLogMemmove() if needed.

4) Finish using GenericXLogFinish(), or abort using GenericXLogAbort(). In
the case of abort initial state of pages will be reverted.

Generic xlog takes care about critical section, unlogged relation, setting
lsn, making buffer dirty. User code is just simple and clear.

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


create-am.2.patch.gz
Description: GNU Zip compressed data


generic-xlog.2.patch.gz
Description: GNU Zip compressed data


bloom-contrib.2.patch.gz
Description: GNU Zip compressed data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Heikki Linnakangas]

On 11/10/2014 10:30 PM, Alexander Korotkov wrote:

Don't allowing CREATE ACCESS METHOD command seems problematic for me. How
could it work with pg_upgrade? pg_dump wouldn't dump extra pg_am records.
So, pg_upgrade would break at creating operator classes on new cluster. So,
I agree with dropping create am command only if we let pg_dump to dump
extra pg_am records...


pg_dump would dump the CREATE EXTENSION command, and the extension's 
installation script inserts the row to pg_am. pg_dump doesn't dump 
objects that are part of an extension, so that's how this would work 
with the CREATE ACCESS METHOD command, too.


Backtracking a bit, to summarize the discussion so far:

* It would be nice to have indexes that are not WAL-logged, but are 
automatically re-created after a crash. But it's a completely different 
and orthogonal feature, so there's no need to discuss that further in 
this thread.


* If an extension is buggy, it can crash and corrupt the whole database. 
There isn't really anything we can do about that, and this patch doesn't 
make that any better or worse.


* CREATE ACCESS METHOD command isn't worth it.

Looking at the patch itself:

* It has bitrotted, thanks to my WAL format changes.

* The memcpy/memmove based API seems difficult to use correctly. Looking 
at the bloom filter example, it seems that you need a lot more code to 
implement WAL-logging with this, than you would with a rmgr-specific 
redo function. I was hoping this to make it simpler.


I think the API has to be more high-level. Or at least also provide a 
higher-level API, in addition to the low level one. Looking at the 
indexams we have, almost all pages use the standard page layout, with 
PageAddItem etc., plus a metapage, plus some indexam-specific metadata 
in the special area. The proposed API makes it pretty complicated to 
deal with that common case. After calling PageAddItem, you need intimate 
knowledge of what PageAddItem did, to create a correct WAL record for 
it. For PageIndexMultiDelete, it's next to impossible.


I think we'll have to accept that the WAL records with this generic API 
are going to be much bulkier than ones tailored for the AM. We could 
provide a compact representation for common operations like PageAddItem, 
but for any more complicated operations, just WAL-log a full page image, 
as it's too fiddly to track the changes at a finer level. For any 
serious performance critical stuff, you'll just have to write an 
old-fashioned rmgr.


(marking this as returned with feedback in the commitfest)

- Heikki



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Alexander Korotkov]

Hi, Heikki!

Thank you for summarizing. In general, I agree with your notes with
some exceptions.

On Mon, Nov 24, 2014 at 1:52 PM, Heikki Linnakangas hlinnakan...@vmware.com
 wrote:

 On 11/10/2014 10:30 PM, Alexander Korotkov wrote:

 Don't allowing CREATE ACCESS METHOD command seems problematic for me. How
 could it work with pg_upgrade? pg_dump wouldn't dump extra pg_am records.
 So, pg_upgrade would break at creating operator classes on new cluster.
 So,
 I agree with dropping create am command only if we let pg_dump to dump
 extra pg_am records...


 pg_dump would dump the CREATE EXTENSION command, and the extension's
 installation script inserts the row to pg_am. pg_dump doesn't dump objects
 that are part of an extension, so that's how this would work with the
 CREATE ACCESS METHOD command, too.


In binary upgrade mode pg_dump have to guarantee that all database objects
will have same oids. That's why in binary upgrade mode pg_dump dumps
extension contents instead of just CREATE EXTENSION command.


 Backtracking a bit, to summarize the discussion so far:

 * It would be nice to have indexes that are not WAL-logged, but are
 automatically re-created after a crash. But it's a completely different and
 orthogonal feature, so there's no need to discuss that further in this
 thread.

 * If an extension is buggy, it can crash and corrupt the whole database.
 There isn't really anything we can do about that, and this patch doesn't
 make that any better or worse.

 * CREATE ACCESS METHOD command isn't worth it.


Taking into account my previous note, how can custom extensions survive
pg_upgrade without CREATE ACCESS METHOD command?

--
With best regards,
Alexander Korotkov.

Re: [HACKERS] WIP: Access method extendability

[Alexander Korotkov]

Hi!

Thanks everybody for discussion. Sorry for delay. I'll try to address most
of questions arised in this discussion.

In general, I'm agree with concept of marking index as invalid in certain
cases.
I see following possible consequences of buggy WAL-logged custom AM:
1) Server crash during insert/update.
2) Server crash during select.
3) Invalid query answers.
4) Server crash during vacuum.
5) Server crash in recovery.

#5 is totally unacceptable. I've tried to design generic WAL record so that
it should be always possible to purely mechanically apply the record. It's
always possible to move piece of memory inside the page. It's always
possible to copy piece of memory from WAL-record to the page. Buggy WAL for
sure could cause an index corruption as well as any other bug in AM. WAL
support doesn't bring nothing special to this expect the fact that WAL is
quite hard to debug.

However, in current implementation I missed some hidden asserts about page
structure. Page could be so corrupted that we can't, for instance, safely
call XLogReadBufferForRedo(). All this cases must be worked out. If we
can't update page during recovery, index must be marked as invalid. It's
some amount of work, but it seems to be very feasible.

#4 seems problematic too. If autovacuum crashes on some index, then
postgres can enter a crash loop. So, if autovacuum crashes on extension
provided AM, that index should be marked as invalid.

Consequences #1, #3 and #3 could be easily caused by buggy opclass as well.
The reason why we're not knee-deep in extension provied bugs in GiST/GIN
opclasses is not easyness of writing correct GiST/GIN opclasses. Actual
reason is that we have only few GiST/GIN opclasses which weren't written by
GiST/GIN authors.

So, I don't expect PostgreSQL to be flooded with buggy AMs once we add AM
extendability. Our motivation behind this proposal is that we want to be
able to develop custom extensions with AMs. We want to be able to provide
our AMs to our customers whithout having to push that into PostgreSQL core
or fork PostgreSQL. Bugs in that AMs in our responsibility to out
customers. Some commercial companies could implement patented AMs (for
instance, fractal tree), and that is their responsibility to their
customers.

Also, I think it's OK to put adopting custom AMs to changes of AM interface
to authors of those custom AMs. AM extensions anyway should be adopted to
each new major release. AFAIR, interface of RelationOpen() function has
been changed not too long ago. Custom AM would use many functions which we
use to access relations. Their interface can be changed in the next release.

PostGIS GiST opclass has bugs which are reproducable, known and not fixed.
This is responsibility of PostGIS to their customers. We can feel sorry for
PostGIS that they are so slow on fixing this. But we shouldn't feel sorry
for GiST extendability, that woulde be redicilous.

Some recearches could write their own extensions. We can hope that they are
smart enough to not recommend it for production use. We can back our hope
with warning during installing extension provided AM. That warning could
say that all corruption caused by extension provided AM is up to AM
developer. This warning could make users to beware of using extension
provided AMs in production
unless they are fully trust extension developer (have support subscription
if it's commercial).

PostgreSQL doesn't have any kind of safe extensions. Every extension must
be trusted. Every extension can break (almost) everything.When one types
CREATE EXTENSION he must completely trust extension author. This applies to
every extension.

I would be very careful with discouraging commercial AM extensions. We
should always keen in the mind how many of PostgreSQL developers are
working for companies which own their commercial PostgreSQL forks and how
big their contribution is. Patented extensions looks scary for sure. But
it's up to software patents not up to PostgreSQL extendability...

Particular steps I'm going to do on these patches:
1) Make generic_redo never crash on broken pages.
2) Make autovacuum launcher mark index as invalid if vacuum process crashed
on custom AM index. Since, we can't write something into postgres cluster
when one process has crushed, ITSM autovacuum should have some separate
place to put this information. Thus, after restart postgres can read it and
mark index as invalid.

Don't allowing CREATE ACCESS METHOD command seems problematic for me. How
could it work with pg_upgrade? pg_dump wouldn't dump extra pg_am records.
So, pg_upgrade would break at creating operator classes on new cluster. So,
I agree with dropping create am command only if we let pg_dump to dump
extra pg_am records...

--
With best regards,
Alexander Korotkov.

Re: [HACKERS] WIP: Access method extendability

[Simon Riggs]

On 28 October 2014 23:25, Andres Freund and...@2ndquadrant.com wrote:
 On 2014-10-28 20:17:57 +, Simon Riggs wrote:
 On 28 October 2014 17:47, Andres Freund and...@2ndquadrant.com wrote:
  On 2014-10-28 17:45:36 +, Simon Riggs wrote:
  I'd like to avoid all of the pain by making persistent AMs that are
  recoverable after a crash, rather than during crash recovery.
 
  Besides the actual difficulities of supporting this, imo not being
  available on HS and directly after a failover essentially makes them
  next to useless.

 Broken WAL implementations are worse than useless.

...because the indexes are also unavailable during HS.

 I'm saying we should work on how to fix broken indexes first, before
 we allow a crop of new code that might cause them.

 Why do we presume all of them will be that buggy? And why is that
 different for nbtree, gin, gist? And how is any form of automated
 invalidation changing anything fundamentally?

The current system does not allow for the possibility of a corruption
bug. If one occurs, the only thing an AM can do is PANIC. It has no
mechanism to isolate the problem and deal with it, which affects the
whole server.

So the issue is one of risk of PANIC or data loss - things we have
always taken strong measures against. That is all I have requested as
a first step. And I request it because I remember and dealt with many
bugs and user problems in earlier times of 6-9 years ago.

You are also right: btree, GIN and GIST will benefit from this also.

 To me this is a pretty independent issue.

Initial users of GiST and GIN were rare. The clear target here is
indexing of JSONB data. I don't expect the users of that to be rare. I
expect adoption to be rapid and the effect of bugs to be widespread.

So I see the ability to report bugs and prevent data loss as essential
in the context of new AMs. Automatic fixing of the problem could be
fairly easy, but might be regarded as nice to have, as long as manual
fixing of the problem is easily possible.

I don't regard any of this as an insult or comment that certain people
write buggy code, while others are better people. Everybody has bugs
and WAL code in complex new index types is complex enough that it is
more likely than other places. I salute those who write innovative new
code for Postgres.

-- 
 Simon Riggs   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Robert Haas]

On Tue, Oct 28, 2014 at 7:25 PM, Andres Freund and...@2ndquadrant.com wrote:
 To me this is a pretty independent issue.

I quite agree.  As Stephen was at pains to remind me last night on
another thread, we cannot force people to write the patches we think
they should write.  They get to pursue what they think the priorities
are, not what anyone else thinks they are.  Of course we can and
should block patches that we think are a bad idea, or that are
badly-designed or badly-implemented for what they are, but we cannot
and should not block someone who feels that the first priority is A
just because we think it is B or C.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Jim Nasby]

On 10/28/14, 3:27 PM, Simon Riggs wrote:

On 28 October 2014 17:50, Jim Nasby jim.na...@bluetreble.com wrote:

On 10/28/14, 9:22 AM, Simon Riggs wrote:


2. Some additional code in Autovacuum to rebuild corrupt indexes at
startup, using AV worker processes to perform a REINDEX CONCURRENTLY.



I don't think loading more functionality into autovac is the right way to do
that.


You'd need to explain why and/or suggest your right way.


Why wouldn't we register it as a background worker?

Not only doesn't this have anything to do with vacuum, but it should operate 
differently as well: once we've rebuilt everything that needs to be rebuilt the 
process should go away until the next startup. That's the opposite of what 
autovac does.

The one potential commonality I see is having a launcher process that's 
responsible for launching multiple workers (if we want to be rebuilding 
multiple indexes at once), but AFAICT that capability is also provided by 
bgworker.c.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Andres Freund]

On 2014-10-29 14:33:00 -0500, Jim Nasby wrote:
 On 10/28/14, 3:27 PM, Simon Riggs wrote:
 On 28 October 2014 17:50, Jim Nasby jim.na...@bluetreble.com wrote:
 On 10/28/14, 9:22 AM, Simon Riggs wrote:
 
 2. Some additional code in Autovacuum to rebuild corrupt indexes at
 startup, using AV worker processes to perform a REINDEX CONCURRENTLY.
 
 
 I don't think loading more functionality into autovac is the right way to do
 that.
 
 You'd need to explain why and/or suggest your right way.
 
 Why wouldn't we register it as a background worker?
 
 Not only doesn't this have anything to do with vacuum, but it should operate 
 differently as well: once we've rebuilt everything that needs to be rebuilt 
 the process should go away until the next startup. That's the opposite of 
 what autovac does.

That's pretty much how autovac workers work. Do stuff until not needed
anymore. The difference is that you have a process that starts them.

It'd not be a good idea to throw this together with user defined
bgworkers because there's a finite number of slots for them. So at the
very least we'd need a separate pool for system bgworkers. Which would
persistently take up resources (PGPROC entries et al). So it seems
better to use the existing pool of autovac workers.

 The one potential commonality I see is having a launcher process that's 
 responsible for launching multiple workers (if we want to be rebuilding 
 multiple indexes at once), but AFAICT that capability is also provided by 
 bgworker.c.

There really is no need to use bgworkers for builtin things. They are
useful because they allow extensions to do what in core already could do
for a long time.

Greetings,

Andres Freund

PS: You mails would be easier to read if htey had sane line lenghts...

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Simon Riggs]

On 15 October 2014 13:08, Alexander Korotkov aekorot...@gmail.com wrote:

 Postgres was initially designed to support access methods extendability.
 This extendability lives to present day. However, this is mostly internal
 in-core extendability. One can quite easily add new access method into
 PostgreSQL core. But if one try to implement access method as external
 module, he will be faced with following difficulties:

...

 Problem of WAL is a bit more complex. According to previous discussions, we
 don't want to let extensions declare their own xlog records. If we let them
 then recovery process will depend on extensions. That is much violates
 reliability. Solution is to implement some generic xlog record which is able
 to represent difference between blocks in some general manner.

Thank you for progressing with these thoughts.

I'm still a little uncertain about the approach, now my eyes are open
to the problems of extendability.

The main problem we had in the past was that GiST and GIN indexes both
had faulty implementations for redo, which in some cases caused severe
issues. Adding new indexes will also suffer the same problems, so I
see a different starting place.

The faults there raised the need for us to be able to mark specific
indexes as corrupt, so that they could be avoided during Hot Standby
and in normal running after promotion.

Here's the order of features I think we need

1. A mechanism to mark an index as corrupt so that it won't be usable
by queries. That needs to work during recovery, so we can persist a
data structure which tells us which indexes are corrupt. Then
something that checks whether an index is known corrupt during
relcache access. So if we decide an index is bad, we record the index
as corrupt and then fire a relcache invalidation.

2. Some additional code in Autovacuum to rebuild corrupt indexes at
startup, using AV worker processes to perform a REINDEX CONCURRENTLY.

This will give us what we need to allow an AM to behave sensibly, even
in the face of its own bugs. It also gives us UNLOGGED indexes for
free. Unlogged indexes means we can change the way unlogged tables
behave to allow them to truncate down to the highest unchanged data at
recovery, so we don't lose all the data when we crash.

3. That then allows us to move towards having indexes that are marked
changed when we perform first DML on the table in any checkpoint
cycle. Which allows us to rebuild indexes which were in the middle of
being changed when we crashed. (The way we'd do that is to have an LSN
on the metapage and then only write WAL for the metapage). The
difference here is that they are UNLOGGED but do not get trashed on
recovery unless they were in the process of changing.

If we do those things, then we won't even need to worry about needing
AMs to write their own WAL records. Recovery will be safe AND we won't
need to go through problems of buggy persistence implementations in
new types of index.

Or put it another way, it will be easier to write new index AMs
because we'll be able to skip the WAL part until we know we want it.

-- 
 Simon Riggs   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Robert Haas]

On Tue, Oct 28, 2014 at 10:22 AM, Simon Riggs si...@2ndquadrant.com wrote:
 Or put it another way, it will be easier to write new index AMs
 because we'll be able to skip the WAL part until we know we want it.

I like the feature you are proposing, but I don't think that we should
block Alexander from moving forward with a more-extensible WAL format.
I believe that's a general need even if we get the features you're
proposing, which would reduce the need for it.  After all, if somebody
builds an out-of-core index AM, ignoring WAL-logging, and then decides
that it works well enough that they want to add WAL-logging, I think
we should make that possible without requiring them to move the whole
thing in-core.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Simon Riggs]

On 28 October 2014 14:53, Robert Haas robertmh...@gmail.com wrote:
 On Tue, Oct 28, 2014 at 10:22 AM, Simon Riggs si...@2ndquadrant.com wrote:
 Or put it another way, it will be easier to write new index AMs
 because we'll be able to skip the WAL part until we know we want it.

 I like the feature you are proposing, but I don't think that we should
 block Alexander from moving forward with a more-extensible WAL format.
 I believe that's a general need even if we get the features you're
 proposing, which would reduce the need for it.  After all, if somebody
 builds an out-of-core index AM, ignoring WAL-logging, and then decides
 that it works well enough that they want to add WAL-logging, I think
 we should make that possible without requiring them to move the whole
 thing in-core.

I'm not proposing an alternate or additional feature.

I'm saying that the first essential step in adding WAL support to new
AMs is to realise that they *will* have bugs (since with the greatest
respect, the last two AMs from our friends did have multiple bugs) and
so we must have a mechanism that prevents such bugs from screwing
everything else up. Which is the mark-corrupt-index and rebuild
requirement.

We skip straight to the add-buggy-AMs part at our extreme peril. We've
got about 10x as many users now since the 8.x bugs and all the new
users like the reputation Postgres has for resilience. I think we
should put the safety net in place first before we start to climb.

The patch as submitted doesn't have any safety checks for whether the
WAL records refer to persistent objects defined by the AM. At the very
least we need to be able to isolate an AM to only screw up their own
objects. Such checks look like they'd require some careful thought and
refactoring first.

-- 
 Simon Riggs   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Andres Freund]

Hi,

On 2014-10-15 16:08:38 +0400, Alexander Korotkov wrote:
 Postgres was initially designed to support access methods extendability.
 This extendability lives to present day. However, this is mostly internal
 in-core extendability. One can quite easily add new access method into
 PostgreSQL core. But if one try to implement access method as external
 module, he will be faced with following difficulties:
 
1. Need to directly insert into pg_am, because of no CREATE ACCESS
METHOD command. And no support of dependencies between am and opclasses
etc.
2. Module can't define xlog records. So, new am would be not WAL-logged.
 
 The first problem is purely mechanical. Nothing prevents us to implement
 CREATE ACCESS METHOD and DROP ACCESS METHOD commands and support all
 required dependencies.
 
 Problem of WAL is a bit more complex. According to previous discussions, we
 don't want to let extensions declare their own xlog records. If we let them
 then recovery process will depend on extensions. That is much violates
 reliability. Solution is to implement some generic xlog record which is
 able to represent difference between blocks in some general manner.

I think this is a somewhat elegant way to attack this problem. But I'm
not so sure it's actually sufficient. Consider e.g. how to deal with hot
standby conflicts? How would you transport the knowledge that there's a
xid conflict to the client?

I guess my question essentially is whether it's actually sufficient for
real world AMs.

The other thing I'm not sure about is that I'm unconvinced that we
really want external AMs...

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Oleg Bartunov]

On Tue, Oct 28, 2014 at 7:57 PM, Simon Riggs si...@2ndquadrant.com wrote:

 On 28 October 2014 14:53, Robert Haas robertmh...@gmail.com wrote:
  On Tue, Oct 28, 2014 at 10:22 AM, Simon Riggs si...@2ndquadrant.com
 wrote:
  Or put it another way, it will be easier to write new index AMs
  because we'll be able to skip the WAL part until we know we want it.
 
  I like the feature you are proposing, but I don't think that we should
  block Alexander from moving forward with a more-extensible WAL format.
  I believe that's a general need even if we get the features you're
  proposing, which would reduce the need for it.  After all, if somebody
  builds an out-of-core index AM, ignoring WAL-logging, and then decides
  that it works well enough that they want to add WAL-logging, I think
  we should make that possible without requiring them to move the whole
  thing in-core.

 I'm not proposing an alternate or additional feature.

 I'm saying that the first essential step in adding WAL support to new
 AMs is to realise that they *will* have bugs (since with the greatest
 respect, the last two AMs from our friends did have multiple bugs) and
 so we must have a mechanism that prevents such bugs from screwing
 everything else up. Which is the mark-corrupt-index and rebuild
 requirement.

 We skip straight to the add-buggy-AMs part at our extreme peril. We've
 got about 10x as many users now since the 8.x bugs and all the new
 users like the reputation Postgres has for resilience. I think we
 should put the safety net in place first before we start to climb.


agree and we thought about this



 The patch as submitted doesn't have any safety checks for whether the
 WAL records refer to persistent objects defined by the AM. At the very
 least we need to be able to isolate an AM to only screw up their own
 objects. Such checks look like they'd require some careful thought and
 refactoring first.


the patch Alexander submitted is the PoC, we wanted to hear developers
opinion and
I see no principal objection to work in this direction and we'll continue
to work on all
possible issues.




 --
  Simon Riggs   http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training  Services


 --
 Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
 To make changes to your subscription:
 http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Stephen Frost]

* Andres Freund (and...@2ndquadrant.com) wrote:
 The other thing I'm not sure about is that I'm unconvinced that we
 really want external AMs...

I was wondering about this also and curious as to if there's been any
prior on-list discussion about this proposal that I've simply missed..?

Would be happy to go back and review earlier discussions, of course, but
I don't recall there being any.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] WIP: Access method extendability

[Simon Riggs]

On 28 October 2014 16:19, Stephen Frost sfr...@snowman.net wrote:

 Would be happy to go back and review earlier discussions, of course, but
 I don't recall there being any.

It depends how far back you go.

I think I've had at least 2 tries at writing something, but not in last 5 years.

-- 
 Simon Riggs   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Simon Riggs]

On 28 October 2014 14:22, Simon Riggs si...@2ndquadrant.com wrote:

 Or put it another way, it will be easier to write new index AMs
 because we'll be able to skip the WAL part until we know we want it.

To be clear: I am suggesting you do *less* work, not more.

By allowing AMs to avoid writing WAL we get
* higher performance unlogged indexes
* we get fewer bugs in early days of new AMs
* writers of new AMs are OK to avoid majority of hard work and hard testing

So overall, we get new AMs working faster because we can skip writing
the WAL code until we are certain the new AM code is useful and bug
free.

For example, if GIN had avoided implementing WAL it would have been
easier to change on-disk representation.

-- 
 Simon Riggs   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Tom Lane]

Stephen Frost sfr...@snowman.net writes:
 * Andres Freund (and...@2ndquadrant.com) wrote:
 The other thing I'm not sure about is that I'm unconvinced that we
 really want external AMs...

 I was wondering about this also and curious as to if there's been any
 prior on-list discussion about this proposal that I've simply missed..?

We've touched on the issue a few times, but I don't think there's been
any attempt to define a project policy about it.

My own thought is that allowing external AMs is simply a natural
consequence of PG's general approach to extensibility, and it would
be surprising if we were to decide we didn't want to allow that.

But having said that, it's quite unclear to me that we need the
CREATE/DROP ACCESS METHOD infrastructure proposed here.  The traditional
theory about that is that if you're competent to develop an AM at all,
you can certainly manage to insert a row into pg_am manually.  I'm afraid
that we'd be adopting and maintaining thousands of lines of code that
won't ever come close to pulling their weight in usefulness, or probably
ever be fully debugged.  (The submitted patch is about 1K lines in itself,
and it doesn't appear to address any of the consequences of establishing
an expectation that AMs are something that can be dropped or modified.
Can you say cache flush?)

So I'd be inclined to put that part of the patch on the back burner until
there are actually multiple externally maintained AMs that could use it.
Even then, I'm not sure we want to buy into DROP ACCESS METHOD.

I think we *do* need some credible method for extensions to emit WAL
records, though.  I've not taken any close look at the code proposed
for that, but the two-sentence design proposal in the original post
sounded plausible as far as it went.

So my vote is to pursue the WAL extensibility part of this, but not the
additional SQL commands.

As for the proposed contrib module, we don't need it to test the WAL
extensibility stuff: we could just rewrite some existing core code to emit
the extensible WAL records instead of whatever it's emitting now.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Andres Freund]

On 2014-10-28 13:06:52 -0400, Tom Lane wrote:
 Stephen Frost sfr...@snowman.net writes:
  * Andres Freund (and...@2ndquadrant.com) wrote:
  The other thing I'm not sure about is that I'm unconvinced that we
  really want external AMs...
 
  I was wondering about this also and curious as to if there's been any
  prior on-list discussion about this proposal that I've simply missed..?
 
 We've touched on the issue a few times, but I don't think there's been
 any attempt to define a project policy about it.
 
 My own thought is that allowing external AMs is simply a natural
 consequence of PG's general approach to extensibility, and it would
 be surprising if we were to decide we didn't want to allow that.

It'd be entirely politicial. I agree. I'm pretty unhappy with the
thought that we end up with several 'for pay' index ams out there. But
then, PG is BSD style licensed.

What I think we need to make absolutely sure is that we preserve the
freedom to tinker with the AM functions. I think we'll very heavily
curse ourselves if we can't as easily add new features there anymore.

 But having said that, it's quite unclear to me that we need the
 CREATE/DROP ACCESS METHOD infrastructure proposed here.  The traditional
 theory about that is that if you're competent to develop an AM at all,
 you can certainly manage to insert a row into pg_am manually.

The problem with doing that is that you not only need to add a row in
pg_am, but also pg_depend. And a way to remove that row when the
respective extension is dropped. Especially the latter imo changed the
landscape a fair bit.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Tom Lane]

Andres Freund and...@2ndquadrant.com writes:
 On 2014-10-28 13:06:52 -0400, Tom Lane wrote:
 But having said that, it's quite unclear to me that we need the
 CREATE/DROP ACCESS METHOD infrastructure proposed here.  The traditional
 theory about that is that if you're competent to develop an AM at all,
 you can certainly manage to insert a row into pg_am manually.

 The problem with doing that is that you not only need to add a row in
 pg_am, but also pg_depend.

(1) That's not that hard; initdb makes pg_depend entries from SQL.
(2) You only need a row in pg_depend if you provide a DROP command
that would need to pay attention to it.

 And a way to remove that row when the
 respective extension is dropped.

I'm not at all sold on the idea that we need to support dropping AMs.
I think it'd be fine to consider that installing an AM into a given
database is a one-way operation.  Then you just need to insert some
pg_depend entries that pin the AM's individual functions, and you're
done.

Yeah, sure, CREATE/DROP ACCESS METHOD would be cleaner.  But in this
case I'm not buying the if you build it they will come argument.
External AMs *can* be built without any such SQL-level support, and if
there were really much demand for them, there would be some out there.
Let's build the essential WAL support first, and leave the syntactic
sugar till we see some demand.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Simon Riggs]

On 28 October 2014 17:06, Tom Lane t...@sss.pgh.pa.us wrote:

 My own thought is that allowing external AMs is simply a natural
 consequence of PG's general approach to extensibility, and it would
 be surprising if we were to decide we didn't want to allow that.

If it wasn't clear from my two earlier attempts, yes, +1 to that.

I'd like to avoid all of the pain by making persistent AMs that are
recoverable after a crash, rather than during crash recovery.

-- 
 Simon Riggs   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Andres Freund]

On 2014-10-28 17:45:36 +, Simon Riggs wrote:
 I'd like to avoid all of the pain by making persistent AMs that are
 recoverable after a crash, rather than during crash recovery.

Besides the actual difficulities of supporting this, imo not being
available on HS and directly after a failover essentially makes them
next to useless.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Tom Lane]

Simon Riggs si...@2ndquadrant.com writes:
 On 28 October 2014 17:06, Tom Lane t...@sss.pgh.pa.us wrote:
 My own thought is that allowing external AMs is simply a natural
 consequence of PG's general approach to extensibility, and it would
 be surprising if we were to decide we didn't want to allow that.

 If it wasn't clear from my two earlier attempts, yes, +1 to that.

 I'd like to avoid all of the pain by making persistent AMs that are
 recoverable after a crash, rather than during crash recovery.

I think the notion of having AMs that explicitly don't have WAL support
is quite orthogonal to what's being discussed in this thread.  It might
be worth doing that just to get the hash AM into a less-weird state
(given that nobody is stepping up to the plate to fix it properly).

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Andres Freund]

On 2014-10-28 13:37:33 -0400, Tom Lane wrote:
 I'm not at all sold on the idea that we need to support dropping AMs.
 I think it'd be fine to consider that installing an AM into a given
 database is a one-way operation.  Then you just need to insert some
 pg_depend entries that pin the AM's individual functions, and you're
 done.

I think that'd be somewhat ugly. An extension adding such a AM would
then either actively need to block dropping (e.g. by pinned entries, as
you mention) or do rather odd things on recreation. I think that'd be
dropping our own standards.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Jim Nasby]

On 10/28/14, 9:22 AM, Simon Riggs wrote:

2. Some additional code in Autovacuum to rebuild corrupt indexes at
startup, using AV worker processes to perform a REINDEX CONCURRENTLY.


I don't think loading more functionality into autovac is the right way to do 
that.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Alexander Korotkov]

On Tue, Oct 28, 2014 at 8:04 PM, Simon Riggs si...@2ndquadrant.com wrote:

 On 28 October 2014 14:22, Simon Riggs si...@2ndquadrant.com wrote:

  Or put it another way, it will be easier to write new index AMs
  because we'll be able to skip the WAL part until we know we want it.

 To be clear: I am suggesting you do *less* work, not more.

 By allowing AMs to avoid writing WAL we get
 * higher performance unlogged indexes
 * we get fewer bugs in early days of new AMs
 * writers of new AMs are OK to avoid majority of hard work and hard testing

 So overall, we get new AMs working faster because we can skip writing
 the WAL code until we are certain the new AM code is useful and bug
 free.

 For example, if GIN had avoided implementing WAL it would have been
 easier to change on-disk representation.


Major problem of changing on-disk representation is that we have to support
both binary formats because of pg_upgrade. This problem is even burdened
with WAL, because WAL record redo function also have to support both
formats. However, it's also quite independent of WAL.

Having access methods as extensions can significantly improves situations
here. Imagine, GIN was an extension. One day we decide to change its binary
format. Then we can issue new extension, GIN2 for instance. User can
upgrade from GIN to GIN2 in following steps:

   1. CREATE EXTENSION gin2;
   2. CREATE INDEX CONCURRENTLY [new_index] USING gin2 ([index_def]);
   3. DROP INDEX  CONCURRENTLY [old_index];
   4. DROP EXTENSION gin;

No need to write and debug the code which reads both old and new binary
format. For sure, we need to support old GIN extension for some time. But,
we have to support old in-core versions too.

Unfortunately, I didn't mention this in the first post because I consider
this as a serious argument for extensible AMs.

Also, I'm not sure that many users have enough of courage to use unlogged
AMs. Absence of durability is only half of trouble, another half is lack of
streaming replication. I think if we have unlogged GIN then external
indexing engines would be used by majority of users instead of GIN.

--
With best regards,
Alexander Korotkov.

Re: [HACKERS] WIP: Access method extendability

[Stephen Frost]

* Alexander Korotkov (aekorot...@gmail.com) wrote:
 Having access methods as extensions can significantly improves situations
 here. Imagine, GIN was an extension. One day we decide to change its binary
 format. Then we can issue new extension, GIN2 for instance. User can
 upgrade from GIN to GIN2 in following steps:

We could support this without having GIN be an extension by simply
having a GIN2 in core also, so I don't buy off on this being a good
reason for extensions to provide AMs.  For my 2c, I'm pretty happy with
the general idea of read-old, write-new to deal with transistions.

It's more complicated, certainly, but I don't think trying to force
users off of an old version is actually going to work all that well and
we'd just end up having to support both the old and new extensions
indefinitely anyway.

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] WIP: Access method extendability

[k...@rice.edu]

On Tue, Oct 28, 2014 at 01:51:21PM -0400, Tom Lane wrote:
 Simon Riggs si...@2ndquadrant.com writes:
  On 28 October 2014 17:06, Tom Lane t...@sss.pgh.pa.us wrote:
  My own thought is that allowing external AMs is simply a natural
  consequence of PG's general approach to extensibility, and it would
  be surprising if we were to decide we didn't want to allow that.
 
  If it wasn't clear from my two earlier attempts, yes, +1 to that.
 
  I'd like to avoid all of the pain by making persistent AMs that are
  recoverable after a crash, rather than during crash recovery.
 
 I think the notion of having AMs that explicitly don't have WAL support
 is quite orthogonal to what's being discussed in this thread.  It might
 be worth doing that just to get the hash AM into a less-weird state
 (given that nobody is stepping up to the plate to fix it properly).
 
   regards, tom lane
 

Hi,

I think that someone is working on the hash index WAL problem, but are
coming up to speed on the whole system, which takes time. I know that
I have not had a large enough block of time to spend on it either. :(

Regards,
Ken


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Simon Riggs]

On 28 October 2014 17:47, Andres Freund and...@2ndquadrant.com wrote:
 On 2014-10-28 17:45:36 +, Simon Riggs wrote:
 I'd like to avoid all of the pain by making persistent AMs that are
 recoverable after a crash, rather than during crash recovery.

 Besides the actual difficulities of supporting this, imo not being
 available on HS and directly after a failover essentially makes them
 next to useless.

Broken WAL implementations are worse than useless.

I'm saying we should work on how to fix broken indexes first, before
we allow a crop of new code that might cause them.

-- 
 Simon Riggs   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Simon Riggs]

On 28 October 2014 17:58, Alexander Korotkov aekorot...@gmail.com wrote:

 Also, I'm not sure that many users have enough of courage to use unlogged
 AMs. Absence of durability is only half of trouble, another half is lack of
 streaming replication. I think if we have unlogged GIN then external
 indexing engines would be used by majority of users instead of GIN.

Please answer the problem I have raised.

How will you act when the new AM that you write breaks?
How will you advise your users that the durability they sensibly
desire is actually causing them data loss?

I haven't opposed your ideas in this patch; I have only observed the
necessary surrounding infrastructure is incomplete.

-- 
 Simon Riggs   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Simon Riggs]

On 28 October 2014 17:50, Jim Nasby jim.na...@bluetreble.com wrote:
 On 10/28/14, 9:22 AM, Simon Riggs wrote:

 2. Some additional code in Autovacuum to rebuild corrupt indexes at
 startup, using AV worker processes to perform a REINDEX CONCURRENTLY.


 I don't think loading more functionality into autovac is the right way to do
 that.

You'd need to explain why and/or suggest your right way.

-- 
 Simon Riggs   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP: Access method extendability

[Andres Freund]

On 2014-10-28 20:17:57 +, Simon Riggs wrote:
 On 28 October 2014 17:47, Andres Freund and...@2ndquadrant.com wrote:
  On 2014-10-28 17:45:36 +, Simon Riggs wrote:
  I'd like to avoid all of the pain by making persistent AMs that are
  recoverable after a crash, rather than during crash recovery.
 
  Besides the actual difficulities of supporting this, imo not being
  available on HS and directly after a failover essentially makes them
  next to useless.
 
 Broken WAL implementations are worse than useless.

 I'm saying we should work on how to fix broken indexes first, before
 we allow a crop of new code that might cause them.

Why do we presume all of them will be that buggy? And why is that
different for nbtree, gin, gist? And how is any form of automated
invalidation changing anything fundamentally?

To me this is a pretty independent issue.

Greetings,

Andres Freund

-- 
 Andres Freund http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training  Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers