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

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>

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
+      <replaceable>set_publication_option</replaceable> specifies additional
-      like <literal>REFRESH
+      options for this operation.  The supported options are:
-      PUBLICATION</literal>.  <literal>refresh_option</literal> specifies
+
-      additional options for the refresh operation, as described
+      <variablelist>
-      under <literal>REFRESH PUBLICATION</literal>.  When
+       <varlistentry>
-      <literal>SKIP REFRESH</literal> is specified, the command will not try
+        <term><literal>refresh</literal> (<type>boolean</type>)</term>
-      to refresh table information.  Note that
+        <listitem>
-      either <literal>REFRESH</literal> or <literal>SKIP REFRESH</literal>
+         <para>
-      must be specified.
+          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>
 

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);
 

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
+			| ALTER SUBSCRIPTION name SET PUBLICATION publication_name_list 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
 				{
 					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

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;

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

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');
 

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';"