On Tue, Jan 24, 2023 at 11:49 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
...
>
> Sorry, the patch set was somehow attached twice. Here is the correct new version
> patch set which addressed all comments so far.
>
Here are my review comments for patch v87-0001.
======
src/backend/replication/logical/reorderbuffer.c
1.
@@ -210,7 +210,7 @@ int logical_decoding_work_mem;
static const Size max_changes_in_memory = 4096; /* XXX for restore only */
/* GUC variable */
-int logical_decoding_mode = LOGICAL_DECODING_MODE_BUFFERED;
+int logical_replication_mode = LOGICAL_REP_MODE_BUFFERED;
I noticed that the comment /* GUC variable */ is currently only above
the logical_replication_mode, but actually logical_decoding_work_mem
is a GUC variable too. Maybe this should be rearranged somehow then
change the comment "GUC variable" -> "GUC variables"?
======
src/backend/utils/misc/guc_tables.c
@@ -4908,13 +4908,13 @@ struct config_enum ConfigureNamesEnum[] =
},
{
- {"logical_decoding_mode", PGC_USERSET, DEVELOPER_OPTIONS,
+ {"logical_replication_mode", PGC_USERSET, DEVELOPER_OPTIONS,
gettext_noop("Allows streaming or serializing each change in logical
decoding."),
NULL,
GUC_NOT_IN_SAMPLE
},
- &logical_decoding_mode,
- LOGICAL_DECODING_MODE_BUFFERED, logical_decoding_mode_options,
+ &logical_replication_mode,
+ LOGICAL_REP_MODE_BUFFERED, logical_replication_mode_options,
NULL, NULL, NULL
},
That gettext_noop string seems incorrect. I think Kuroda-san
previously reported the same, but then you replied it has been fixed
already [1]
> I felt the description seems not to be suitable for current behavior.
> A short description should be like "Sets a behavior of logical replication", and
> further descriptions can be added in lond description.
I adjusted the description here.
But this doesn't look fixed to me. (??)
======
src/include/replication/reorderbuffer.h
3.
@@ -18,14 +18,14 @@
#include "utils/timestamp.h"
extern PGDLLIMPORT int logical_decoding_work_mem;
-extern PGDLLIMPORT int logical_decoding_mode;
+extern PGDLLIMPORT int logical_replication_mode;
Probably here should also be a comment to say "/* GUC variables */"
------
[1]
https://www.postgresql.org/message-id/OS0PR01MB5716AE9F095F9E7888987BC794C99%40OS0PR01MB5716.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia