Re: pglogical - logical replication contrib module

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

Here is some more review

+- `pglogical.replication_set_add_table(set_name name, table_name regclass, synchronize boolean)`
+  Adds a table to replication set.
+
+  Parameters:
+  - `set_name` - name of the existing replication set
+  - `table_name` - name or OID of the table to be added to the set
+  - `synchronize` - if true, the table data is synchronized on all subscribers
+    which are subscribed to given replication set, default false
+

The argument to this function is actually named "relation" not "table_name" though we might want to update the function
toname the argument table_name.
 

Also we don't explain what 'synchronize' means I first thought that a value of false would mean that existing data
won'tbe copied but any new changes will be.
 
A value of false actually seems to mean that nothing will happen with the table until the synchronize function is
manuallycalled. We seem to be using the word 'synchronize' in different sense in different places I find it confusing
(iesynchronize_data and syncronize_structure in create_subscription).
 





*** a/contrib/pglogical/pglogical_sync.c
--- b/contrib/pglogical/pglogical_sync.c
+ static void
+ dump_structure(PGLogicalSubscription *sub, const char *snapshot)
+ {
+   char        pg_dump[MAXPGPATH];
+   uint32      version;
+   int         res;
+   StringInfoData  command;
+
+   if (find_other_exec_version(my_exec_path, PGDUMP_BINARY, &version, pg_dump))
+       elog(ERROR, "pglogical subscriber init failed to find pg_dump relative to binary %s",
+            my_exec_path);
+
+   if (version / 100 != PG_VERSION_NUM / 100)
+       elog(ERROR, "pglogical subscriber init found pg_dump with wrong major version %d.%d, expected %d.%d",
+            version / 100 / 100, version / 100 % 100,
+            PG_VERSION_NUM / 100 / 100, PG_VERSION_NUM / 100 % 100);
+
+   initStringInfo(&command);
+ #if PG_VERSION_NUM < 90500
+   appendStringInfo(&command, "%s --snapshot=\"%s\" -s -N %s -N pglogical_origin -F c -f \"/tmp/pglogical-%d.dump\"
\"%s\"",
+ #else
+   appendStringInfo(&command, "%s --snapshot=\"%s\" -s -N %s -F c -f \"/tmp/pglogical-%d.dump\" \"%s\"",

1) I am not sure we can assume/require that the pg_dump binary be in the same location as the postgres binary.  I don't
knowthink we've ever required that client binaries (ie psql, pg_dump, pg_restore ...) be in the same directory as
postgres. pg_upgrade does require this so maybe this isn't a problem in practice but I thought I'd point it out.
Ideallywouldn't need to call an external program to get a schema dump but turning pg_dump into a library is beyond the
scopeof this patch.
 


2) I don't think we can hard-coded /tmp as the directory for the schema dump.  I don't think will work on most windows
systemsand even on a unix system $TMPDIR might be set to something else.  Maybe writing this into pgsql_tmp would be a
betterchoice.  
 

Furtherdown in
pglogical_sync_subscription(PGLogicalSubscription *sub)
+   switch (status)
+   {
+       /* Already synced, nothing to do except cleanup. */
+       case SYNC_STATUS_READY:
+           MemoryContextDelete(myctx);
+           return;
+       /* We can recover from crashes during these. */
+       case SYNC_STATUS_INIT:
+       case SYNC_STATUS_CATCHUP:
+           break;
+       default:
+           elog(ERROR,
+                "subscriber %s initialization failed during nonrecoverable step (%c), please try the setup again",
+                sub->name, status);
+           break;
+   }

I think the default case needs to do something to unregister the background worker.  We already discussed trying to get
theerror message to a user in a better way either way there isn't any sense in this background worker being launched
againif the error is nonrecoverable.
 


+
+               tables = copy_replication_sets_data(sub->origin_if->dsn,
+                                                   sub->target_if->dsn,
+                                                   snapshot,
+                                                   sub->replication_sets);
+
+               /* Store info about all the synchronized tables. */
+               StartTransactionCommand();
+               foreach (lc, tables)

Shouldn't we be storing the info about the synchronized tables as part of the same transaction that does the sync?  


I'll keeping going through the code as I have time.   I think it is appropriate to move this to the next CF since the
CFis past the end date and the patch has received some review.   When you have an updated version of the patch post it,
don'twait until March.