Re: Fixing the docs for ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION - Mailing list pgsql-hackers

On Sat, May 22, 2021 at 10:22 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> My concern isn't that the code is doing the wrong thing, but that the docs and the error messages are confusing.
Thisis particularly troubling given that having a single action which combines the dropping of one publication with the
refreshingof 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
prohibitionis intuitive.  If I want to copy the data for a set of tables on a remote server, and only copy that data
exactlyonce, 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
isdisallowed in this setting. If I then decide to just drop the publication (since my experiment didn't work) and try
todo 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.
Butagain, the command complains that "copy_data" is unrecognized.  At this point, I go back to the docs and it clearly
saysthat "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
DROPPUBLICATION, 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,
andDROP 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