Hi Hou-san. Given there are some issues raised about the 0001 patch [1] I am skipping that one until I see the replies.
Meanwhile, here are some review comments for the patches v1-0002 and v1-0003 //////////////////// v1-0002 ====== Commit message 1. The pgoutput module uses a global variable(publish_no_origin) to cache the action for the origin filter. But we only initialize publish_no_origin when user specifies the "origin" in the output paramters which means we could refer to an uninitialized variable if user didn't specify the paramter. ~ 1a. typos /variable(publish_no_origin)/variable (publish_no_origin)/ /paramters/parameters/ /paramter./paramter./ ~ 1b. "...we could refer to an uninitialized variable" I'm not sure what this means. Previously it was static, so it wouldn't be "uninitialised"; it would be false. Perhaps there might be a stale value from a previous pgoutput, but IIUC that's the point made by your next paragraph ("Besides, we don't...") ~~~ 2. To improve it, the patch stores the map within the private data of the output plugin so that it will get initialized and reset along with the output plugin context. 2a. /To improve it,/To fix this/ ~ 2b. "stores the map" What map? This might be a cut/paste error from the v1-0001 patch comment. //////////////////// v1-0003 ====== Commit message 1. Missing patch comment. ====== src/backend/replication/pgoutput/pgoutput.c 2. maybe_send_schema - if (in_streaming) + if (data->in_streaming) set_schema_sent_in_streamed_txn((PGOutputData *) ctx->output_plugin_private, relentry, topxid); ~ Since you added a new 'data' variable, you might as well make use of it here instead of doing "(PGOutputData *) ctx->output_plugin_private" again. ====== src/include/replication/pgoutput.h 3. MemoryContext cachectx; /* private memory context for cache data */ + bool in_streaming; + Even though there was no comment previously when this was static, IMO it is better to comment on all the structure fields where possible. ------ [1] https://www.postgresql.org/message-id/ZQk1Ca_eFDTmBiZy%40paquier.xyz Kind Regards, Peter Smith. Fujitsu Australia