Обсуждение: HOOKS for Synchronous Replication

Hi,

The implementation of a set of hooks for efficient synchronous replication 
without extensive patching of Postgresql source is now available at:
http://gorda.di.uminho.pt/community/

Note that this is far from a full replication product. It is made available 
for early feedback by the Postgresql hacker community and because it is 
probably already useful for other projects (e.g. materialized views).

This is the first self-contained component extracted from a working prototype 
of replication based on group communication under development by the GORDA 
project. 

Comments and suggestions welcome,

Regards Alfranio Junior.

On Mon, Jun 20, 2005 at 11:03:45AM +0100, Alfranio Correia Junior wrote:

Alfranio,

> The implementation of a set of hooks for efficient synchronous replication 
> without extensive patching of Postgresql source is now available at:
> 
>     http://gorda.di.uminho.pt/community/

Some random coments, from general eyeballing the patch:

First of all your patch does not conform with the project style.  Please
have a look at how other files are indented, in particular regarding
brace position and ereport() arguments (any function arguments really,
but in ereport your problems are more visible).  Also, always use
ereport() for user messages, elog() for conditions that involve
can't-happen situations (server bugs, like not finding a tuple in a
catalog that should be there, etc).

Also there are some changes that you certainly don't want committed.
For example why are you removing the TransState from xact.c?

Also, consider using the XactCallback mechanism instead of inventing
your own.


On a different front, have you considered talking to the people behind
Slony-II to see if they'd have some use for your hooks?

-- 
Alvaro Herrera (<alvherre[a]surnet.cl>)
Thou shalt check the array bounds of all strings (indeed, all arrays), for
surely where thou typest "foo" someone someday shall type
"supercalifragilisticexpialidocious" (5th Commandment for C programmers)

On Mon, 20 Jun 2005, Alvaro Herrera wrote:

> On Mon, Jun 20, 2005 at 11:03:45AM +0100, Alfranio Correia Junior wrote:
>
> Alfranio,
>
>> The implementation of a set of hooks for efficient synchronous replication
>> without extensive patching of Postgresql source is now available at:
>>
>>     http://gorda.di.uminho.pt/community/
>
> ...
>
> Also there are some changes that you certainly don't want committed.
> For example why are you removing the TransState from xact.c?

That puzzled me too, until I noticed that it's not removed, just moved to 
xact.h. The current transaction state is exposed in a new 
getCurrentTransactionState function, that's why TransState has to be in a 
header file...

I just took a quick glance, I can't say why it's that way.

- Heikki

Thank you for the comments.

>First of all your patch does not conform with the project style.  Please
>have a look at how other files are indented, in particular regarding
>brace position and ereport() arguments (any function arguments really,
>but in ereport your problems are more visible).  Also, always use
>ereport() for user messages, elog() for conditions that involve
>can't-happen situations (server bugs, like not finding a tuple in a
>catalog that should be there, etc).
>  
>
OK !!! I am going to correct these problems.

>Also there are some changes that you certainly don't want committed.
>For example why are you removing the TransState from xact.c?
>  
>
I simply moved it to the xact.h...
Basically, it was done to be able to access the current transaction 
state from other files.
Thus, I can avoid to rebuild the structure that stores the global 
triggers in the middle of a transaction.
However, the lack of this functionality it is not critical since the 
superuser is the only
person that can change such triggers.

>Also, consider using the XactCallback mechanism instead of inventing
>your own.
>
>  
>
Unfortunately, the XactCallback is called after "commit" which
means that a transaction cannot be rolled back.
For synchronous replication, it is necessary to have a before commit event.
Moreover,  the addition of a callback function implies modifications to 
the source code, am I wrong ?
These global triggers could be easily adapted to call functions "before" 
and "after commit" without
changing the code.

>On a different front, have you considered talking to the people behind
>Slony-II to see if they'd have some use for your hooks?
>
>  
>
I would like to share the hooks with them and other things that I have 
been done.

Best regards,

Alfranio Junior.

>First of all your patch does not conform with the project style.  Please
>have a look at how other files are indented, in particular regarding
>brace position and ereport() arguments (any function arguments really,
>but in ereport your problems are more visible).  Also, always use
>ereport() for user messages, elog() for conditions that involve
>can't-happen situations (server bugs, like not finding a tuple in a
>catalog that should be there, etc).
>  
>
I think it is ok now.
However, I corrected the indentation manually.
I could not run some of the tools, namely the "entab".

/usr/lib/gcc-lib/i386-redhat-linux/3.3.3/include/varargs.h:4:2: #error 
"GCC no longer implements <varargs.h>."
/usr/lib/gcc-lib/i386-redhat-linux/3.3.3/include/varargs.h:5:2: #error 
"Revise your code to use <stdarg.h>."
halt.c:23: error: syntax error before "va_dcl"
halt.c:24: error: syntax error before '{' token
----
Linux alfranio.lsd.di.uminho.pt 2.6.8-1.521 #1 Mon Aug 16 09:01:18 EDT 
2004 i686 i686 i386 GNU/Linux(GCC) 3.3.3 20040412 (Red Hat Linux 3.3.3-7)

Do you have any idea ?

Best regards,

Alfranio Junior.

Alfranio Correia Junior wrote:
> I think it is ok now.
> However, I corrected the indentation manually.
> I could not run some of the tools, namely the "entab".
> 
> /usr/lib/gcc-lib/i386-redhat-linux/3.3.3/include/varargs.h:4:2: #error 
> "GCC no longer implements <varargs.h>."
> /usr/lib/gcc-lib/i386-redhat-linux/3.3.3/include/varargs.h:5:2: #error 
> "Revise your code to use <stdarg.h>."
> halt.c:23: error: syntax error before "va_dcl"
> halt.c:24: error: syntax error before '{' token

I believe this should be fixed in CVS HEAD.

-Neil