Re: Fixing the docs for ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION

On Sat, May 22, 2021 at 10:22 PM Mark Dilger
<mark(dot)dilger(at)enterprisedb(dot)com> wrote:
> My concern isn't that the code is doing the wrong thing, but that the docs and the error messages are confusing. This is particularly troubling given that having a single action which combines the dropping of one publication with the refreshing of other publications is not particularly intuitive.
>
> I agree that disallowing copy_data DROP PUBLICATION is a reasonable design choice, but I do not agree that this prohibition is intuitive. If I want to copy the data for a set of tables on a remote server, and only copy that data exactly once, I might be looking for an atomic action to do so. The docs are totally unclear on whether this is supported, so I might try:
>
> CREATE SUBSCRIPTION tempsub CONNECTION 'dbname=remotedb' PUBLICATION remotepub WITH (connect = false, enabled = false, slot_name = NONE, create_slot = false);
> ALTER SUBSCRIPTION tempsub DROP PUBLICATION remotepub WITH (refresh = true, copy_data = true);
>
> with the intention that the data will be copied right before the publication is dropped. When I get an error that says 'unrecognized subscription parameter: "copy_data"', I'm likely to think I mistyped the parameter name, not that it is disallowed in this setting. If I then decide to just drop the publication (since my experiment didn't work) and try to do so using:
>
> ALTER SUBSCRIPTION tempsub DROP PUBLICATION remotepub WITH (refresh = false, copy_data = false);
>
> I seem to be playing by the rules, since I am explicitly not requesting "copy_data". That's what the "false" means. But again, the command complains that "copy_data" is unrecognized. At this point, I go back to the docs and it clearly says that "copy_data" is a supported parameter in this command. I'm totally confused.
>
> I think the docs should say that "copy_data" is not allowed for DROP PUBLICATION. I think no error should occur for copy_data = false. For copy_data = true, I think the error message should say that copy_data is disallowed during a DROP PUBLICATION, rather than saying that the parameter is unrecognized.

Thanks for the detailed explanation. I think there are two
possibilities - unrecognised options and disallowed options. If a user
enters an option 'blah_blah', then the error "unrecognized
subscription parameter: "blah_blah"" is meaningful. If a user enters
'copy_data' for DROP PUBLICATION, then an error something like
""copy_data" is not allowed for ALTER SUBSCRIPTION ... DROP
PUBLICATION" will be more meaningful. If this understanding is
correct, I wonder we should also have similar change for:

postgres=# ALTER SUBSCRIPTION testsub REFRESH PUBLICATION WITH (refresh=true);
ERROR: unrecognized subscription parameter: "refresh"

postgres=# ALTER SUBSCRIPTION testsub REFRESH PUBLICATION WITH
(synchronous_commit=' ');
ERROR: unrecognized subscription parameter: "synchronous_commit"

postgres=# ALTER SUBSCRIPTION testsub SET (refresh=true);
ERROR: unrecognized subscription parameter: "refresh"

> Well, not really. We're using the phrase "set_publication_option" for all three of SET PUBLICATION, ADD PUBLICATION, and DROP PUBLICATION. Since that's not really supported, we should use it only for the first two, and have a separate "drop_publication_option" for the third.

There's another thread [1], that tries to fix this, where the earlier
suggestion was to drop_publication_option, but later the agreement was
to change the "set_publication_option" to "publication_option", and
had it for SET/ADD/DROP with a note like below. If that doesn't work,
I suggest putting the thoughts there in that thread.
- Additionally, refresh options as described
- under <literal>REFRESH PUBLICATION</literal> may be specified.
+ Additionally, refresh options as described under
<literal>REFRESH PUBLICATION</literal>
+ may be specified, except for <literal>DROP PUBLICATION</literal>.

> Thanks for your response. The docs and error messages still don't look right to me.

I think, for the docs part we can move the discussion to the thread
[1], if you are okay, and have the error message discussion here.

[1] - https://www.postgresql.org/message-id/flat/CALDaNm34qugTr5M0d1JgCgk2Qdo6LZ9UEbTBG%3DTBNV5QADPLWg%40mail.gmail.com

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com