Re: [Postgres-xc-developers] Patch for bug#3013984: Sequence scope

[Michael P. <mic...@gm...>]

Hi Benny,

I had a look at your patch,
and it is really a nice feature you implemented.

With this functionnality, it is possible to drop sequences on GTM in a very
smooth way when a database is dropped.
I have a couple of comments about it though.

1) in gtm_seq.c, function seq_start_with_nsp
You make a memory compare that will always return a true result because you
compare the same string.
       return ((memcmp(nsp->gsk_key, nsp->gsk_key, nsp->gsk_keylen) ==
0)
                       && (seq->gsk_key[nsp->gsk_keylen] == '.'));
I believe this is the right code:
       return ((memcmp(nsp->gsk_key, seq->gsk_key, nsp->gsk_keylen) ==
0)
                       && (seq->gsk_key[nsp->gsk_keylen] == '.'));

2) in gtm_seq.c, function GTM_RmSeqWithNsp
When you encounter a sequence that is used by another process, you mark it
for deletion and then you exit the process,
letting perhaps a lot of sequences not dropped.
I think priority should be given to deletion, as DROP DATABASE has to be
forced.
Btw, a busy sequence cannot basically be found because that would mean that
an instance is still using the database,
and in this case database drop is not possible.

3) in gtm_c.h, about the message type definition.
I made a modification in message type so as not to use a unique character.
Using an enumeration (typedef enum) makes it more consistent with
Postgres-xc code, and it makes code maintenance easier.

4) Also I notice a couple of typo problems, take care when you use spaces
and tabs!

I corrected those points above in the patch attached.
I also changed a couple of function names to make it clearer.

You should check it and make a couple of additional tests also.
Perhaps you have additional ideas about this implementation, don't hesitate
to share what you think.

Regards,

-- 
Michael Paquier
http://michaelpq.users.sourceforge.net