Improve CREATE SUBSCRIPTION option parsing

When creating a subscription with slot_name = NONE, we failed to check
that also create_slot = false and enabled = false were set.  This
created an invalid subscription and could later lead to a crash if a
NULL slot name was accessed.  Add more checks around that for
robustness.

Reported-by: tushar <tushar.ahuja@enterprisedb.com>

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 89358a4ec3c06f9a3da7287ab26b34bf48c27795..9afdd690789cb637a34e624b7a1aab6bfeeab4ad 100644 (file)
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -168,7 +168,9 @@ parse_subscription_options(List *options, bool *connect, bool *enabled_given,
                                                                         false, 0, false);
                }
                else
-                       elog(ERROR, "unrecognized option: %s", defel->defname);
+                       ereport(ERROR,
+                                       (errcode(ERRCODE_SYNTAX_ERROR),
+                                        errmsg("unrecognized subscription parameter: %s", defel->defname)));
        }
 
        /*
@@ -214,6 +216,16 @@ parse_subscription_options(List *options, bool *connect, bool *enabled_given,
                        ereport(ERROR,
                                        (errcode(ERRCODE_SYNTAX_ERROR),
                                         errmsg("slot_name = NONE and create_slot = true are mutually exclusive options")));
+
+               if (enabled && !*enabled_given && *enabled)
+                       ereport(ERROR,
+                                       (errcode(ERRCODE_SYNTAX_ERROR),
+                                        errmsg("subscription with slot_name = NONE must also set enabled = false")));
+
+               if (create_slot && !create_slot_given && *create_slot)
+                       ereport(ERROR,
+                                       (errcode(ERRCODE_SYNTAX_ERROR),
+                                        errmsg("subscription with slot_name = NONE must also set create_slot = false")));
        }
 }
 

diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 9d1eab9e1e67ec9ee98a727fa2e3376de6e29237..7d1787db5cb3c8bfca8cc57bd703335a0f358cdb 100644 (file)
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -1552,6 +1552,15 @@ ApplyWorkerMain(Datum main_arg)
 
                myslotname = MySubscription->slotname;
 
+               /*
+                * This shouldn't happen if the subscription is enabled, but guard
+                * against DDL bugs or manual catalog changes.  (libpqwalreceiver
+                * will crash if slot is NULL.
+                */
+               if (!myslotname)
+                       ereport(ERROR,
+                                       (errmsg("subscription has no replication slot set")));
+
                /* Setup replication origin tracking. */
                StartTransactionCommand();
                snprintf(originname, sizeof(originname), "pg_%u", MySubscription->oid);

diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 1c42013b4728d2315c56a3507ed0d0766cfbc7af..91ba8ab95a67f232a818662da446eb43fdc0844f 100644 (file)
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -56,6 +56,12 @@ CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpu
 ERROR:  slot_name = NONE and enabled = true are mutually exclusive options
 CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, create_slot = true);
 ERROR:  slot_name = NONE and create_slot = true are mutually exclusive options
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE);
+ERROR:  subscription with slot_name = NONE must also set enabled = false
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = false);
+ERROR:  subscription with slot_name = NONE must also set create_slot = false
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, create_slot = false);
+ERROR:  subscription with slot_name = NONE must also set enabled = false
 -- ok - with slot_name = NONE
 CREATE SUBSCRIPTION testsub3 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, connect = false);
 WARNING:  tables were not subscribed, you will have to run ALTER SUBSCRIPTION ... REFRESH PUBLICATION to subscribe the tables
@@ -82,6 +88,8 @@ ALTER SUBSCRIPTION testsub SET (slot_name = 'newname');
 -- fail
 ALTER SUBSCRIPTION doesnotexist CONNECTION 'dbname=doesnotexist2';
 ERROR:  subscription "doesnotexist" does not exist
+ALTER SUBSCRIPTION testsub SET (create_slot = false);
+ERROR:  unrecognized subscription parameter: create_slot
 \dRs+
                                               List of subscriptions
   Name   |           Owner           | Enabled |     Publication     | Synchronous commit |       Conninfo       

diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 36cdd96c7754f31945096d9741b632b1acab719f..4b694a357e621f3c01a3e61015be6a34e25d4447 100644 (file)
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -44,6 +44,9 @@ CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpu
 CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (connect = false, create_slot = true);
 CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = true);
 CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, create_slot = true);
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE);
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = false);
+CREATE SUBSCRIPTION testsub2 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, create_slot = false);
 
 -- ok - with slot_name = NONE
 CREATE SUBSCRIPTION testsub3 CONNECTION 'dbname=doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, connect = false);
@@ -64,6 +67,7 @@ ALTER SUBSCRIPTION testsub SET (slot_name = 'newname');
 
 -- fail
 ALTER SUBSCRIPTION doesnotexist CONNECTION 'dbname=doesnotexist2';
+ALTER SUBSCRIPTION testsub SET (create_slot = false);
 
 \dRs+