Обсуждение: Misleading error message in logical decoding for binary plugins

On 2014-08-29 22:42:46 +0900, Michael Paquier wrote:
> Hi all,
> 
> Using a plugin producing binary output, I came across this error:
> =# select data from pg_logical_slot_peek_changes('foo', NULL, NULL);
> ERROR:  0A000: output plugin cannot produce binary output
> LOCATION:  pg_logical_slot_get_changes_guts, logicalfuncs.c:404
> 
> Shouldn't the error message be here something like "binary output plugin
> cannot produce textual output"? The plugin used in my case produces binary
> output, but what is requested from it with pg_logical_slot_peek_changes is
> textual output.

I don't like the new message much. It's imo even more misleading than
the old message. How about
"output pluing produces binary output but the sink only accepts textual data"?

Greetings,

Andres Freund

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

On 2014-08-29 23:07:44 +0900, Michael Paquier wrote:
> On Fri, Aug 29, 2014 at 10:48 PM, Andres Freund <andres@2ndquadrant.com>
> wrote:
> 
> > On 2014-08-29 22:42:46 +0900, Michael Paquier wrote:
> > > Hi all,
> > >
> > > Using a plugin producing binary output, I came across this error:
> > > =# select data from pg_logical_slot_peek_changes('foo', NULL, NULL);
> > > ERROR:  0A000: output plugin cannot produce binary output
> > > LOCATION:  pg_logical_slot_get_changes_guts, logicalfuncs.c:404
> > >
> > > Shouldn't the error message be here something like "binary output plugin
> > > cannot produce textual output"? The plugin used in my case produces
> > binary
> > > output, but what is requested from it with pg_logical_slot_peek_changes
> > is
> > > textual output.
> >
> > I don't like the new message much. It's imo even more misleading than
> > the old message. How about
> > "output plugin produces binary output but the sink only accepts textual
> > data"?
> >
> Sounds fine for me. I am sure that native English speakers will come up as
> well with additional suggestions.
> 
> Btw, I am having a hard time understanding in which case
> OUTPUT_PLUGIN_BINARY_OUTPUT is actually useful. Looking at the code it is
> only a subset of what OUTPUT_PLUGIN_TEXTUAL_OUTPUT can do as textual can
> generate both binary and textual output, while binary can only generate
> binary output.

No, a textual output plugin is *NOT* allowed to produce binary
output. That'd violate e.g. pg_logical_slot_peek_changes's return type
because it's only declared to return text.
If you compile with assertions enabled not adhering to that will
actually result in an error:/* * Assert ctx->out is in database encoding when we're writing textual * output. */if
(!p->binary_output)   Assert(pg_verify_mbstr(GetDatabaseEncoding(),                           ctx->out->data,
ctx->out->len,                          false));
 

> The only code path where a flag OUTPUT_PLUGIN_* is used is
> in this code path for this very specific error message.

Well, LogicalDecodingContext->binary_output is checked too.

> On top of that, a
> logical receiver receives data in textual form even if the decoder is
> marked as binary when getting a message with PQgetCopyData.

I have no idea what you mean by that. The data is sent in the format the
output plugin writes the data in.

Greetings,

Andres Freund

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

On 2014-08-29 23:31:49 +0900, Michael Paquier wrote:
> On Fri, Aug 29, 2014 at 11:15 PM, Andres Freund <andres@2ndquadrant.com>
> wrote:
> 
> > No, a textual output plugin is *NOT* allowed to produce binary
> > output. That'd violate e.g. pg_logical_slot_peek_changes's return type
> > because it's only declared to return text.
> >
> 
> A textual output plugin can call pg_logical_slot_peek_binary_changes and
> pg_logical_slot_peek_changes as well,

Well, for one a output plugin doesn't call
pg_logical_slot_peek_binary_changes, it's the other way round. And sure:
Every text is also "binary". So that's just fine.

> and a binary output plugin can only call
> pg_logical_slot_peek_binary_changes, and will error out with
> pg_logical_slot_peek_changes:
> =# select pg_create_logical_replication_slot('foo', 'test_decoding');
>  pg_create_logical_replication_slot
> ------------------------------------
>  (foo,0/16C6880)
> (1 row)
> =# create table aa as select 1;
> SELECT 1
> =# select substring(encode(data, 'escape'), 1, 20),
>                substring(data, 1, 20)
>      FROM pg_logical_slot_peek_binary_changes('foo', NULL, NULL);
>       substring       |                 substring
> ----------------------+--------------------------------------------
>  BEGIN 1000           | \x424547494e2031303030
>  table public.aa: INS | \x7461626c65207075626c69632e61613a20494e53
>  COMMIT 1000          | \x434f4d4d49542031303030
> (3 rows)
> =# select pg_logical_slot_peek_changes('foo', NULL, NULL, 'force-binary',
> 'true');
> ERROR:  0A000: output plugin cannot produce binary output
> LOCATION:  pg_logical_slot_get_changes_guts, logicalfuncs.c:404
> =# select substring(data, 1, 20)
>     from pg_logical_slot_peek_binary_changes('foo', NULL, NULL,
> 'force-binary', 'true');
>                  substring
> --------------------------------------------
>  \x424547494e2031303030
>  \x7461626c65207075626c69632e61613a20494e53
>  \x434f4d4d49542031303030
> (3 rows)
> 
> Is that expected?

