Re: pglogical_output - a general purpose logical decoding output plugin - Mailing list pgsql-hackers

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

Applies cleanly on current master (b416c0bb622ce5d33fdbec3bbce00451132f10ec).

Builds without any problems on current Debian unstable (am64 arch, GCC 5.3.1-4, glibc 2.21-6)
There are 2 errors in tests, but they also occur on trunk build.
parallel group (20 tests):  json_encoding combocid portals_p2 advisory_lock tsdicts xmlmap equivclass guc
functional_depsdependency json select_views cluster tsearch window jsonb indirect_toast bitmapops foreign_key
foreign_data   select_views             ... FAILED    portals_p2               ... ok
 
parallel group (2 tests):  create_view create_index    create_index             ... FAILED    create_view
...ok
 

README.md:
+Only one database is replicated, rather than the whole PostgreSQL install. A
[...]
+Unlike block-level ("physical") streaming replication, the change stream from
+the `pglogical` output plugin is compatible across different PostgreSQL
+versions and can even be consumed by non-PostgreSQL clients.
+
+Because logical decoding is used, only the changed rows are sent on the wire.
+There's no index change data, no vacuum activity, etc transmitted.
+
+The use of a replication slot means that the change stream is reliable and
+crash-safe. If

Isn't it feature of logical replication, not this particular plugin?
I'm not sure whether all that text needs to be repeated here.
OTOH this is good summary - so maybe just add links to base
documentation about replication, logical replication, slots, etc.

+# Usage
+
+The overall flow of client/server interaction is:
This part (flow) belongs to DESIGN.md, not to usage.

+* [Client issues `IDENTIFY_SYSTEM`
Is the [ needed here?

protocol.txt
Contains wrapped lines, but also very long lines. While long
lines make sense for tables, they should not occur in paragraphs, e.g. in
+== Arguments client supplies to output plugin
and following ones. It looks like parts of this file were formatted, and parts not.

In summary, documentation is mostly OK, and it describe plugin quite nicely.
The only thing I haven't fully understood is COPY-related - so it might be
good to extend a bit. And how COPY relates to JSON output format?

pglogical_output_plhooks.c
+#if PG_VERSION_NUM >= 90500
+        username = GetUserNameFromId(GetUserId(), false);
+#else
+        username = GetUserNameFromId(GetUserId());
+#endif

Is it needed? I mean - will tris module compiled for 9.4 (or earlier)
versions, or will it be 9.5 (or even 9.6)+ only?

pglogical_output.c
+        /*
+         * Will we forward changesets? We have to if we're on 9.4;
+         * otherwise honour the client's request.
+         */
+        if (PG_VERSION_NUM/100 == 904)
+        {
+            /*
+             * 9.4 unconditionally forwards changesets due to lack of
+             * replication origins, and it can't ever send origin info
+             * for the same reason.
+             */

Similar case. In mail from 2015-11-12 (path v2) you removed v9.4 compatibility,
so I'm not sure whether checking for 9.4 or 9.5 makes any sense now.

This review focuses mostly on documentation, but I went through both
documentation and code. I understood most of the code (and it makes
sense after some cosideration :-) ), but I'm not proficient in PostgreSQL
to be fully sure that there are no hidden bugs.
At the same time - I haven't seen problems and suspicious fragments of code,
so after fixing mentioned problems it should go to the commiter.

Best regards.


The new status of this patch is: Waiting on Author