Re: dropdb --force

st 2. 10. 2019 v 5:20 odesílatel Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
napsal:

> On Thu, Sep 26, 2019 at 7:18 PM Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
>
>>
>> út 24. 9. 2019 v 14:52 odesílatel Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
>> napsal:
>>
>>>
>>> One other minor comment:
>>> +
>>> + This will also fail, if the connections do not terminate in 5
>>> seconds.
>>> + </para>
>>>
>>> Is there any implementation in the patch for the above note?
>>>
>>
>> Yes, is there.
>>
>> The force flag ensure sending SIGTERM to related clients. Nothing more.
>> There are still check
>>
>> -->if (CountOtherDBBackends(db_id, &notherbackends, &npreparedxacts))
>> <--><-->ereport(ERROR,
>> <--><--><--><-->(errcode(ERRCODE_OBJECT_IN_USE),
>> <--><--><--><--> errmsg("database \"%s\" is being accessed by other
>> users",
>> <--><--><--><--><--><-->dbname),
>> <--><--><--><--> errdetail_busy_db(notherbackends, npreparedxacts)));
>>
>> that can fails after 5 sec. Sending signal doesn't ensure nothing, so I
>> am for no changes in these check.
>>
>
> I think 5 seconds is a hard-coded value that can even change in the
> future. So, it is better to write something more generic like "This will
> also fail if we are not able to terminate connections" or something like
> that. This part of the documentation might change after addressing the
> other comments below.
>

done

> One more point I would like to add here is that I think it is worth
>> considering to split this patch by keeping the changes in dropdb
>> utility as a separate patch. Even though the code is not very much
>> but I think it can be a separate patch atop the main patch which
>> contains the core server changes.
>>
>
> > I did it - last patch contains server side only. I expect so client side
> (very small patch) can be nex
>
> I still see the code related to dropdb utility in the patch. See,
> diff --git a/src/bin/scripts/dropdb.c b/src/bin/scripts/dropdb.c
> index dacd8e5f1d..1bb80fda74 100644
> --- a/src/bin/scripts/dropdb.c
> +++ b/src/bin/scripts/dropdb.c
> @@ -34,6 +34,7 @@ main(int argc, char *argv[])
> {"interactive", no_argument, NULL, 'i'},
> {"if-exists", no_argument, &if_exists, 1},
> {"maintenance-db", required_argument, NULL, 2},
> + {"force", no_argument, NULL, 'f'},
> {NULL, 0, NULL, 0}
> };
>

removed

> Few other comments:
> --------------------------------
> 1.
> +void
> +TerminateOtherDBBackends(Oid databaseId)
> +{
> + ProcArrayStruct *arrayP = procArray;
> + List *pids = NIL;
> + int i;
> +
> + LWLockAcquire(ProcArrayLock, LW_SHARED);
> +
> + for (i = 0; i < procArray->numProcs; i++)
> + {
> + int pgprocno = arrayP->pgprocnos[i];
> + PGPROC *proc = &allProcs[pgprocno];
> +
> + if (proc->databaseId != databaseId)
> + continue;
> + if (proc == MyProc)
> + continue;
> +
> + if (proc->pid != 0)
> + pids = lappend_int(pids, proc->pid);
> + }
>
> So here we are terminating only connection which doesn't have any prepared
> transaction. The behavior of this patch with the prepared transactions
> will be terrible. Basically, after terminating all the connections via
> TerminateOtherDBBackends, we will give error once CountOtherDBBackends is
> invoked. I have tested the same and it gives an error like below:
>
> postgres=# drop database db1 With (Force);
> ERROR: database "db1" is being accessed by other users
> DETAIL: There is 1 prepared transaction using the database.
>
> I think we have two options here:
> (a) Document that even with force option, if there are any prepared
> transactions in the same database, we won't drop the database. Along with
> this, fix the code such that we don't terminate other connections if the
> prepared transactions are active.
> (b) Rollback the prepared transactions.
>

I not use prepared transactions often, and then I have not own strong
opinion about it. Original parch didn't touch this area, so I think we can
continue in this direction (minimally for start).

I did precheck of opened prepared transactions, and when I find any opened,
then I raise a exception (before when I try to terminate other processes).

I updated doc about possible stops.

> I think (a) is a better option because if the number of prepared
> transactions is large, then it might take time and I am not sure if it is
> worth to add the complexity of rolling back all the prepared xacts. OTOH,
> if you or others think option (b) is good and doesn't add much complexity,
> then I think it is worth exploring that option.
>
> I think we should definitely do something to deal with this even if you
> don't like the proposed options because the current behavior of the patch
> seems worse than either of these options.
>
> 2.
> -DROP DATABASE [ IF EXISTS ] <replaceable
> class="parameter">name</replaceable>
> +DROP DATABASE [ IF EXISTS ] <replaceable
> class="parameter">name</replaceable> [ WITH ( <replaceable
> class="parameter">option</replaceable> [, ...] ) ]
>
> It is better to keep WITH clause as optional similar to Copy command.
> This might address Peter E's concern related to WITH clause.
>

WITH keyword is optional now

>
> 3.
> - * DROP DATABASE [ IF EXISTS ]
> + * DROP DATABASE [ ( options ) ] [ IF EXISTS ]
>

fixed

> You seem to forget to change the syntax in the above comments after
> changing the patch.
>
> 4.
> + If anyone else is connected to the target database, this command will
> fail
> + - unless you use the <literal>FORCE</literal> option described below.
>
> I don't understand the significance of using '-' before unless. I think
> we can remove it.
>

fixed

> Changed the patch status as 'Waiting on Author'.
>

Thank you for careful review. I hope so all issues are out.

Regards

Pavel

>
> --
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>