On Wed, Oct 2, 2019 at 10:21 PM Pavel Stehule <pavel.stehule@gmail.com> wrote: > > Thank you for careful review. I hope so all issues are out. > > Thanks Pavel for fixing the comments. Few comments: The else part cannot be hit in DropDatabase function as gram.y expects FORCE. + + if (strcmp(opt->defname, "force") == 0) + force = true; + else + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("unrecognized DROP DATABASE option \"%s\"", opt->defname), + parser_errposition(pstate, opt->location))); + } +
I know - but somebody can call DropDatabase function outside parser. So is better check all possibilities.
We should change gram.y to accept any keyword and throw error from DropDatabase function. + */ +drop_option: FORCE + { + $$ = makeDefElem("force", NULL, @1); + } + ;
I spent some time with thinking about it, and I think so this variant (with keyword) is well readable and very illustrative. This will be lost with generic variant.
When the keyword FORCE already exists, then I prefer current state.
"This will also fail" should be "This will fail" + <para> + This will fail, if current user has no permissions to terminate other + connections. Required permissions are the same as with + <literal>pg_terminate_backend</literal>, described + in <xref linkend="functions-admin-signal"/>. + + This will also fail if we are not able to terminate connections or + when there are active prepared transactions or active logical replication + slots. + </para>
fixed
Can we add few tests for this feature.
there are not any other test for DROP DATABASE
We can check syntax later inside second patch (for -f option of dropdb command)
