From 62345698513cbcb3c48a6dae414abf0f24fd163a Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Wed, 17 May 2017 20:47:37 -0400 Subject: [PATCH] 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 --- src/backend/commands/subscriptioncmds.c | 14 +++++++++++++- src/backend/replication/logical/worker.c | 9 +++++++++ src/test/regress/expected/subscription.out | 8 ++++++++ src/test/regress/sql/subscription.sql | 4 ++++ 4 files changed, 34 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c index 89358a4ec3..9afdd69078 100644 --- 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 9d1eab9e1e..7d1787db5c 100644 --- 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 1c42013b47..91ba8ab95a 100644 --- 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 36cdd96c77..4b694a357e 100644 --- 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+