On Fri, Oct 4, 2019 at 9:54 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
>
>
>
> čt 3. 10. 2019 v 19:48 odesílatel vignesh C <vignesh21@gmail.com> napsal:
>>
>> 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)
>
Changes in this patch looks fine to me.
I'm not sure if we have forgotten to miss attaching the second patch
or can you provide the link to second patch.
Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com
