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

First part of code review (for about 1/3rd of code):

+ /* Protocol capabilities */
Is this protocol version number and minimal recognized version number,
or major and minor version number? I assume that PGLOGICAL_PROTO_VERSION_NUM
is current protocol version (as in config max_proto_version and
min_proto_version). But why we have MIN_VERSION and not MAX_VERSION?

From code in pglogical_output.c (lines 215-225 it looks like
is treated as minimal protocol version number.

I can see that those constants are exported in pglogical_infofuncs.c and
pglogical.sql, but I do not understand omission of MAX.

+ typedef struct HookFuncName
+ typedef struct PGLogicalTupleData
I haven't found those used anything, and they are not mentioned in
documentation. Are those structures needed?

+ pglogical_config.c:
+               switch(get_param_key(elem->defname))
+               {
+                       val = get_param_value(elem, false, 
Why do we need this line here? All cases contain some variant of
val = get_param_value(elem, false, TYPE);
as first line after "case PARAM_*:" so this line should be removed.

+       val = get_param(options, "startup_params_format", false, false,
+                                       OUTPUT_PARAM_TYPE_UINT32, &found);
+       params_format = DatumGetUInt32(val);
Shouldn't we check "found" here? We work with val (which is Datum(0)) - won't
it throw SIGFAULT, or similar?
Additionally - I haven't found any case where code would use "found"
passed from get_param() - so as it's not used it might be removed.


+               elog(DEBUG1, "Binary mode rejected: Server and client endian 
sizeof(Datum) mismatch");
+               return false;
+       }
+       if (data->client_binary_sizeofint != 0
+               && data->client_binary_sizeofint != sizeof(int))
+       {
+               elog(DEBUG1, "Binary mode rejected: Server and client endian 
sizeof(int) mismatch");
+               return false;
+       }
+       if (data->client_binary_sizeoflong != 0
+               && data->client_binary_sizeoflong != sizeof(long))
+       {
+               elog(DEBUG1, "Binary mode rejected: Server and client endian 
sizeof(long) mismatch");
Isn't "endian" here case of copy-paste from first error?
Error messages should rather look like:
                elog(DEBUG1, "Binary mode rejected: Server and client 
sizeof(Datum) mismatch");

+ static void pg_decode_shutdown(LogicalDecodingContext * ctx)
In pg_decode_startup we create main memory context, and create hooks memory
context. In pg_decode_shutdown we delete hooks memory context but not main
context. Is this OK, or should we also add:

Sent via pgsql-hackers mailing list (
To make changes to your subscription:

Reply via email to