logical_replication_mode
I suggest we rename this setting to something starting with debug_.
Right now, the name looks much too tempting for users to fiddle with. I
think this is similar to force_parallel_mode.
Also, the descriptions in guc_tables.c could be improved. For example,
gettext_noop("Controls when to replicate or apply each change."),
is pretty content-free and unhelpful.
On Thu, Aug 24, 2023 at 12:45 PM Peter Eisentraut <peter@eisentraut.org> wrote:
I suggest we rename this setting to something starting with debug_.
Right now, the name looks much too tempting for users to fiddle with. I
think this is similar to force_parallel_mode.
+1. How about debug_logical_replication?
Also, the descriptions in guc_tables.c could be improved. For example,
gettext_noop("Controls when to replicate or apply each change."),
is pretty content-free and unhelpful.
The other possibility I could think of is to change short_desc as:
"Allows to replicate each change for large transactions.". Do you have
any better ideas?
--
With Regards,
Amit Kapila.
On Friday, August 25, 2023 12:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Aug 24, 2023 at 12:45 PM Peter Eisentraut <peter@eisentraut.org>
wrote:I suggest we rename this setting to something starting with debug_.
Right now, the name looks much too tempting for users to fiddle with.
I think this is similar to force_parallel_mode.+1. How about debug_logical_replication?
Also, the descriptions in guc_tables.c could be improved. For
example,gettext_noop("Controls when to replicate or apply each change."),
is pretty content-free and unhelpful.
The other possibility I could think of is to change short_desc as:
"Allows to replicate each change for large transactions.". Do you have any
better ideas?
How about "Forces immediate streaming or serialization of changes in large
transactions." which is similar to the description in document.
I agree that renaming it to debug_xx would be better and
here is a patch that tries to do this.
Best Regards,
Hou zj
Attachments:
0001-Rename-logical_replication_mode-to-debug_logical_rep.patchapplication/octet-stream; name=0001-Rename-logical_replication_mode-to-debug_logical_rep.patchDownload
From ed5044f19bb048ace98a9e568c4bd89d7871238c Mon Sep 17 00:00:00 2001
From: Hou Zhijie <houzj.fnst@cn.fujitsu.com>
Date: Fri, 25 Aug 2023 13:59:00 +0800
Subject: [PATCH] Rename logical_replication_mode to debug_logical_replication
The logical_replication_mode GUC is intended for testing and debugging
purposes, but its current name may be misleading and encourage users to make
unnecessary changes.
To avoid confusion, renaming the GUC to a less misleading name
debug_logical_replication that casual users are less likely to mistakenly
assume needs to be modified in a regular logical replication setup.
---
doc/src/sgml/config.sgml | 12 ++++++------
.../replication/logical/applyparallelworker.c | 2 +-
src/backend/replication/logical/reorderbuffer.c | 12 ++++++------
src/backend/utils/misc/guc_tables.c | 14 +++++++-------
src/include/replication/reorderbuffer.h | 10 +++++-----
src/test/subscription/t/015_stream.pl | 2 +-
src/test/subscription/t/016_stream_subxact.pl | 2 +-
.../subscription/t/018_stream_subxact_abort.pl | 4 ++--
.../subscription/t/019_stream_subxact_ddl_abort.pl | 2 +-
src/test/subscription/t/023_twophase_stream.pl | 4 ++--
10 files changed, 32 insertions(+), 32 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 11251fa05e..b066049843 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -11712,10 +11712,10 @@ LOG: CleanUpLock: deleting: lock(0xb7acd844) id(24688,24696,0,0,0,1)
</listitem>
</varlistentry>
- <varlistentry id="guc-logical-replication-mode" xreflabel="logical_replication_mode">
- <term><varname>logical_replication_mode</varname> (<type>enum</type>)
+ <varlistentry id="guc-debug-logical-replication" xreflabel="debug_logical_replication">
+ <term><varname>debug_logical_replication</varname> (<type>enum</type>)
<indexterm>
- <primary><varname>logical_replication_mode</varname> configuration parameter</primary>
+ <primary><varname>debug_logical_replication</varname> configuration parameter</primary>
</indexterm>
</term>
<listitem>
@@ -11724,12 +11724,12 @@ LOG: CleanUpLock: deleting: lock(0xb7acd844) id(24688,24696,0,0,0,1)
<literal>immediate</literal>. The default is <literal>buffered</literal>.
This parameter is intended to be used to test logical decoding and
replication of large transactions. The effect of
- <varname>logical_replication_mode</varname> is different for the
+ <varname>debug_logical_replication</varname> is different for the
publisher and subscriber:
</para>
<para>
- On the publisher side, <varname>logical_replication_mode</varname>
+ On the publisher side, <varname>debug_logical_replication</varname>
allows streaming or serializing changes immediately in logical decoding.
When set to <literal>immediate</literal>, stream each change if the
<link linkend="sql-createsubscription-with-streaming"><literal>streaming</literal></link>
@@ -11742,7 +11742,7 @@ LOG: CleanUpLock: deleting: lock(0xb7acd844) id(24688,24696,0,0,0,1)
<para>
On the subscriber side, if the <literal>streaming</literal> option is set to
- <literal>parallel</literal>, <varname>logical_replication_mode</varname>
+ <literal>parallel</literal>, <varname>debug_logical_replication</varname>
can be used to direct the leader apply worker to send changes to the
shared memory queue or to serialize all changes to the file. When set to
<literal>buffered</literal>, the leader sends changes to parallel apply
diff --git a/src/backend/replication/logical/applyparallelworker.c b/src/backend/replication/logical/applyparallelworker.c
index 4e8ee2973e..6dfcb742f5 100644
--- a/src/backend/replication/logical/applyparallelworker.c
+++ b/src/backend/replication/logical/applyparallelworker.c
@@ -1160,7 +1160,7 @@ pa_send_data(ParallelApplyWorkerInfo *winfo, Size nbytes, const void *data)
* We don't try to send data to parallel worker for 'immediate' mode. This
* is primarily used for testing purposes.
*/
- if (unlikely(logical_replication_mode == LOGICAL_REP_MODE_IMMEDIATE))
+ if (unlikely(debug_logical_replication == DEBUG_LOGICAL_REP_IMMEDIATE))
return false;
/*
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 87a4d2a24b..63b14c05d6 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -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_replication_mode = LOGICAL_REP_MODE_BUFFERED;
+int debug_logical_replication = DEBUG_LOGICAL_REP_BUFFERED;
/* ---------------------------------------
* primary reorderbuffer support routines
@@ -3566,7 +3566,7 @@ ReorderBufferLargestStreamableTopTXN(ReorderBuffer *rb)
* pick the largest (sub)transaction at-a-time to evict and spill its changes to
* disk or send to the output plugin until we reach under the memory limit.
*
- * If logical_replication_mode is set to "immediate", stream or serialize the
+ * If debug_logical_replication is set to "immediate", stream or serialize the
* changes immediately.
*
* XXX At this point we select the transactions until we reach under the memory
@@ -3580,15 +3580,15 @@ ReorderBufferCheckMemoryLimit(ReorderBuffer *rb)
ReorderBufferTXN *txn;
/*
- * Bail out if logical_replication_mode is buffered and we haven't
+ * Bail out if debug_logical_replication is buffered and we haven't
* exceeded the memory limit.
*/
- if (logical_replication_mode == LOGICAL_REP_MODE_BUFFERED &&
+ if (debug_logical_replication == DEBUG_LOGICAL_REP_BUFFERED &&
rb->size < logical_decoding_work_mem * 1024L)
return;
/*
- * If logical_replication_mode is immediate, loop until there's no change.
+ * If debug_logical_replication is immediate, loop until there's no change.
* Otherwise, loop until we reach under the memory limit. One might think
* that just by evicting the largest (sub)transaction we will come under
* the memory limit based on assumption that the selected transaction is
@@ -3598,7 +3598,7 @@ ReorderBufferCheckMemoryLimit(ReorderBuffer *rb)
* change.
*/
while (rb->size >= logical_decoding_work_mem * 1024L ||
- (logical_replication_mode == LOGICAL_REP_MODE_IMMEDIATE &&
+ (debug_logical_replication == DEBUG_LOGICAL_REP_IMMEDIATE &&
rb->size > 0))
{
/*
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 7e00dccb21..1e01dc6b62 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -420,9 +420,9 @@ static const struct config_enum_entry ssl_protocol_versions_info[] = {
{NULL, 0, false}
};
-static const struct config_enum_entry logical_replication_mode_options[] = {
- {"buffered", LOGICAL_REP_MODE_BUFFERED, false},
- {"immediate", LOGICAL_REP_MODE_IMMEDIATE, false},
+static const struct config_enum_entry debug_logical_replication_options[] = {
+ {"buffered", DEBUG_LOGICAL_REP_BUFFERED, false},
+ {"immediate", DEBUG_LOGICAL_REP_IMMEDIATE, false},
{NULL, 0, false}
};
@@ -4969,15 +4969,15 @@ struct config_enum ConfigureNamesEnum[] =
},
{
- {"logical_replication_mode", PGC_USERSET, DEVELOPER_OPTIONS,
- gettext_noop("Controls when to replicate or apply each change."),
+ {"debug_logical_replication", PGC_USERSET, DEVELOPER_OPTIONS,
+ gettext_noop("Forces immediate streaming or serialization of changes in large transactions."),
gettext_noop("On the publisher, it allows streaming or serializing each change in logical decoding. "
"On the subscriber, it allows serialization of all changes to files and notifies the "
"parallel apply workers to read and apply them at the end of the transaction."),
GUC_NOT_IN_SAMPLE
},
- &logical_replication_mode,
- LOGICAL_REP_MODE_BUFFERED, logical_replication_mode_options,
+ &debug_logical_replication,
+ DEBUG_LOGICAL_REP_BUFFERED, debug_logical_replication_options,
NULL, NULL, NULL
},
diff --git a/src/include/replication/reorderbuffer.h b/src/include/replication/reorderbuffer.h
index 1b9db22acb..41d208166d 100644
--- a/src/include/replication/reorderbuffer.h
+++ b/src/include/replication/reorderbuffer.h
@@ -19,14 +19,14 @@
/* GUC variables */
extern PGDLLIMPORT int logical_decoding_work_mem;
-extern PGDLLIMPORT int logical_replication_mode;
+extern PGDLLIMPORT int debug_logical_replication;
-/* possible values for logical_replication_mode */
+/* possible values for debug_logical_replication */
typedef enum
{
- LOGICAL_REP_MODE_BUFFERED,
- LOGICAL_REP_MODE_IMMEDIATE
-} LogicalRepMode;
+ DEBUG_LOGICAL_REP_BUFFERED,
+ DEBUG_LOGICAL_REP_IMMEDIATE
+} DebugLogicalRepMode;
/* an individual tuple, stored in one chunk of memory */
typedef struct ReorderBufferTupleBuf
diff --git a/src/test/subscription/t/015_stream.pl b/src/test/subscription/t/015_stream.pl
index b450a78adf..d396d91c63 100644
--- a/src/test/subscription/t/015_stream.pl
+++ b/src/test/subscription/t/015_stream.pl
@@ -295,7 +295,7 @@ is($result, qq(10000), 'data replicated to subscriber after dropping index');
# Test serializing changes to files and notify the parallel apply worker to
# apply them at the end of the transaction.
$node_subscriber->append_conf('postgresql.conf',
- 'logical_replication_mode = immediate');
+ 'debug_logical_replication = immediate');
# Reset the log_min_messages to default.
$node_subscriber->append_conf('postgresql.conf',
"log_min_messages = warning");
diff --git a/src/test/subscription/t/016_stream_subxact.pl b/src/test/subscription/t/016_stream_subxact.pl
index 838049af65..1bfa15375e 100644
--- a/src/test/subscription/t/016_stream_subxact.pl
+++ b/src/test/subscription/t/016_stream_subxact.pl
@@ -79,7 +79,7 @@ sub test_streaming
my $node_publisher = PostgreSQL::Test::Cluster->new('publisher');
$node_publisher->init(allows_streaming => 'logical');
$node_publisher->append_conf('postgresql.conf',
- 'logical_replication_mode = immediate');
+ 'debug_logical_replication = immediate');
$node_publisher->start;
# Create subscriber node
diff --git a/src/test/subscription/t/018_stream_subxact_abort.pl b/src/test/subscription/t/018_stream_subxact_abort.pl
index 77c96011a9..90f1f13680 100644
--- a/src/test/subscription/t/018_stream_subxact_abort.pl
+++ b/src/test/subscription/t/018_stream_subxact_abort.pl
@@ -130,7 +130,7 @@ sub test_streaming
my $node_publisher = PostgreSQL::Test::Cluster->new('publisher');
$node_publisher->init(allows_streaming => 'logical');
$node_publisher->append_conf('postgresql.conf',
- 'logical_replication_mode = immediate');
+ 'debug_logical_replication = immediate');
$node_publisher->start;
# Create subscriber node
@@ -203,7 +203,7 @@ test_streaming($node_publisher, $node_subscriber, $appname, 1);
# Test serializing changes to files and notify the parallel apply worker to
# apply them at the end of the transaction.
$node_subscriber->append_conf('postgresql.conf',
- 'logical_replication_mode = immediate');
+ 'debug_logical_replication = immediate');
# Reset the log_min_messages to default.
$node_subscriber->append_conf('postgresql.conf',
"log_min_messages = warning");
diff --git a/src/test/subscription/t/019_stream_subxact_ddl_abort.pl b/src/test/subscription/t/019_stream_subxact_ddl_abort.pl
index 6c2a9c5bf1..6cd5920188 100644
--- a/src/test/subscription/t/019_stream_subxact_ddl_abort.pl
+++ b/src/test/subscription/t/019_stream_subxact_ddl_abort.pl
@@ -16,7 +16,7 @@ use Test::More;
my $node_publisher = PostgreSQL::Test::Cluster->new('publisher');
$node_publisher->init(allows_streaming => 'logical');
$node_publisher->append_conf('postgresql.conf',
- 'logical_replication_mode = immediate');
+ 'debug_logical_replication = immediate');
$node_publisher->start;
# Create subscriber node
diff --git a/src/test/subscription/t/023_twophase_stream.pl b/src/test/subscription/t/023_twophase_stream.pl
index fdc9e2a0f0..65d4d66997 100644
--- a/src/test/subscription/t/023_twophase_stream.pl
+++ b/src/test/subscription/t/023_twophase_stream.pl
@@ -301,7 +301,7 @@ $node_publisher->init(allows_streaming => 'logical');
$node_publisher->append_conf(
'postgresql.conf', qq(
max_prepared_transactions = 10
-logical_replication_mode = immediate
+debug_logical_replication = immediate
));
$node_publisher->start;
@@ -389,7 +389,7 @@ test_streaming($node_publisher, $node_subscriber, $appname, 1);
# Test serializing changes to files and notify the parallel apply worker to
# apply them at the end of the transaction.
$node_subscriber->append_conf('postgresql.conf',
- 'logical_replication_mode = immediate');
+ 'debug_logical_replication = immediate');
# Reset the log_min_messages to default.
$node_subscriber->append_conf('postgresql.conf',
"log_min_messages = warning");
--
2.30.0.windows.2
On 25.08.23 08:52, Zhijie Hou (Fujitsu) wrote:
On Friday, August 25, 2023 12:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Aug 24, 2023 at 12:45 PM Peter Eisentraut <peter@eisentraut.org>
wrote:I suggest we rename this setting to something starting with debug_.
Right now, the name looks much too tempting for users to fiddle with.
I think this is similar to force_parallel_mode.+1. How about debug_logical_replication?
Also, the descriptions in guc_tables.c could be improved. For
example,gettext_noop("Controls when to replicate or apply each change."),
is pretty content-free and unhelpful.
The other possibility I could think of is to change short_desc as:
"Allows to replicate each change for large transactions.". Do you have any
better ideas?How about "Forces immediate streaming or serialization of changes in large
transactions." which is similar to the description in document.I agree that renaming it to debug_xx would be better and
here is a patch that tries to do this.
Maybe debug_logical_replication is too general? Something like
debug_logical_replication_streaming would be more concrete. (Or
debug_logical_streaming.) Is that an appropriate name for what it's doing?
On Fri, Aug 25, 2023 at 12:38 PM Peter Eisentraut <peter@eisentraut.org> wrote:
On 25.08.23 08:52, Zhijie Hou (Fujitsu) wrote:
On Friday, August 25, 2023 12:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Aug 24, 2023 at 12:45 PM Peter Eisentraut <peter@eisentraut.org>
wrote:I suggest we rename this setting to something starting with debug_.
Right now, the name looks much too tempting for users to fiddle with.
I think this is similar to force_parallel_mode.+1. How about debug_logical_replication?
Also, the descriptions in guc_tables.c could be improved. For
example,gettext_noop("Controls when to replicate or apply each change."),
is pretty content-free and unhelpful.
The other possibility I could think of is to change short_desc as:
"Allows to replicate each change for large transactions.". Do you have any
better ideas?How about "Forces immediate streaming or serialization of changes in large
transactions." which is similar to the description in document.I agree that renaming it to debug_xx would be better and
here is a patch that tries to do this.Maybe debug_logical_replication is too general? Something like
debug_logical_replication_streaming would be more concrete.
Yeah, that sounds better.
(Or
debug_logical_streaming.) Is that an appropriate name for what it's doing?
Yes.
--
With Regards,
Amit Kapila.
On Friday, August 25, 2023 5:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Fri, Aug 25, 2023 at 12:38 PM Peter Eisentraut <peter@eisentraut.org> wrote:
On 25.08.23 08:52, Zhijie Hou (Fujitsu) wrote:
On Friday, August 25, 2023 12:28 PM Amit Kapila
<amit.kapila16@gmail.com> wrote:
On Thu, Aug 24, 2023 at 12:45 PM Peter Eisentraut
<peter@eisentraut.org>
wrote:I suggest we rename this setting to something starting with debug_.
Right now, the name looks much too tempting for users to fiddle with.
I think this is similar to force_parallel_mode.+1. How about debug_logical_replication?
Also, the descriptions in guc_tables.c could be improved. For
example,gettext_noop("Controls when to replicate or apply each
change."),is pretty content-free and unhelpful.
The other possibility I could think of is to change short_desc as:
"Allows to replicate each change for large transactions.". Do you
have any better ideas?How about "Forces immediate streaming or serialization of changes in
large transactions." which is similar to the description in document.I agree that renaming it to debug_xx would be better and here is a
patch that tries to do this.Maybe debug_logical_replication is too general? Something like
debug_logical_replication_streaming would be more concrete.Yeah, that sounds better.
OK, here is the debug_logical_replication_streaming version.
Best Regards,
Hou zj
Attachments:
0001-Rename-logical_replication_mode-to-debug_logical_rep.patchapplication/octet-stream; name=0001-Rename-logical_replication_mode-to-debug_logical_rep.patchDownload
From 4baff7ac5a0c0cd1588e03ef2db1da848cc14d40 Mon Sep 17 00:00:00 2001
From: Hou Zhijie <houzj.fnst@cn.fujitsu.com>
Date: Fri, 25 Aug 2023 13:59:00 +0800
Subject: [PATCH] Rename logical_replication_mode to
debug_logical_replication_streaming
The logical_replication_mode GUC is intended for testing and debugging
purposes, but its current name may be misleading and encourage users to make
unnecessary changes.
To avoid confusion, renaming the GUC to a less misleading name
debug_logical_replication_streaming that casual users are less likely to mistakenly
assume needs to be modified in a regular logical replication setup.
---
doc/src/sgml/config.sgml | 12 ++++----
.../replication/logical/applyparallelworker.c | 2 +-
.../replication/logical/reorderbuffer.c | 30 +++++++++----------
src/backend/utils/misc/guc_tables.c | 14 ++++-----
src/include/replication/reorderbuffer.h | 10 +++----
src/test/subscription/t/015_stream.pl | 2 +-
src/test/subscription/t/016_stream_subxact.pl | 2 +-
.../t/018_stream_subxact_abort.pl | 4 +--
.../t/019_stream_subxact_ddl_abort.pl | 2 +-
.../subscription/t/023_twophase_stream.pl | 4 +--
10 files changed, 41 insertions(+), 41 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 11251fa05e..694d667bf9 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -11712,10 +11712,10 @@ LOG: CleanUpLock: deleting: lock(0xb7acd844) id(24688,24696,0,0,0,1)
</listitem>
</varlistentry>
- <varlistentry id="guc-logical-replication-mode" xreflabel="logical_replication_mode">
- <term><varname>logical_replication_mode</varname> (<type>enum</type>)
+ <varlistentry id="guc-debug-logical-replication-streaming" xreflabel="debug_logical_replication_streaming">
+ <term><varname>debug_logical_replication_streaming</varname> (<type>enum</type>)
<indexterm>
- <primary><varname>logical_replication_mode</varname> configuration parameter</primary>
+ <primary><varname>debug_logical_replication_streaming</varname> configuration parameter</primary>
</indexterm>
</term>
<listitem>
@@ -11724,12 +11724,12 @@ LOG: CleanUpLock: deleting: lock(0xb7acd844) id(24688,24696,0,0,0,1)
<literal>immediate</literal>. The default is <literal>buffered</literal>.
This parameter is intended to be used to test logical decoding and
replication of large transactions. The effect of
- <varname>logical_replication_mode</varname> is different for the
+ <varname>debug_logical_replication_streaming</varname> is different for the
publisher and subscriber:
</para>
<para>
- On the publisher side, <varname>logical_replication_mode</varname>
+ On the publisher side, <varname>debug_logical_replication_streaming</varname>
allows streaming or serializing changes immediately in logical decoding.
When set to <literal>immediate</literal>, stream each change if the
<link linkend="sql-createsubscription-with-streaming"><literal>streaming</literal></link>
@@ -11742,7 +11742,7 @@ LOG: CleanUpLock: deleting: lock(0xb7acd844) id(24688,24696,0,0,0,1)
<para>
On the subscriber side, if the <literal>streaming</literal> option is set to
- <literal>parallel</literal>, <varname>logical_replication_mode</varname>
+ <literal>parallel</literal>, <varname>debug_logical_replication_streaming</varname>
can be used to direct the leader apply worker to send changes to the
shared memory queue or to serialize all changes to the file. When set to
<literal>buffered</literal>, the leader sends changes to parallel apply
diff --git a/src/backend/replication/logical/applyparallelworker.c b/src/backend/replication/logical/applyparallelworker.c
index 4e8ee2973e..82f48a488e 100644
--- a/src/backend/replication/logical/applyparallelworker.c
+++ b/src/backend/replication/logical/applyparallelworker.c
@@ -1160,7 +1160,7 @@ pa_send_data(ParallelApplyWorkerInfo *winfo, Size nbytes, const void *data)
* We don't try to send data to parallel worker for 'immediate' mode. This
* is primarily used for testing purposes.
*/
- if (unlikely(logical_replication_mode == LOGICAL_REP_MODE_IMMEDIATE))
+ if (unlikely(debug_logical_replication_streaming == DEBUG_LOGICAL_REP_STREAMING_IMMEDIATE))
return false;
/*
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 87a4d2a24b..62797fa44d 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -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_replication_mode = LOGICAL_REP_MODE_BUFFERED;
+int debug_logical_replication_streaming = DEBUG_LOGICAL_REP_STREAMING_BUFFERED;
/* ---------------------------------------
* primary reorderbuffer support routines
@@ -3566,8 +3566,8 @@ ReorderBufferLargestStreamableTopTXN(ReorderBuffer *rb)
* pick the largest (sub)transaction at-a-time to evict and spill its changes to
* disk or send to the output plugin until we reach under the memory limit.
*
- * If logical_replication_mode is set to "immediate", stream or serialize the
- * changes immediately.
+ * If debug_logical_replication_streaming is set to "immediate", stream or
+ * serialize the changes immediately.
*
* XXX At this point we select the transactions until we reach under the memory
* limit, but we might also adapt a more elaborate eviction strategy - for example
@@ -3580,25 +3580,25 @@ ReorderBufferCheckMemoryLimit(ReorderBuffer *rb)
ReorderBufferTXN *txn;
/*
- * Bail out if logical_replication_mode is buffered and we haven't
- * exceeded the memory limit.
+ * Bail out if debug_logical_replication_streaming is buffered and we
+ * haven't exceeded the memory limit.
*/
- if (logical_replication_mode == LOGICAL_REP_MODE_BUFFERED &&
+ if (debug_logical_replication_streaming == DEBUG_LOGICAL_REP_STREAMING_BUFFERED &&
rb->size < logical_decoding_work_mem * 1024L)
return;
/*
- * If logical_replication_mode is immediate, loop until there's no change.
- * Otherwise, loop until we reach under the memory limit. One might think
- * that just by evicting the largest (sub)transaction we will come under
- * the memory limit based on assumption that the selected transaction is
- * at least as large as the most recent change (which caused us to go over
- * the memory limit). However, that is not true because a user can reduce
- * the logical_decoding_work_mem to a smaller value before the most recent
- * change.
+ * If debug_logical_replication_streaming is immediate, loop until there's
+ * no change. Otherwise, loop until we reach under the memory limit. One
+ * might think that just by evicting the largest (sub)transaction we will
+ * come under the memory limit based on assumption that the selected
+ * transaction is at least as large as the most recent change (which caused
+ * us to go over the memory limit). However, that is not true because a
+ * user can reduce the logical_decoding_work_mem to a smaller value before
+ * the most recent change.
*/
while (rb->size >= logical_decoding_work_mem * 1024L ||
- (logical_replication_mode == LOGICAL_REP_MODE_IMMEDIATE &&
+ (debug_logical_replication_streaming == DEBUG_LOGICAL_REP_STREAMING_IMMEDIATE &&
rb->size > 0))
{
/*
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 7e00dccb21..bcb5d5ad90 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -420,9 +420,9 @@ static const struct config_enum_entry ssl_protocol_versions_info[] = {
{NULL, 0, false}
};
-static const struct config_enum_entry logical_replication_mode_options[] = {
- {"buffered", LOGICAL_REP_MODE_BUFFERED, false},
- {"immediate", LOGICAL_REP_MODE_IMMEDIATE, false},
+static const struct config_enum_entry debug_logical_replication_streaming_options[] = {
+ {"buffered", DEBUG_LOGICAL_REP_STREAMING_BUFFERED, false},
+ {"immediate", DEBUG_LOGICAL_REP_STREAMING_IMMEDIATE, false},
{NULL, 0, false}
};
@@ -4969,15 +4969,15 @@ struct config_enum ConfigureNamesEnum[] =
},
{
- {"logical_replication_mode", PGC_USERSET, DEVELOPER_OPTIONS,
- gettext_noop("Controls when to replicate or apply each change."),
+ {"debug_logical_replication_streaming", PGC_USERSET, DEVELOPER_OPTIONS,
+ gettext_noop("Forces immediate streaming or serialization of changes in large transactions."),
gettext_noop("On the publisher, it allows streaming or serializing each change in logical decoding. "
"On the subscriber, it allows serialization of all changes to files and notifies the "
"parallel apply workers to read and apply them at the end of the transaction."),
GUC_NOT_IN_SAMPLE
},
- &logical_replication_mode,
- LOGICAL_REP_MODE_BUFFERED, logical_replication_mode_options,
+ &debug_logical_replication_streaming,
+ DEBUG_LOGICAL_REP_STREAMING_BUFFERED, debug_logical_replication_streaming_options,
NULL, NULL, NULL
},
diff --git a/src/include/replication/reorderbuffer.h b/src/include/replication/reorderbuffer.h
index 1b9db22acb..3cb03168de 100644
--- a/src/include/replication/reorderbuffer.h
+++ b/src/include/replication/reorderbuffer.h
@@ -19,14 +19,14 @@
/* GUC variables */
extern PGDLLIMPORT int logical_decoding_work_mem;
-extern PGDLLIMPORT int logical_replication_mode;
+extern PGDLLIMPORT int debug_logical_replication_streaming;
-/* possible values for logical_replication_mode */
+/* possible values for debug_logical_replication_streaming */
typedef enum
{
- LOGICAL_REP_MODE_BUFFERED,
- LOGICAL_REP_MODE_IMMEDIATE
-} LogicalRepMode;
+ DEBUG_LOGICAL_REP_STREAMING_BUFFERED,
+ DEBUG_LOGICAL_REP_STREAMING_IMMEDIATE
+} DebugLogicalRepStreamingMode;
/* an individual tuple, stored in one chunk of memory */
typedef struct ReorderBufferTupleBuf
diff --git a/src/test/subscription/t/015_stream.pl b/src/test/subscription/t/015_stream.pl
index b450a78adf..603d00f9e7 100644
--- a/src/test/subscription/t/015_stream.pl
+++ b/src/test/subscription/t/015_stream.pl
@@ -295,7 +295,7 @@ is($result, qq(10000), 'data replicated to subscriber after dropping index');
# Test serializing changes to files and notify the parallel apply worker to
# apply them at the end of the transaction.
$node_subscriber->append_conf('postgresql.conf',
- 'logical_replication_mode = immediate');
+ 'debug_logical_replication_streaming = immediate');
# Reset the log_min_messages to default.
$node_subscriber->append_conf('postgresql.conf',
"log_min_messages = warning");
diff --git a/src/test/subscription/t/016_stream_subxact.pl b/src/test/subscription/t/016_stream_subxact.pl
index 838049af65..9a2f06f272 100644
--- a/src/test/subscription/t/016_stream_subxact.pl
+++ b/src/test/subscription/t/016_stream_subxact.pl
@@ -79,7 +79,7 @@ sub test_streaming
my $node_publisher = PostgreSQL::Test::Cluster->new('publisher');
$node_publisher->init(allows_streaming => 'logical');
$node_publisher->append_conf('postgresql.conf',
- 'logical_replication_mode = immediate');
+ 'debug_logical_replication_streaming = immediate');
$node_publisher->start;
# Create subscriber node
diff --git a/src/test/subscription/t/018_stream_subxact_abort.pl b/src/test/subscription/t/018_stream_subxact_abort.pl
index 77c96011a9..201138882c 100644
--- a/src/test/subscription/t/018_stream_subxact_abort.pl
+++ b/src/test/subscription/t/018_stream_subxact_abort.pl
@@ -130,7 +130,7 @@ sub test_streaming
my $node_publisher = PostgreSQL::Test::Cluster->new('publisher');
$node_publisher->init(allows_streaming => 'logical');
$node_publisher->append_conf('postgresql.conf',
- 'logical_replication_mode = immediate');
+ 'debug_logical_replication_streaming = immediate');
$node_publisher->start;
# Create subscriber node
@@ -203,7 +203,7 @@ test_streaming($node_publisher, $node_subscriber, $appname, 1);
# Test serializing changes to files and notify the parallel apply worker to
# apply them at the end of the transaction.
$node_subscriber->append_conf('postgresql.conf',
- 'logical_replication_mode = immediate');
+ 'debug_logical_replication_streaming = immediate');
# Reset the log_min_messages to default.
$node_subscriber->append_conf('postgresql.conf',
"log_min_messages = warning");
diff --git a/src/test/subscription/t/019_stream_subxact_ddl_abort.pl b/src/test/subscription/t/019_stream_subxact_ddl_abort.pl
index 6c2a9c5bf1..1ad7ace84a 100644
--- a/src/test/subscription/t/019_stream_subxact_ddl_abort.pl
+++ b/src/test/subscription/t/019_stream_subxact_ddl_abort.pl
@@ -16,7 +16,7 @@ use Test::More;
my $node_publisher = PostgreSQL::Test::Cluster->new('publisher');
$node_publisher->init(allows_streaming => 'logical');
$node_publisher->append_conf('postgresql.conf',
- 'logical_replication_mode = immediate');
+ 'debug_logical_replication_streaming = immediate');
$node_publisher->start;
# Create subscriber node
diff --git a/src/test/subscription/t/023_twophase_stream.pl b/src/test/subscription/t/023_twophase_stream.pl
index fdc9e2a0f0..be9f2aab28 100644
--- a/src/test/subscription/t/023_twophase_stream.pl
+++ b/src/test/subscription/t/023_twophase_stream.pl
@@ -301,7 +301,7 @@ $node_publisher->init(allows_streaming => 'logical');
$node_publisher->append_conf(
'postgresql.conf', qq(
max_prepared_transactions = 10
-logical_replication_mode = immediate
+debug_logical_replication_streaming = immediate
));
$node_publisher->start;
@@ -389,7 +389,7 @@ test_streaming($node_publisher, $node_subscriber, $appname, 1);
# Test serializing changes to files and notify the parallel apply worker to
# apply them at the end of the transaction.
$node_subscriber->append_conf('postgresql.conf',
- 'logical_replication_mode = immediate');
+ 'debug_logical_replication_streaming = immediate');
# Reset the log_min_messages to default.
$node_subscriber->append_conf('postgresql.conf',
"log_min_messages = warning");
--
2.28.0.windows.1
Hi Hou-san.
I had a look at the patch 0001.
It looks OK to me, but here are a couple of comments:
======
1. Is this fix intended for PG16?
I found some mention of this GUC old name lurking in the release v16 notes [1]https://www.postgresql.org/docs/16/release-16.html.
~~~
2. DebugLogicalRepStreamingMode
-/* possible values for logical_replication_mode */
+/* possible values for debug_logical_replication_streaming */
typedef enum
{
- LOGICAL_REP_MODE_BUFFERED,
- LOGICAL_REP_MODE_IMMEDIATE
-} LogicalRepMode;
+ DEBUG_LOGICAL_REP_STREAMING_BUFFERED,
+ DEBUG_LOGICAL_REP_STREAMING_IMMEDIATE
+} DebugLogicalRepStreamingMode;Shouldn't this typedef name be included in the typedef.list file?
------
[1]: https://www.postgresql.org/docs/16/release-16.html
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Aug 29, 2023 at 12:56 PM Peter Smith <smithpb2250@gmail.com> wrote:
I had a look at the patch 0001.
It looks OK to me, but here are a couple of comments:
======
1. Is this fix intended for PG16?
Yes.
I found some mention of this GUC old name lurking in the release v16 notes [1].
That should be changed as well but we can do that as a separate patch
just for v16.
--
With Regards,
Amit Kapila.
On Tuesday, August 29, 2023 3:26 PM Peter Smith <smithpb2250@gmail.com> wrote:
Thanks for reviewing.
2. DebugLogicalRepStreamingMode
-/* possible values for logical_replication_mode */ +/* possible values for debug_logical_replication_streaming */ typedef enum { - LOGICAL_REP_MODE_BUFFERED, - LOGICAL_REP_MODE_IMMEDIATE -} LogicalRepMode; + DEBUG_LOGICAL_REP_STREAMING_BUFFERED, + DEBUG_LOGICAL_REP_STREAMING_IMMEDIATE +} DebugLogicalRepStreamingMode;Shouldn't this typedef name be included in the typedef.list file?
I think it's unnecessary to add this as there currently is no reference to the name.
See other similar examples like DebugParallelMode, RecoveryPrefetchValue ...
And the name is also not included in BF[1]https://buildfarm.postgresql.org/cgi-bin/typedefs.pl?branch=HEAD
[1]: https://buildfarm.postgresql.org/cgi-bin/typedefs.pl?branch=HEAD
Best Regards,
Hou zj
On 27.08.23 14:05, Zhijie Hou (Fujitsu) wrote:
On Friday, August 25, 2023 5:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Fri, Aug 25, 2023 at 12:38 PM Peter Eisentraut <peter@eisentraut.org> wrote:
On 25.08.23 08:52, Zhijie Hou (Fujitsu) wrote:
On Friday, August 25, 2023 12:28 PM Amit Kapila
<amit.kapila16@gmail.com> wrote:
On Thu, Aug 24, 2023 at 12:45 PM Peter Eisentraut
<peter@eisentraut.org>
wrote:I suggest we rename this setting to something starting with debug_.
Right now, the name looks much too tempting for users to fiddle with.
I think this is similar to force_parallel_mode.+1. How about debug_logical_replication?
Also, the descriptions in guc_tables.c could be improved. For
example,gettext_noop("Controls when to replicate or apply each
change."),is pretty content-free and unhelpful.
The other possibility I could think of is to change short_desc as:
"Allows to replicate each change for large transactions.". Do you
have any better ideas?How about "Forces immediate streaming or serialization of changes in
large transactions." which is similar to the description in document.I agree that renaming it to debug_xx would be better and here is a
patch that tries to do this.Maybe debug_logical_replication is too general? Something like
debug_logical_replication_streaming would be more concrete.Yeah, that sounds better.
OK, here is the debug_logical_replication_streaming version.
committed