On Sat, May 22, 2021 at 10:22 PM Mark Dilger <mark.dil...@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. 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
