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.

Reported-by: tushar <[email protected]>

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index b2fae027f5f7257e1f79cfcc49500ebfa0095a00..5723be744d7150d491bb4824d4b5c264c29eed04 100644 (file)
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -6609,7 +6609,7 @@ SCRAM-SHA-256$<replaceable>&lt;iteration count&gt;</>:<replaceable>&lt;salt&gt;<
   <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 a3471a044227e879b843dc5ef427eddbc0e5c134..bead99622e4c28d452f001e7b77da9f492a42537 100644 (file)
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -22,7 +22,7 @@ PostgreSQL documentation
  <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 @@ ALTER SUBSCRIPTION <replaceable class="PARAMETER">name</replaceable> RENAME TO <
      <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 @@ ALTER SUBSCRIPTION <replaceable class="PARAMETER">name</replaceable> RENAME TO <
      </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 @@ ALTER SUBSCRIPTION <replaceable class="PARAMETER">name</replaceable> RENAME TO <
    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 86eb31df936d7a44cd707a42576c18a90cf48bdf..ad98b38efe8adc6d899f319dc171fd0eb1ac7959 100644 (file)
--- 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, &copy_data, &synchronous_commit);
+                              &slotname, &copy_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, &copy_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, &copy_data,
-                                          NULL);
+                                          NULL, NULL);
 
                AlterSubscription_refresh(sub, copy_data);
 

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 7e03624eb45a1617b69e7e5767f39b34b612cd13..ada95e5bc372008fd50012321bfa6f2cc21b15b3 100644 (file)
--- 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 8720e713c42cd83abd762398be7884496b5e1e98..2d2e2c0fbc1e9998c591ec8704a6f5431df8b6ad 100644 (file)
--- 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 91ba8ab95a67f232a818662da446eb43fdc0844f..4fcbf7efe97fa63b267bf0fc6fea9527aac92804 100644 (file)
--- 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 4b694a357e621f3c01a3e61015be6a34e25d4447..36fa1bbac806639bbb6871934bd3890e1f59f147 100644 (file)
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -61,7 +61,7 @@ ALTER SUBSCRIPTION testsub CONNECTION 'foobar';
 
 \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 e5638d3322f41818d86c546157c7c2045a0e5162..f9cf5e43920b850b838fa6f6633b24f242778da0 100644 (file)
--- a/src/test/subscription/t/001_rep_changes.pl
+++ b/src/test/subscription/t/001_rep_changes.pl
@@ -143,7 +143,7 @@ $oldpid = $node_publisher->safe_psql('postgres',
    "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';"