Postgres Pro
Downloads
Home   >   mailing lists

Thread: patch to ensure logical decoding errors early

patch to ensure logical decoding errors early

From
Dave Cramer
Date:
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.

2) Currently when the decoding context is created and the output plugin does not exist the protocol will respond with CopyDone. This patch will return an error instead and abort the copy connection.



Dave Cramer
Attachment

Re: patch to ensure logical decoding errors early

From
Andres Freund
Date:
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

Re: patch to ensure logical decoding errors early

From
Dave Cramer
Date:

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

Re: patch to ensure logical decoding errors early

From
Petr Jelinek
Date:
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

Re: patch to ensure logical decoding errors early

From
Dave Cramer
Date:

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.


Attachment

Re: patch to ensure logical decoding errors early

From
Alvaro Herrera
Date:
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

Re: patch to ensure logical decoding errors early

From
Simon Riggs
Date:
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