Re: [HACKERS] ALTER SUBSCRIPTION ..SET PUBLICATION refresh is not throwing error.

2017-06-05 Thread Peter Eisentraut 
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 >> >:
>>>
>>>
>>> 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


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

Re: [HACKERS] ALTER SUBSCRIPTION ..SET PUBLICATION refresh is not throwing error.

2017-06-02 Thread Peter Eisentraut 
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 > >:
>>
>>
>> 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
From 697c4cbdd386a4bd856614de6cedf5af8d1b63be Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 2 Jun 2017 21:59:07 -0400
Subject: [PATCH] Fix ALTER SUBSCRIPTION grammar ambiguity

There was a grammar ambiguity between SET PUBLICATION name REFRESH and
SET PUBLICATION SKIP REFRESH, because SKIP is not a reserved word.  To
resolve that, fold the refresh choice into the WITH options.  Refreshing
is the default now.

Author: tushar 
---
 doc/src/sgml/catalogs.sgml |  2 +-
 doc/src/sgml/ref/alter_subscription.sgml   | 35 --
 src/backend/commands/subscriptioncmds.c| 34 +
 src/backend/parser/gram.y  | 14 ++--
 src/include/nodes/parsenodes.h |  1 -
 src/test/regress/expected/subscription.out |  2 +-
 src/test/regress/sql/subscription.sql  |  2 +-
 src/test/subscription/t/001_rep_changes.pl |  2 +-
 8 files changed, 54 insertions(+), 38 deletions(-)

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index b2fae027f5..5723be744d 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -6609,7 +6609,7 @@ 
pg_subscription_rel
   
This catalog only contains tables known to the subscription after running
either CREATE SUBSCRIPTION or
-   ALTER SUBSCRIPTION ... REFRESH.
+   ALTER SUBSCRIPTION ... REFRESH PUBLICATION.
   
 
   
diff --git a/doc/src/sgml/ref/alter_subscription.sgml 
b/doc/src/sgml/ref/alter_subscription.sgml
index a3471a0442..bead99622e 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -22,7 +22,7 @@
  
 
 ALTER SUBSCRIPTION name 
CONNECTION 'conninfo'
-ALTER SUBSCRIPTION name SET 
PUBLICATION publication_name [, 
...] { REFRESH [ WITH ( refresh_option [= value] [, ... ] ) ] | SKIP REFRESH }
+ALTER SUBSCRIPTION name SET 
PUBLICATION publication_name [, 
...] [ WITH ( set_publication_option [= value] [, ... ] ) ]
 ALTER SUBSCRIPTION name REFRESH 
PUBLICATION [ WITH ( refresh_option [= value] [, ... ] ) ]
 ALTER SUBSCRIPTION name ENABLE
 ALTER SUBSCRIPTION name DISABLE
@@ -80,18 +80,29 @@ Parameters
  
   Changes list of subscribed publications. See
for more information.
+  By default this command will also act like REFRESH
+  PUBLICATION.
  
 
  
-  When REFRESH is specified, this command will also act
-  like REFRESH
-  PUBLICATION.  refresh_option specifies
-  additional options for the refresh operation, as described
-  under REFRESH PUBLICATION.  When
-  SKIP REFRESH is specified, the command will not try
-  to refresh table information.  Note that
-  either REFRESH or SKIP REFRESH
-  must be specified.
+  set_publication_option specifies additional
+  options for this operation.  The supported options are:
+
+  
+   
+refresh (boolean)
+
+ 
+  When false, the command will not try to refresh table information.
+  REFRESH PUBLICATION should then be executed 
separately.
+  The default is true.
+ 
+
+   
+  
+
+  Additionally, refresh options as described
+  under REFRESH PUBLICATION may be specified.
  
 

@@ -107,7 +118,7 @@ Parameters
  
 
  
-  refresh_option specifies additional options for the
+  refresh_option specifies additional options 
for the
   refresh operation.  The supported options are:
 
   
@@ -185,7 +196,7 @@ Examples
Change the publication subscribed by a subscription to
insert_only:
 
-ALTER SUBSCRIPTION mysub SET PUBLICATION insert_only REFRESH;
+ALTER SUBSCRIPTION mysub SET PUBLICATION insert_only;
 
   
 
diff --git a/src/backend/commands/subscriptioncmds.c 
b/src/backend/commands/subscriptioncmds.c
index 86eb31df93..ad98b38efe

Re: [HACKERS] ALTER SUBSCRIPTION ..SET PUBLICATION refresh is not throwing error.

2017-05-27 Thread Petr Jelinek 
On 27/05/17 04:00, Euler Taveira wrote:
> 2017-05-26 21:29 GMT-03:00 Petr Jelinek  >:
> 
> 
> 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


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

Re: [HACKERS] ALTER SUBSCRIPTION ..SET PUBLICATION refresh is not throwing error.

2017-05-26 Thread Euler Taveira 
2017-05-26 21:29 GMT-03:00 Petr Jelinek :

>
> 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.


-- 
   Euler Taveira   Timbira -
http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

Re: [HACKERS] ALTER SUBSCRIPTION ..SET PUBLICATION refresh is not throwing error.

2017-05-26 Thread Petr Jelinek 
On 27/05/17 02:13, Euler Taveira wrote:
> 2017-05-26 17:58 GMT-03:00 Peter Eisentraut
>  >:
> 
> 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


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

Re: [HACKERS] ALTER SUBSCRIPTION ..SET PUBLICATION refresh is not throwing error.

2017-05-26 Thread Euler Taveira 
2017-05-26 17:58 GMT-03:00 Peter Eisentraut <
peter.eisentr...@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).


-- 
   Euler Taveira   Timbira -
http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

Re: [HACKERS] ALTER SUBSCRIPTION ..SET PUBLICATION refresh is not throwing error.

2017-05-26 Thread Peter Eisentraut 
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


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

Re: [HACKERS] ALTER SUBSCRIPTION ..SET PUBLICATION refresh is not throwing error.

2017-05-24 Thread Petr Jelinek 
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 >> >:
>>>
>>>
>>> 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


Remove-the-SKIP-REFRESH-syntax-suggar-in-ALTER-SUBSC-v2.patch
Description: binary/octet-stream

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

Re: [HACKERS] ALTER SUBSCRIPTION ..SET PUBLICATION refresh is not throwing error.

2017-05-24 Thread Petr Jelinek 
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 > >:
>>
>>
>> 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


0001-Remove-the-SKIP-REFRESH-syntax-suggar-in-ALTER-SUBSC.patch
Description: binary/octet-stream

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

Re: [HACKERS] ALTER SUBSCRIPTION ..SET PUBLICATION refresh is not throwing error.

2017-05-24 Thread Petr Jelinek 
On 24/05/17 21:07, Euler Taveira wrote:
> 2017-05-23 6:00 GMT-03:00 tushar  >:
> 
> 
> 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


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

Re: [HACKERS] ALTER SUBSCRIPTION ..SET PUBLICATION refresh is not throwing error.

2017-05-24 Thread Euler Taveira 
2017-05-23 6:00 GMT-03:00 tushar :

>
> 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.

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.


-- 
   Euler Taveira   Timbira -
http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

[HACKERS] ALTER SUBSCRIPTION ..SET PUBLICATION refresh is not throwing error.

2017-05-23 Thread tushar 

Hi,

ALTER SUBSCRIPTION ..SET PUBLICATION  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



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