Re: dropdb --force

st 18. 9. 2019 v 4:57 odesílatel Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
napsal:

>
>
> st 18. 9. 2019 v 4:53 odesílatel Ryan Lambert <ryan(at)rustprooflabs(dot)com>
> napsal:
>
>> Hi Pavel,
>> I took a quick look through the patch, I'll try to build and test it
>> tomorrow.
>>
>>
>> --- a/src/include/nodes/parsenodes.h
>> +++ b/src/include/nodes/parsenodes.h
>> @@ -3145,6 +3145,7 @@ typedef struct DropdbStmt
>> NodeTag type;
>> char *dbname; /* database to drop */
>> bool missing_ok; /* skip error if db is missing? */
>> + List *options; /* currently only FORCE is supported */
>> } DropdbStmt;
>>
>> Why put FORCE as the single item in an options list? A bool var seems
>> like it would be more clear and consistent.
>>
>
> see discussion - it was one from Tom's objections. It is due possible
> future enhancing or modification. It can looks strange, because now there
> is only one option, but it allow very easy modifications. More it is
> consistent with lot of other pg commands.
>

I can imagine so somebody will write support for concurrently dropping - so
this list will be longer

>
>
>> - * DROP DATABASE [ IF EXISTS ]
>> + * DROP DATABASE [ ( FORCE ) ] [ IF EXISTS ]
>>
>> Why is it `[ ( FORCE ) ]` instead of `[ FORCE ]`?
>> There are also places in the code that seem like extra () are around
>> FORCE. Like here:
>>
>
> It was discussed before. FORCE is not reserved keyword, so inside list is
> due safety against possible collisions.
>
>
>> + appendPQExpBuffer(&sql, "DROP DATABASE %s%s%s;",
>> + (force ? " (FORCE) " : ""),
>> + (if_exists ? "IF EXISTS " : ""), fmtId(dbname));
>>
>> And here:
>>
>> +$node->safe_psql('postgres', 'CREATE DATABASE foobar2');
>> +$node->issues_sql_like(
>> + [ 'dropdb', '--force', 'foobar2' ],
>> + qr/statement: DROP DATABASE (FORCE) foobar2/,
>> + 'SQL DROP DATABASE (FORCE) run');
>> +
>>
>> I'll try to build and test the patch tomorrow.
>>
>> Thanks,
>>
>> *Ryan Lambert*
>>
>>>
>>>>