Thread: [HACKERS] ALTER SUBSCRIPTION ..SET PUBLICATION refresh is notthrowing error.

Hi,

ALTER SUBSCRIPTION ..SET PUBLICATION <no name> refresh  is removing all 
the attached subscription('s).

X Machine -

s=# create table t(n int);
CREATE TABLE
s=# create table t1(n int);
CREATE TABLE
s=# create publication pub for table  t,t1;
CREATE PUBLICATION
s=#

Y Machine -

s=# create table t(n int);
CREATE TABLE
s=# create table t1(n int);
CREATE TABLE
s=# create subscription s1 connection 'dbname=s port=5432 user=centos 
host=localhost' publication pub;
NOTICE:  synchronized table states
NOTICE:  created replication slot "s1" on publisher
CREATE SUBSCRIPTION

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=#

I think - this is probably due to not given publication NAME in the sql 
query .

we are doing a syntax check at the time of REFRESH but not with SKIP 
REFRESH

s=# alter subscription s1 set publication   refresh ;
ERROR:  syntax error at or near ";"
LINE 1: alter subscription s1 set publication   refresh ;                                                        ^
s=# alter subscription s1 set publication  skip refresh ;
ALTER SUBSCRIPTION
s=#

-- 
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

On 24/05/17 21:07, Euler Taveira wrote:
> 2017-05-23 6:00 GMT-03:00 tushar <tushar.ahuja@enterprisedb.com
> <mailto:tushar.ahuja@enterprisedb.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.

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

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.ahuja@enterprisedb.com
>> <mailto:tushar.ahuja@enterprisedb.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.

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.ahuja@enterprisedb.com
>>> <mailto:tushar.ahuja@enterprisedb.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 5/24/17 15:38, Petr Jelinek wrote:
>>> 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...

Do we want not-refreshing to be the default behavior?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 27/05/17 02:13, Euler Taveira wrote:
> 2017-05-26 17:58 GMT-03:00 Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com
> <mailto:peter.eisentraut@2ndquadrant.com>>:
> 
>     On 5/24/17 15:38, Petr Jelinek wrote:
>     >>> 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...
> 
>     Do we want not-refreshing to be the default behavior?
> 
> 
> It is a different behavior from the initial proposal. However, we
> fortunately have ALTER SUBSCRIPTION foo REFRESH PUBLICATION and can
> refresh later. Also, if "refresh" is more popular than "skip", it is
> just a small word in the command. That's the price we pay to avoid
> ambiguity that the previous syntax had.At least I think Petr's proposal
> is less confusing than mine (my proposal maintains current behavior but
> can cause some confusion).
> 

Actually another possibility would be to remove the REFRESH keyword
completely and just have [ WITH (...) ] and have the refresh option
there, ie simplified version of what you have suggested (without the
ugliness of specifying refresh twice to disable).

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

On 27/05/17 04:00, Euler Taveira wrote:
> 2017-05-26 21:29 GMT-03:00 Petr Jelinek <petr.jelinek@2ndquadrant.com
> <mailto:petr.jelinek@2ndquadrant.com>>:
> 
> 
>     Actually another possibility would be to remove the REFRESH keyword
>     completely and just have [ WITH (...) ] and have the refresh option
>     there, ie simplified version of what you have suggested (without the
>     ugliness of specifying refresh twice to disable).
> 
> 
> It will cause confusion. It seems that WITH sets ALTER SUBSCRIPTION
> properties. Indeed, they are REFRESH properties. I think we shouldn't
> exclude REFRESH keyword. Syntax leaves no doubt that WITH are REFRESH
> properties.
> 

Maybe, I don't know, it might not be that confusing when SET PUBLICATION
and REFRESH PUBLICATION have same set of WITH options.

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

On 5/27/17 06:54, Petr Jelinek wrote:
> On 27/05/17 04:00, Euler Taveira wrote:
>> 2017-05-26 21:29 GMT-03:00 Petr Jelinek <petr.jelinek@2ndquadrant.com
>> <mailto:petr.jelinek@2ndquadrant.com>>:
>>
>>
>>     Actually another possibility would be to remove the REFRESH keyword
>>     completely and just have [ WITH (...) ] and have the refresh option
>>     there, ie simplified version of what you have suggested (without the
>>     ugliness of specifying refresh twice to disable).
>>
>>
>> It will cause confusion. It seems that WITH sets ALTER SUBSCRIPTION
>> properties. Indeed, they are REFRESH properties. I think we shouldn't
>> exclude REFRESH keyword. Syntax leaves no doubt that WITH are REFRESH
>> properties.
>>
> 
> Maybe, I don't know, it might not be that confusing when SET PUBLICATION
> and REFRESH PUBLICATION have same set of WITH options.

I'm not sure what the conclusion from the above discussion was supposed
to be, but here is a patch.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 6/2/17 22:13, Peter Eisentraut wrote:
> On 5/27/17 06:54, Petr Jelinek wrote:
>> On 27/05/17 04:00, Euler Taveira wrote:
>>> 2017-05-26 21:29 GMT-03:00 Petr Jelinek <petr.jelinek@2ndquadrant.com
>>> <mailto:petr.jelinek@2ndquadrant.com>>:
>>>
>>>
>>>     Actually another possibility would be to remove the REFRESH keyword
>>>     completely and just have [ WITH (...) ] and have the refresh option
>>>     there, ie simplified version of what you have suggested (without the
>>>     ugliness of specifying refresh twice to disable).
>>>
>>>
>>> It will cause confusion. It seems that WITH sets ALTER SUBSCRIPTION
>>> properties. Indeed, they are REFRESH properties. I think we shouldn't
>>> exclude REFRESH keyword. Syntax leaves no doubt that WITH are REFRESH
>>> properties.
>>>
>>
>> Maybe, I don't know, it might not be that confusing when SET PUBLICATION
>> and REFRESH PUBLICATION have same set of WITH options.
> 
> I'm not sure what the conclusion from the above discussion was supposed
> to be, but here is a patch.

committed

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services