Re: ALTER SUBSCRIPTION ..SET PUBLICATION <no name> refresh is not throwing error.

On 24/05/17 21:28, Petr Jelinek wrote:
> On 24/05/17 21:21, Petr Jelinek wrote:
>> On 24/05/17 21:07, Euler Taveira wrote:
>>> 2017-05-23 6:00 GMT-03:00 tushar <tushar(dot)ahuja(at)enterprisedb(dot)com
>>> <mailto:tushar(dot)ahuja(at)enterprisedb(dot)com>>:
>>>
>>>
>>> s=# alter subscription s1 set publication skip refresh ;
>>> NOTICE: removed subscription for table public.t
>>> NOTICE: removed subscription for table public.t1
>>> ALTER SUBSCRIPTION
>>> s=#
>>>
>>>
>>> That's a design flaw. Since SKIP is not a reserved word, parser consider
>>> it as a publication name. Instead of causing an error, it executes
>>> another command (REFRESH) that is the opposite someone expects. Also, as
>>> "skip" is not a publication name, it removes all tables in the subscription.
>>>
>>
>> Ah that explains why I originally added the ugly NOREFRESH keyword.
>>
>>> ALTER SUBSCRIPTION name SET PUBLICATION publication_name_list SKIP REFRESH
>>> ALTER SUBSCRIPTION name SET PUBLICATION publication_name_list REFRESH
>>> opt_definition
>>>
>>> I think the first command was a bad design. Why don't we transform SKIP
>>> REFRESH into a REFRESH option?
>>>
>>> ALTER SUBSCRIPTION sub1 SET PUBLICATION pub1 REFRESH WITH (skip = true);
>>>
>>> skip (boolean): specifies that the command will not try to refresh table
>>> information. The default is false.
>>
>> That's quite confusing IMHO, saying REFRESH but then adding option to
>> actually not refresh is not a good interface.
>>
>> I wonder if we actually need the SKIP REFRESH syntax, there is the
>> "REFRESH [ WITH ... ]" when user wants to refresh, so if REFRESH is not
>> specified, we can just behave as if SKIP REFRESH was used, it's not like
>> there is 3rd possible behavior.
>>
>
> Attached patch does exactly that.
>

And of course I forgot to update docs...

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