Re: dropdb --force

so 19. 10. 2019 v 12:37 odesílatel Amit Kapila <amit.kapila16@gmail.com>
napsal:

> On Fri, Oct 4, 2019 at 9:54 PM Pavel Stehule <pavel.stehule@gmail.com>
> wrote:
> >
> >>
> >> Can we add few tests for this feature.
> >
> > there are not any other test for DROP DATABASE
> >
>
> I think there are no dedicated tests for it but in a few tests, we use
> it like in contrib\sepgsql\sql\alter.sql.  I am not sure if we can
> write a predictable test for force option because it will never be
> guaranteed to drop the database in the presence of other active
> sessions.
>

done - I push tests to /tests/regress/psql.sql


> Few more comments:
> ---------------------------------
> 1.
> + if (nprepared > 0)
> + ereport(ERROR,
> + (errcode(ERRCODE_OBJECT_IN_USE),
> + errmsg("database \"%s\" is used by %d prepared transaction(s)",
> + get_database_name(databaseId),
> + nprepared)));
> +
>
> You need to use errdetail_plural here to avoid translation problems,
> see docs[1] for a detailed explanation.  You can use function
> errdetail_busy_db.  Also, the indentation is not proper.
>

fixed


> 2.
> TerminateOtherDBBackends()
> {
> ..
> + /* We know so we have all necessary rights now */
> + foreach (lc, pids)
> + {
> + int pid = lfirst_int(lc);
> + PGPROC    *proc = BackendPidGetProc(pid);
> +
> + if (proc != NULL)
> + {
> + /* If we have setsid(), signal the backend's whole process group */
> +#ifdef HAVE_SETSID
> + (void) kill(-pid, SIGTERM);
> +#else
> + (void) kill(pid, SIGTERM);
> +#endif
> + }
> + }
> +
> + /* sleep 100ms */
> + pg_usleep(100 * 1000L);
> ..
> }
>
> So here we are sending SIGTERM to all the processes and wait for 100ms
> to allow them to exit.  Have you tested this with many processes?  I
> think we can create 100~500 sessions or maybe more to the database
> being dropped and see what is behavior?  One thing to notice is that
> in function CountOtherDBBackends() we are sending SIGTERM to 10
> autovacuum processes at-a-time.  That has been introduced in commit
> 4abd7b49f1e9, but the reason for the same is not mentioned.  I am not
> sure if it is to avoid sending SIGTERM to many processes in quick
> succession.
>

I tested it on linux Linux nemesis

5.3.6-300.fc31.x86_64 #1 SMP Mon Oct 14 12:26:42 UTC 2019 x86_64 x86_64
x86_64 GNU/Linux

Tested with 1800 connections without any problem (under low load (only
pg_sleep was called).


> I think there should be more comments atop TerminateOtherDBBackends to
> explain the working of it and some assumptions of that function.
>

done


> 3.
> +opt_drop_option_list:
> + opt_with '(' drop_option_list ')' { $$ = $3; }
> + | /* EMPTY */
>
> I think it is better to keep opt_with as part of the main syntax
> rather than clubbing it with drop_option_list as we have in other
> cases in the code.
>

done


> 4.
> +drop_option: FORCE
> + {
> + $$ = makeDefElem("force", NULL, @1);
> + }
> + ;
>
> We generally keep the option name "FORCE" in the new line.
>

done


> 5.  I think it is better if can support tab-completion for this feature.
>

done

I am sending fresh patch

Regards

Pavel


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