patch to ensure logical decoding errors early

Hi,

On 2018-07-31 14:51:12 -0400, Dave Cramer wrote:

This patch does 2 things

1) Ensure that when the slot is created
with pg_create_physical_replication_slot if the output plugin does not
exist it will error.

*logical, I assume?

diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c
index 3cd4eef..9f883b9 100644
--- a/src/backend/replication/logical/logical.c
+++ b/src/backend/replication/logical/logical.c
@@ -143,8 +143,7 @@ StartupDecodingContext(List *output_plugin_options,
* (re-)load output plugins, so we detect a bad (removed) output plugin
* now.
*/
-	if (!fast_forward)
-		LoadOutputPlugin(&ctx->callbacks, NameStr(slot->data.plugin));
+	LoadOutputPlugin(&ctx->callbacks, NameStr(slot->data.plugin));

So this actually was broken by 9c7d06d60680c7f00d931233873dee81fdb311c6
and worked before? Petr, Simon? Isn't the actual bug here that
CreateInitDecodingContext() passes true for fast_forward? Dave, could
you confirm this is the case? If so, this'll end up actually being an
open items entry...

/*
* Now that the slot's xmin has been set, we can announce ourselves as a
@@ -312,7 +311,7 @@ CreateInitDecodingContext(char *plugin,
ReplicationSlotSave();

ctx = StartupDecodingContext(NIL, InvalidXLogRecPtr, xmin_horizon,
- need_full_snapshot, true,
+ need_full_snapshot, true,
read_page, prepare_write, do_write,
update_progress);

Huh?

Greetings,

Andres Freund

On 31 July 2018 at 14:58, Andres Freund <andres@anarazel.de> wrote:

Hi,

On 2018-07-31 14:51:12 -0400, Dave Cramer wrote:

This patch does 2 things

1) Ensure that when the slot is created
with pg_create_physical_replication_slot if the output plugin does not
exist it will error.

*logical, I assume?

Yes, logical.

diff --git a/src/backend/replication/logical/logical.c

b/src/backend/replication/logical/logical.c

index 3cd4eef..9f883b9 100644
--- a/src/backend/replication/logical/logical.c
+++ b/src/backend/replication/logical/logical.c
@@ -143,8 +143,7 @@ StartupDecodingContext(List *output_plugin_options,
* (re-)load output plugins, so we detect a bad (removed) output

plugin

* now.
*/
- if (!fast_forward)
- LoadOutputPlugin(&ctx->callbacks,

NameStr(slot->data.plugin));

+ LoadOutputPlugin(&ctx->callbacks, NameStr(slot->data.plugin));

So this actually was broken by 9c7d06d60680c7f00d931233873dee81fdb311c6
and worked before? Petr, Simon? Isn't the actual bug here that
CreateInitDecodingContext() passes true for fast_forward? Dave, could
you confirm this is the case? If so, this'll end up actually being an
open items entry...

Ya, I think that is really the issue. I will redo my patch

/*
* Now that the slot's xmin has been set, we can announce

ourselves as a

@@ -312,7 +311,7 @@ CreateInitDecodingContext(char *plugin,
ReplicationSlotSave();

ctx = StartupDecodingContext(NIL, InvalidXLogRecPtr, xmin_horizon,
-

need_full_snapshot, true,

+

need_full_snapshot, true,

read_page, prepare_write, do_write,

update_progress);

Hmmm, I guess I should read my patches more carefully before sending them

Huh?

Dave Cramer

Hi,

On 31/07/18 20:58, Andres Freund wrote>

diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c
index 3cd4eef..9f883b9 100644
--- a/src/backend/replication/logical/logical.c
+++ b/src/backend/replication/logical/logical.c
@@ -143,8 +143,7 @@ StartupDecodingContext(List *output_plugin_options,
* (re-)load output plugins, so we detect a bad (removed) output plugin
* now.
*/
-	if (!fast_forward)
-		LoadOutputPlugin(&ctx->callbacks, NameStr(slot->data.plugin));
+	LoadOutputPlugin(&ctx->callbacks, NameStr(slot->data.plugin));

So this actually was broken by 9c7d06d60680c7f00d931233873dee81fdb311c6
and worked before? Petr, Simon? Isn't the actual bug here that
CreateInitDecodingContext() passes true for fast_forward? Dave, could
you confirm this is the case? If so, this'll end up actually being an
open items entry...

Indeed.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 1 August 2018 at 10:13, Petr Jelinek <petr.jelinek@2ndquadrant.com>
wrote:

Hi,

On 31/07/18 20:58, Andres Freund wrote>

diff --git a/src/backend/replication/logical/logical.c

b/src/backend/replication/logical/logical.c

index 3cd4eef..9f883b9 100644
--- a/src/backend/replication/logical/logical.c
+++ b/src/backend/replication/logical/logical.c
@@ -143,8 +143,7 @@ StartupDecodingContext(List *output_plugin_options,
* (re-)load output plugins, so we detect a bad (removed) output

plugin

* now.
*/
- if (!fast_forward)
- LoadOutputPlugin(&ctx->callbacks,

NameStr(slot->data.plugin));

+ LoadOutputPlugin(&ctx->callbacks, NameStr(slot->data.plugin));

So this actually was broken by 9c7d06d60680c7f00d931233873dee81fdb311c6
and worked before? Petr, Simon? Isn't the actual bug here that
CreateInitDecodingContext() passes true for fast_forward? Dave, could
you confirm this is the case? If so, this'll end up actually being an
open items entry...

Indeed.

See attached patch which fixes it, and adds a test for it.

On 2018-Aug-01, Dave Cramer wrote:

See attached patch which fixes it, and adds a test for it.

Pushed, thanks.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 1 August 2018 at 23:11, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

On 2018-Aug-01, Dave Cramer wrote:

See attached patch which fixes it, and adds a test for it.

Pushed, thanks.

Thanks

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services