Yes. The output plugin declares whether it requires the *output method*
to support binary data. pg_logical_slot_peek_changes *can not* support
binary data because it outputs data as
text. pg_logical_slot_peek_binary_changes *can* support binary data
because it returns bytea (and thus it also can output text, because
that's essentially a subset of binary data).

Greetings,

Andres Freund

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

<div dir="ltr">On Fri, Aug 29, 2014 at 11:39 PM, Andres Freund <span dir="ltr"><<a
href="mailto:andres@2ndquadrant.com"target="_blank">andres@2ndquadrant.com</a>></span> wrote:<br /><div
class="gmail_extra"><divclass="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px
#cccsolid;padding-left:1ex">Yes. The output plugin declares whether it requires the *output method*<br /> to support
binarydata. pg_logical_slot_peek_changes *can not* support<br /> binary data because it outputs data as<br /> text.
pg_logical_slot_peek_binary_changes*can* support binary data<br /> because it returns bytea (and thus it also can
outputtext, because<br /> that's essentially a subset of binary data).<br /></blockquote>Thanks for the explanations.
Thiswould meritate more details within the docs, like what those two modes actually do and what the user can expect as
differences,advantages and disadvantages if he chooses one or the other when starting decoding...<br /></div>-- <br
/>Michael<br/></div></div> 

On Fri, Aug 29, 2014 at 9:48 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2014-08-29 22:42:46 +0900, Michael Paquier wrote:
>> Hi all,
>>
>> Using a plugin producing binary output, I came across this error:
>> =# select data from pg_logical_slot_peek_changes('foo', NULL, NULL);
>> ERROR:  0A000: output plugin cannot produce binary output
>> LOCATION:  pg_logical_slot_get_changes_guts, logicalfuncs.c:404
>>
>> Shouldn't the error message be here something like "binary output plugin
>> cannot produce textual output"? The plugin used in my case produces binary
>> output, but what is requested from it with pg_logical_slot_peek_changes is
>> textual output.
>
> I don't like the new message much. It's imo even more misleading than
> the old message. How about
> "output pluing produces binary output but the sink only accepts textual data"?

Maybe:

ERROR: pg_logical_slot_peek_changes cannot be used with a plugin that
produces only binary output
HINT: Use pg_logical_slot_peek_binary_changes instead.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2014-09-03 09:36:32 -0400, Robert Haas wrote:
> On Fri, Aug 29, 2014 at 9:48 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> > On 2014-08-29 22:42:46 +0900, Michael Paquier wrote:
> >> Hi all,
> >>
> >> Using a plugin producing binary output, I came across this error:
> >> =# select data from pg_logical_slot_peek_changes('foo', NULL, NULL);
> >> ERROR:  0A000: output plugin cannot produce binary output
> >> LOCATION:  pg_logical_slot_get_changes_guts, logicalfuncs.c:404
> >>
> >> Shouldn't the error message be here something like "binary output plugin
> >> cannot produce textual output"? The plugin used in my case produces binary
> >> output, but what is requested from it with pg_logical_slot_peek_changes is
> >> textual output.
> >
> > I don't like the new message much. It's imo even more misleading than
> > the old message. How about
> > "output pluing produces binary output but the sink only accepts textual data"?
> 
> Maybe:
> 
> ERROR: pg_logical_slot_peek_changes cannot be used with a plugin that
> produces only binary output
> HINT: Use pg_logical_slot_peek_binary_changes instead.

That level has no knowledge of what it's used by, so I think that'd
require bigger changes than worth it.

Greetings,

Andres Freund

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

On Wed, Sep 3, 2014 at 10:45 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2014-09-03 09:36:32 -0400, Robert Haas wrote:
>> On Fri, Aug 29, 2014 at 9:48 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>> > On 2014-08-29 22:42:46 +0900, Michael Paquier wrote:
>> >> Hi all,
>> >>
>> >> Using a plugin producing binary output, I came across this error:
>> >> =# select data from pg_logical_slot_peek_changes('foo', NULL, NULL);
>> >> ERROR:  0A000: output plugin cannot produce binary output
>> >> LOCATION:  pg_logical_slot_get_changes_guts, logicalfuncs.c:404
>> >>
>> >> Shouldn't the error message be here something like "binary output plugin
>> >> cannot produce textual output"? The plugin used in my case produces binary
>> >> output, but what is requested from it with pg_logical_slot_peek_changes is
>> >> textual output.
>> >
>> > I don't like the new message much. It's imo even more misleading than
>> > the old message. How about
>> > "output pluing produces binary output but the sink only accepts textual data"?
>>
>> Maybe:
>>
>> ERROR: pg_logical_slot_peek_changes cannot be used with a plugin that
>> produces only binary output
>> HINT: Use pg_logical_slot_peek_binary_changes instead.
>
> That level has no knowledge of what it's used by, so I think that'd
> require bigger changes than worth it.

ERROR: this logical decoding plugin can only produce binary output
ERROR: logical decoding plugin "%s" can only produce binary output

?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2014-09-03 10:59:17 -0400, Robert Haas wrote:
> On Wed, Sep 3, 2014 at 10:45 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> > On 2014-09-03 09:36:32 -0400, Robert Haas wrote:
> >> On Fri, Aug 29, 2014 at 9:48 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> >> > On 2014-08-29 22:42:46 +0900, Michael Paquier wrote:
> >> >> Hi all,
> >> >>
> >> >> Using a plugin producing binary output, I came across this error:
> >> >> =# select data from pg_logical_slot_peek_changes('foo', NULL, NULL);
> >> >> ERROR:  0A000: output plugin cannot produce binary output
> >> >> LOCATION:  pg_logical_slot_get_changes_guts, logicalfuncs.c:404
> >> >>
> >> >> Shouldn't the error message be here something like "binary output plugin
> >> >> cannot produce textual output"? The plugin used in my case produces binary
> >> >> output, but what is requested from it with pg_logical_slot_peek_changes is
> >> >> textual output.
> >> >
> >> > I don't like the new message much. It's imo even more misleading than
> >> > the old message. How about
> >> > "output pluing produces binary output but the sink only accepts textual data"?
> >>
> >> Maybe:
> >>
> >> ERROR: pg_logical_slot_peek_changes cannot be used with a plugin that
> >> produces only binary output
> >> HINT: Use pg_logical_slot_peek_binary_changes instead.
> >
> > That level has no knowledge of what it's used by, so I think that'd
> > require bigger changes than worth it.
> 
> ERROR: this logical decoding plugin can only produce binary output
> ERROR: logical decoding plugin "%s" can only produce binary output

ERROR: logical decoding plugin "%s" produces binary output, but sink only copes with textual data

Not sure about 'sink'. Maybe 'receiving side' or 'receiver'?

Not 100% sure if the name is available in that site, but if not it can
be left of without hurting much.

Greetings,

Andres Freund

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

On Wed, Sep 3, 2014 at 11:06 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>> >> Maybe:
>> >>
>> >> ERROR: pg_logical_slot_peek_changes cannot be used with a plugin that
>> >> produces only binary output
>> >> HINT: Use pg_logical_slot_peek_binary_changes instead.
>> >
>> > That level has no knowledge of what it's used by, so I think that'd
>> > require bigger changes than worth it.
>>
>> ERROR: this logical decoding plugin can only produce binary output
>> ERROR: logical decoding plugin "%s" can only produce binary output
>
> ERROR: logical decoding plugin "%s" produces binary output, but sink only copes with textual data
>
> Not sure about 'sink'. Maybe 'receiving side' or 'receiver'?
>
> Not 100% sure if the name is available in that site, but if not it can
> be left of without hurting much.

I was trying to avoid mentioning the word "sink" because we don't
actually have a real term for that.  From the user's perspective, it's
not going to be obvious that the function they invoked is the sink or
receiver; to them, it's just an interface - if anything, it's a
*sender* of the changes to them.

In case I lose that argument, please at least write "allows" instead
of "copes with"; the latter I think is too informal for an error
message.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2014-09-03 11:23:21 -0400, Robert Haas wrote:
> On Wed, Sep 3, 2014 at 11:06 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> >> >> Maybe:
> >> >>
> >> >> ERROR: pg_logical_slot_peek_changes cannot be used with a plugin that
> >> >> produces only binary output
> >> >> HINT: Use pg_logical_slot_peek_binary_changes instead.
> >> >
> >> > That level has no knowledge of what it's used by, so I think that'd
> >> > require bigger changes than worth it.
> >>
> >> ERROR: this logical decoding plugin can only produce binary output
> >> ERROR: logical decoding plugin "%s" can only produce binary output
> >
> > ERROR: logical decoding plugin "%s" produces binary output, but sink only copes with textual data
> >
> > Not sure about 'sink'. Maybe 'receiving side' or 'receiver'?
> >
> > Not 100% sure if the name is available in that site, but if not it can
> > be left of without hurting much.
> 
> I was trying to avoid mentioning the word "sink" because we don't
> actually have a real term for that.

I understand the hesitation. I don't like it either, but I don't think
it gets clearer by leaving it off entirely.

>  From the user's perspective, it's
> not going to be obvious that the function they invoked is the sink or
> receiver; to them, it's just an interface - if anything, it's a
> *sender* of the changes to them.

Is 'logical output method' perhaps better? It'd coincide with the terms
in the code and docs too.

> In case I lose that argument, please at least write "allows" instead
> of "copes with"; the latter I think is too informal for an error
> message.

Ok, sure.

Greetings,

Andres Freund

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