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.jeli...@2ndquadrant.com
>> <mailto:petr.jeli...@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
From 697c4cbdd386a4bd856614de6cedf5af8d1b63be Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
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 <tushar.ah...@enterprisedb.com>
---
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 @@
<title><structname>pg_subscription_rel</structname></title>
<para>
This catalog only contains tables known to the subscription after running
either <command>CREATE SUBSCRIPTION</command> or
- <command>ALTER SUBSCRIPTION ... REFRESH</command>.
+ <command>ALTER SUBSCRIPTION ... REFRESH PUBLICATION</command>.
</para>
<table>
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 @@
<refsynopsisdiv>
<synopsis>
ALTER SUBSCRIPTION <replaceable class="PARAMETER">name</replaceable>
CONNECTION '<replaceable>conninfo</replaceable>'
-ALTER SUBSCRIPTION <replaceable class="PARAMETER">name</replaceable> SET
PUBLICATION <replaceable class="PARAMETER">publication_name</replaceable> [,
...] { REFRESH [ WITH ( <replaceable
class="PARAMETER">refresh_option</replaceable> [= <replaceable
class="PARAMETER">value</replaceable>] [, ... ] ) ] | SKIP REFRESH }
+ALTER SUBSCRIPTION <replaceable class="PARAMETER">name</replaceable> SET
PUBLICATION <replaceable class="PARAMETER">publication_name</replaceable> [,
...] [ WITH ( <replaceable
class="PARAMETER">set_publication_option</replaceable> [= <replaceable
class="PARAMETER">value</replaceable>] [, ... ] ) ]
ALTER SUBSCRIPTION <replaceable class="PARAMETER">name</replaceable> REFRESH
PUBLICATION [ WITH ( <replaceable
class="PARAMETER">refresh_option</replaceable> [= <replaceable
class="PARAMETER">value</replaceable>] [, ... ] ) ]
ALTER SUBSCRIPTION <replaceable class="PARAMETER">name</replaceable> ENABLE
ALTER SUBSCRIPTION <replaceable class="PARAMETER">name</replaceable> DISABLE
@@ -80,18 +80,29 @@ <title>Parameters</title>
<para>
Changes list of subscribed publications. See
<xref linkend="SQL-CREATESUBSCRIPTION"> for more information.
+ By default this command will also act like <literal>REFRESH
+ PUBLICATION</literal>.
</para>
<para>
- When <literal>REFRESH</literal> is specified, this command will also act
- like <literal>REFRESH
- PUBLICATION</literal>. <literal>refresh_option</literal> specifies
- additional options for the refresh operation, as described
- under <literal>REFRESH PUBLICATION</literal>. When
- <literal>SKIP REFRESH</literal> is specified, the command will not try
- to refresh table information. Note that
- either <literal>REFRESH</literal> or <literal>SKIP REFRESH</literal>
- must be specified.
+ <replaceable>set_publication_option</replaceable> specifies additional
+ options for this operation. The supported options are:
+
+ <variablelist>
+ <varlistentry>
+ <term><literal>refresh</literal> (<type>boolean</type>)</term>
+ <listitem>
+ <para>
+ When false, the command will not try to refresh table information.
+ <literal>REFRESH PUBLICATION</literal> should then be executed
separately.
+ The default is <literal>true</literal>.
+ </para>
+ </listitem>
+ </varlistentry>
+ </variablelist>
+
+ Additionally, refresh options as described
+ under <literal>REFRESH PUBLICATION</literal> may be specified.
</para>
</listitem>
</varlistentry>
@@ -107,7 +118,7 @@ <title>Parameters</title>
</para>
<para>
- <literal>refresh_option</literal> specifies additional options for the
+ <replaceable>refresh_option</replaceable> specifies additional options
for the
refresh operation. The supported options are:
<variablelist>
@@ -185,7 +196,7 @@ <title>Examples</title>
Change the publication subscribed by a subscription to
<literal>insert_only</literal>:
<programlisting>
-ALTER SUBSCRIPTION mysub SET PUBLICATION insert_only REFRESH;
+ALTER SUBSCRIPTION mysub SET PUBLICATION insert_only;
</programlisting>
</para>
diff --git a/src/backend/commands/subscriptioncmds.c
b/src/backend/commands/subscriptioncmds.c
index 86eb31df93..ad98b38efe 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -64,12 +64,14 @@ static void
parse_subscription_options(List *options, bool *connect, bool *enabled_given,
bool *enabled, bool
*create_slot,
bool *slot_name_given, char
**slot_name,
- bool *copy_data, char
**synchronous_commit)
+ bool *copy_data, char
**synchronous_commit,
+ bool *refresh)
{
ListCell *lc;
bool connect_given = false;
bool create_slot_given = false;
bool copy_data_given = false;
+ bool refresh_given = false;
/* If connect is specified, the others also need to be. */
Assert(!connect || (enabled && create_slot && copy_data));
@@ -92,6 +94,8 @@ parse_subscription_options(List *options, bool *connect, bool
*enabled_given,
*copy_data = true;
if (synchronous_commit)
*synchronous_commit = NULL;
+ if (refresh)
+ *refresh = true;
/* Parse options */
foreach(lc, options)
@@ -167,6 +171,16 @@ parse_subscription_options(List *options, bool *connect,
bool *enabled_given,
PGC_BACKEND, PGC_S_TEST, GUC_ACTION_SET,
false,
0, false);
}
+ else if (strcmp(defel->defname, "refresh") == 0 && refresh)
+ {
+ if (refresh_given)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("conflicting or
redundant options")));
+
+ refresh_given = true;
+ *refresh = defGetBoolean(defel);
+ }
else
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -315,7 +329,8 @@ CreateSubscription(CreateSubscriptionStmt *stmt, bool
isTopLevel)
*/
parse_subscription_options(stmt->options, &connect, &enabled_given,
&enabled,
&create_slot, &slotname_given,
- &slotname,
©_data, &synchronous_commit);
+ &slotname,
©_data, &synchronous_commit,
+ NULL);
/*
* Since creating a replication slot is not transactional, rolling back
@@ -645,7 +660,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt)
parse_subscription_options(stmt->options, NULL,
NULL, NULL,
NULL, &slotname_given, &slotname,
-
NULL, &synchronous_commit);
+
NULL, &synchronous_commit, NULL);
if (slotname_given)
{
@@ -680,7 +695,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt)
parse_subscription_options(stmt->options, NULL,
&enabled_given, &enabled, NULL,
-
NULL, NULL, NULL, NULL);
+
NULL, NULL, NULL, NULL, NULL);
Assert(enabled_given);
if (!sub->slotname && enabled)
@@ -712,13 +727,13 @@ AlterSubscription(AlterSubscriptionStmt *stmt)
break;
case ALTER_SUBSCRIPTION_PUBLICATION:
- case ALTER_SUBSCRIPTION_PUBLICATION_REFRESH:
{
bool copy_data;
+ bool refresh;
parse_subscription_options(stmt->options, NULL,
NULL, NULL,
NULL, NULL, NULL, ©_data,
-
NULL);
+
NULL, &refresh);
values[Anum_pg_subscription_subpublications -
1] =
publicationListToArray(stmt->publication);
@@ -727,12 +742,13 @@ AlterSubscription(AlterSubscriptionStmt *stmt)
update_tuple = true;
/* Refresh if user asked us to. */
- if (stmt->kind ==
ALTER_SUBSCRIPTION_PUBLICATION_REFRESH)
+ if (refresh)
{
if (!sub->enabled)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("ALTER
SUBSCRIPTION ... REFRESH is not allowed for disabled subscriptions")));
+ errmsg("ALTER
SUBSCRIPTION with refresh is not allowed for disabled subscriptions"),
+ errhint("Use
ALTER SUBSCRIPTION ... SET PUBLICATION ... WITH (refresh = false).")));
/* Make sure refresh sees the new list
of publications. */
sub->publications = stmt->publication;
@@ -754,7 +770,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt)
parse_subscription_options(stmt->options, NULL,
NULL, NULL,
NULL, NULL, NULL, ©_data,
-
NULL);
+
NULL, NULL);
AlterSubscription_refresh(sub, copy_data);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 7e03624eb4..ada95e5bc3 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -9279,24 +9279,14 @@ AlterSubscriptionStmt:
n->options = $6;
$$ = (Node *)n;
}
- | ALTER SUBSCRIPTION name SET PUBLICATION
publication_name_list REFRESH opt_definition
- {
- AlterSubscriptionStmt *n =
- makeNode(AlterSubscriptionStmt);
- n->kind =
ALTER_SUBSCRIPTION_PUBLICATION_REFRESH;
- n->subname = $3;
- n->publication = $6;
- n->options = $8;
- $$ = (Node *)n;
- }
- | ALTER SUBSCRIPTION name SET PUBLICATION
publication_name_list SKIP REFRESH
+ | ALTER SUBSCRIPTION name SET PUBLICATION
publication_name_list opt_definition
{
AlterSubscriptionStmt *n =
makeNode(AlterSubscriptionStmt);
n->kind =
ALTER_SUBSCRIPTION_PUBLICATION;
n->subname = $3;
n->publication = $6;
- n->options = NIL;
+ n->options = $7;
$$ = (Node *)n;
}
| ALTER SUBSCRIPTION name ENABLE_P
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 8720e713c4..2d2e2c0fbc 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3382,7 +3382,6 @@ typedef enum AlterSubscriptionType
ALTER_SUBSCRIPTION_OPTIONS,
ALTER_SUBSCRIPTION_CONNECTION,
ALTER_SUBSCRIPTION_PUBLICATION,
- ALTER_SUBSCRIPTION_PUBLICATION_REFRESH,
ALTER_SUBSCRIPTION_REFRESH,
ALTER_SUBSCRIPTION_ENABLED
} AlterSubscriptionType;
diff --git a/src/test/regress/expected/subscription.out
b/src/test/regress/expected/subscription.out
index 91ba8ab95a..4fcbf7efe9 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -82,7 +82,7 @@ ERROR: invalid connection string syntax: missing "=" after
"foobar" in connecti
testsub | regress_subscription_user | f | {testpub} | off
| dbname=doesnotexist
(1 row)
-ALTER SUBSCRIPTION testsub SET PUBLICATION testpub2, testpub3 SKIP REFRESH;
+ALTER SUBSCRIPTION testsub SET PUBLICATION testpub2, testpub3 WITH (refresh =
false);
ALTER SUBSCRIPTION testsub CONNECTION 'dbname=doesnotexist2';
ALTER SUBSCRIPTION testsub SET (slot_name = 'newname');
-- fail
diff --git a/src/test/regress/sql/subscription.sql
b/src/test/regress/sql/subscription.sql
index 4b694a357e..36fa1bbac8 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -61,7 +61,7 @@ CREATE SUBSCRIPTION testsub3 CONNECTION 'dbname=doesnotexist'
PUBLICATION testpu
\dRs+
-ALTER SUBSCRIPTION testsub SET PUBLICATION testpub2, testpub3 SKIP REFRESH;
+ALTER SUBSCRIPTION testsub SET PUBLICATION testpub2, testpub3 WITH (refresh =
false);
ALTER SUBSCRIPTION testsub CONNECTION 'dbname=doesnotexist2';
ALTER SUBSCRIPTION testsub SET (slot_name = 'newname');
diff --git a/src/test/subscription/t/001_rep_changes.pl
b/src/test/subscription/t/001_rep_changes.pl
index e5638d3322..f9cf5e4392 100644
--- a/src/test/subscription/t/001_rep_changes.pl
+++ b/src/test/subscription/t/001_rep_changes.pl
@@ -143,7 +143,7 @@
"SELECT pid FROM pg_stat_replication WHERE application_name =
'$appname';"
);
$node_subscriber->safe_psql('postgres',
-"ALTER SUBSCRIPTION tap_sub SET PUBLICATION tap_pub_ins_only REFRESH WITH
(copy_data = false)"
+"ALTER SUBSCRIPTION tap_sub SET PUBLICATION tap_pub_ins_only WITH (copy_data =
false)"
);
$node_publisher->poll_query_until('postgres',
"SELECT pid != $oldpid FROM pg_stat_replication WHERE application_name =
'$appname';"
--
2.13.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
