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