Thread: Misleading error message in logical decoding for binary plugins
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.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
A patch is attached (with s/pluggin/plugin in bonus). Comments welcome.
--
Michael
Michael
Attachment
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 Fri, Aug 29, 2014 at 10:48 PM, Andres Freund <andres@2ndquadrant.com> wrote:
Sounds fine for me. I am sure that native English speakers will come up as well with additional suggestions.On 2014-08-29 22:42:46 +0900, Michael Paquier wrote:I don't like the new message much. It's imo even more misleading than
> 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.
the old message. How about
"output plugin produces binary output but the sink only accepts textual data"?
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. The only code path where a flag OUTPUT_PLUGIN_* is used is in this code path for this very specific error message. 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.
Perhaps I am missing something... But in this case I think that the documentation should have a more detailed explanation about the output format options. Currently only the option value is mentioned, and there is nothing about what they actually do. This is error-prone for the user.
Regards,
--
Michael
Michael
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 Fri, Aug 29, 2014 at 11:15 PM, Andres Freund <andres@2ndquadrant.com> wrote:
A textual output plugin can call pg_logical_slot_peek_binary_changes and pg_logical_slot_peek_changes as well,
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)
--
Michael
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,
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?
Michael
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 11:15 PM, Andres Freund <andres@2ndquadrant.com> wrote:
-- > On top of that, aI have no idea what you mean by that. The data is sent in the format the
> logical receiver receives data in textual form even if the decoder is
> marked as binary when getting a message with PQgetCopyData.
output plugin writes the data in.
Well, that's where we don't understand each other. By reading the docs I understand that by setting output_type to OUTPUT_PLUGIN_BINARY_OUTPUT, all the changes generated by an output plugin would be automatically converted to binary and sent to a client back as binary data, but that's not the case. For example, when using pg_recvlogical on a slot plugged with test_decoding, the output received is not changed and remains like that even when using force-binary:
BEGIN 1000
table public.aa: INSERT: a[integer]:2"
COMMIT 1000
Perhaps I didn't understand the docs well, but this is confusing and it should be clearly specified to the user that output_type only impacts the SQL interface with the peek&get functions (as far as I tested). As things stand now, by reading the docs a user can only know that output_type can be set as OUTPUT_PLUGIN_BINARY_OUTPUT or OUTPUT_PLUGIN_TEXTUAL_OUTPUT, but there is nothing about what each value means and how it impacts the output format, and you need to look at the source code to get that this parameter is used for some sanity checks, only within the logical functions. That's not really user-friendly.
BEGIN 1000
table public.aa: INSERT: a[integer]:2"
COMMIT 1000
Perhaps I didn't understand the docs well, but this is confusing and it should be clearly specified to the user that output_type only impacts the SQL interface with the peek&get functions (as far as I tested). As things stand now, by reading the docs a user can only know that output_type can be set as OUTPUT_PLUGIN_BINARY_OUTPUT or OUTPUT_PLUGIN_TEXTUAL_OUTPUT, but there is nothing about what each value means and how it impacts the output format, and you need to look at the source code to get that this parameter is used for some sanity checks, only within the logical functions. That's not really user-friendly.
Attached is a patch improving the documentation regarding those comments.
Regards,Michael
Attachment
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