On Sat, Sep 21, 2019 at 10:09 PM Pavel Stehule <pavel.stehule@gmail.com> wrote: > fixed > > Thank you for check. I am sending updated patch > Thanks for fixing all the comments. Couple of suggestions: -DROP DATABASE [ IF EXISTS ] <replaceable class="parameter">name</replaceable> +DROP DATABASE [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] [ IF EXISTS ] <replaceable class="parameter">name</replaceable> + +<phrase>where <replaceable class="parameter">option</replaceable> can be one of:</phrase> + + FORCE
Currently only force option is supported in: drop database (force) dbname Should we have a list or a single element for this? I'm not sure if we have any plans to add more option in this area.
If we open door for next possible syntax enhancing and it is motivation for this syntax, then we should to use pattern for this situation.
It looks little bit strange, but I think so current code is very well readable. I wrote comment, so only one flag is supported now, but
syntax allows add other flags. I don't think so using defelem directly reduces significantly enough lines - just if we implement some
what looks like possible list, then we should to use lists inside.
If possible we can add an error message like 'ERROR: unrecognized drop database option "force1"' if an invalid input is given. The above error message will be inline with error message of vacuum's error message whose syntax is similar to the current feature. We could throw the error from here: case T_DropdbStmt: { DropdbStmt *stmt = (DropdbStmt *) parsetree; + bool force = false; + ListCell *lc; + + foreach(lc, stmt->options) + { + DefElem *item = (DefElem *) lfirst(lc); + + if (strcmp(item->defname, "force") == 0) + force = true; + } Thoughts?
I moved this check to separate function DropDatabase with new check and exception like you proposed.