Обсуждение: Use atexit() in initdb and pg_basebackup
initdb and pg_basebackup can use atexit() to register cleanup actions instead of requiring the use of custom exit_nicely() etc. Patches attached. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On 2018-Dec-29, Peter Eisentraut wrote:
> @@ -387,6 +388,7 @@ StreamLog(void)
> if (!conn)
> /* Error message already written in GetConnection() */
> return;
> + atexit(disconnect_atexit);
>
> if (!CheckServerVersionForStreaming(conn))
> {
Seems you're registering the atexit cb twice here; you should only do so
in the first "!conn" block.
It would be nicer to be able to call atexit() in GetConnection() instead
of at each callsite, but that would require a place to save each conn
struct into, which is probably more work than warranted.
> @@ -3438,5 +3437,8 @@ main(int argc, char *argv[])
>
> destroyPQExpBuffer(start_db_cmd);
>
> + /* prevent cleanup */
> + made_new_pgdata = found_existing_pgdata = made_new_xlogdir = found_existing_xlogdir = false;
> +
> return 0;
> }
This is a bit ugly, but meh.
Other than the first point, LGTM.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Jan 04, 2019 at 04:35:51PM -0300, Alvaro Herrera wrote: > On 2018-Dec-29, Peter Eisentraut wrote: >> @@ -3438,5 +3437,8 @@ main(int argc, char *argv[]) >> >> destroyPQExpBuffer(start_db_cmd); >> >> + /* prevent cleanup */ >> + made_new_pgdata = found_existing_pgdata = made_new_xlogdir = found_existing_xlogdir = false; >> + >> return 0; >> } > > This is a bit ugly, but meh. > > Other than the first point, LGTM. Re-meuh (French version). That's partially a problem of this patch because all those flags get reset. I think that it would be cleaner to replace all those boolean flags with just a simple bits16 or such, making the flag cleanup reset way cleaner, and less error-prone if more flag types are added in the future. -- Michael
Вложения
On 04/01/2019 20:35, Alvaro Herrera wrote: > Seems you're registering the atexit cb twice here; you should only do so > in the first "!conn" block. OK, fixed. >> @@ -3438,5 +3437,8 @@ main(int argc, char *argv[]) >> >> destroyPQExpBuffer(start_db_cmd); >> >> + /* prevent cleanup */ >> + made_new_pgdata = found_existing_pgdata = made_new_xlogdir = found_existing_xlogdir = false; >> + >> return 0; >> } > > This is a bit ugly, but meh. Yeah. Actually, we already have a solution of this in pg_basebackup, with a bool success variable. I rewrote it like that. At least it's better for uniformity. I also added an atexit() conversion in isolationtester. It's mostly the same thing. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On 2019-Jan-05, Peter Eisentraut wrote: > On 04/01/2019 20:35, Alvaro Herrera wrote: > >> + /* prevent cleanup */ > >> + made_new_pgdata = found_existing_pgdata = made_new_xlogdir = found_existing_xlogdir = false; > >> + > >> return 0; > >> } > > > > This is a bit ugly, but meh. > > Yeah. Actually, we already have a solution of this in pg_basebackup, > with a bool success variable. I rewrote it like that. At least it's > better for uniformity. Ah, yeah, much better, LGTM. > I also added an atexit() conversion in isolationtester. It's mostly the > same thing. LGTM in a quick eyeball. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 05/01/2019 16:42, Alvaro Herrera wrote: >> Yeah. Actually, we already have a solution of this in pg_basebackup, >> with a bool success variable. I rewrote it like that. At least it's >> better for uniformity. > > Ah, yeah, much better, LGTM. > >> I also added an atexit() conversion in isolationtester. It's mostly the >> same thing. > > LGTM in a quick eyeball. committed -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services