Re: Perform streaming logical transactions by background workers and parallel apply

On Tue, Jan 24, 2023 at 11:49 PM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)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