Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hQJnf-0002N0-HE for pgsql-hackers@arkaria.postgresql.org; Mon, 13 May 2019 22:52:03 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1hQJne-0007d0-2A for pgsql-hackers@arkaria.postgresql.org; Mon, 13 May 2019 22:52:02 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hQJnd-0007cs-IW for pgsql-hackers@lists.postgresql.org; Mon, 13 May 2019 22:52:01 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hQJna-0002Y9-OP for pgsql-hackers@postgresql.org; Mon, 13 May 2019 22:52:00 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.14.4/8.14.4) with ESMTP id x4DMpsg2003972; Mon, 13 May 2019 18:51:54 -0400 From: Tom Lane To: Alvaro Herrera cc: Peter Eisentraut , Ian Barwick , "pgsql-hackers@postgresql.org" Subject: Re: PG12, PGXS and linking pgfeutils In-reply-to: <20190513195851.GA10077@alvherre.pgsql> References: <20190513195851.GA10077@alvherre.pgsql> Comments: In-reply-to Alvaro Herrera message dated "Mon, 13 May 2019 15:58:51 -0400" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <3946.1557787873.0@sss.pgh.pa.us> Date: Mon, 13 May 2019 18:51:54 -0400 Message-ID: <3971.1557787914@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <3946.1557787873.1@sss.pgh.pa.us> Alvaro Herrera writes: > On 2019-May-13, Tom Lane wrote: >> I started working on a patch to do that, and soon noticed that there >> are pre-existing files logging.[hc] in src/bin/pg_rewind/. This seems >> like a Bad Thing, in fact the #includes in pg_rewind/ are already a >> little confused due to this. I think we should either rename those >> two pg_rewind files to something else, or rename the generic ones, >> perhaps to "fe_logging.[hc]". The latter could be done nearly >> trivially as part of the movement patch, but on cosmetic grounds >> I'd be more inclined to do the former instead. Thoughts? > I'd rename both :-) On closer inspection, there's so little left in pg_rewind's logging.h/.c (one function and a couple of global variables) that the better answer is probably just to move those objects somewhere else and nuke the separate files altogether. As attached. regards, tom lane ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="get-rid-of-pg_rewind-logging-files.patch"; charset="us-ascii" Content-ID: <3946.1557787873.2@sss.pgh.pa.us> Content-Description: get-rid-of-pg_rewind-logging-files.patch Content-Transfer-Encoding: quoted-printable diff --git a/src/bin/pg_rewind/Makefile b/src/bin/pg_rewind/Makefile index 019e199..5455fd4 100644 --- a/src/bin/pg_rewind/Makefile +++ b/src/bin/pg_rewind/Makefile @@ -19,7 +19,7 @@ override CPPFLAGS :=3D -I$(libpq_srcdir) -DFRONTEND $(CP= PFLAGS) LDFLAGS_INTERNAL +=3D -L$(top_builddir)/src/fe_utils -lpgfeutils $(libpq_= pgport) = OBJS =3D pg_rewind.o parsexlog.o xlogreader.o datapagemap.o timeline.o \ - fetch.o file_ops.o copy_fetch.o libpq_fetch.o filemap.o logging.o \ + fetch.o file_ops.o copy_fetch.o libpq_fetch.o filemap.o \ $(WIN32RES) = EXTRA_CLEAN =3D xlogreader.c diff --git a/src/bin/pg_rewind/copy_fetch.c b/src/bin/pg_rewind/copy_fetch= .c index 3fd0404..2ada861 100644 --- a/src/bin/pg_rewind/copy_fetch.c +++ b/src/bin/pg_rewind/copy_fetch.c @@ -18,7 +18,6 @@ #include "fetch.h" #include "file_ops.h" #include "filemap.h" -#include "logging.h" #include "pg_rewind.h" = static void recurse_dir(const char *datadir, const char *path, diff --git a/src/bin/pg_rewind/file_ops.c b/src/bin/pg_rewind/file_ops.c index e442f93..f9e41b1 100644 --- a/src/bin/pg_rewind/file_ops.c +++ b/src/bin/pg_rewind/file_ops.c @@ -21,7 +21,6 @@ #include "common/file_perm.h" #include "file_ops.h" #include "filemap.h" -#include "logging.h" #include "pg_rewind.h" = /* diff --git a/src/bin/pg_rewind/filemap.c b/src/bin/pg_rewind/filemap.c index 63d0bae..813eadc 100644 --- a/src/bin/pg_rewind/filemap.c +++ b/src/bin/pg_rewind/filemap.c @@ -15,12 +15,10 @@ = #include "datapagemap.h" #include "filemap.h" -#include "logging.h" #include "pg_rewind.h" = #include "common/string.h" #include "catalog/pg_tablespace_d.h" -#include "fe_utils/logging.h" #include "storage/fd.h" = filemap_t *filemap =3D NULL; diff --git a/src/bin/pg_rewind/libpq_fetch.c b/src/bin/pg_rewind/libpq_fet= ch.c index 11ec045..b6fa7e5 100644 --- a/src/bin/pg_rewind/libpq_fetch.c +++ b/src/bin/pg_rewind/libpq_fetch.c @@ -19,12 +19,10 @@ #include "fetch.h" #include "file_ops.h" #include "filemap.h" -#include "logging.h" = #include "libpq-fe.h" #include "catalog/pg_type_d.h" #include "fe_utils/connect.h" -#include "fe_utils/logging.h" #include "port/pg_bswap.h" = static PGconn *conn =3D NULL; diff --git a/src/bin/pg_rewind/logging.c b/src/bin/pg_rewind/logging.c deleted file mode 100644 index 8169f73..0000000 --- a/src/bin/pg_rewind/logging.c +++ /dev/null @@ -1,79 +0,0 @@ -/*-----------------------------------------------------------------------= -- - * - * logging.c - * logging functions - * - * Copyright (c) 2010-2019, PostgreSQL Global Development Group - * - *-----------------------------------------------------------------------= -- - */ -#include "postgres_fe.h" - -#include -#include - -#include "pg_rewind.h" -#include "logging.h" - -#include "pgtime.h" - -/* Progress counters */ -uint64 fetch_size; -uint64 fetch_done; - -static pg_time_t last_progress_report =3D 0; - - -/* - * Print a progress report based on the global variables. - * - * Progress report is written at maximum once per second, unless the - * force parameter is set to true. - */ -void -progress_report(bool force) -{ - int percent; - char fetch_done_str[32]; - char fetch_size_str[32]; - pg_time_t now; - - if (!showprogress) - return; - - now =3D time(NULL); - if (now =3D=3D last_progress_report && !force) - return; /* Max once per second */ - - last_progress_report =3D now; - percent =3D fetch_size ? (int) ((fetch_done) * 100 / fetch_size) : 0; - - /* - * Avoid overflowing past 100% or the full size. This may make the total - * size number change as we approach the end of the backup (the estimate - * will always be wrong if WAL is included), but that's better than havi= ng - * the done column be bigger than the total. - */ - if (percent > 100) - percent =3D 100; - if (fetch_done > fetch_size) - fetch_size =3D fetch_done; - - /* - * Separate step to keep platform-dependent format code out of - * translatable strings. And we only test for INT64_FORMAT availability - * in snprintf, not fprintf. - */ - snprintf(fetch_done_str, sizeof(fetch_done_str), INT64_FORMAT, - fetch_done / 1024); - snprintf(fetch_size_str, sizeof(fetch_size_str), INT64_FORMAT, - fetch_size / 1024); - - fprintf(stderr, _("%*s/%s kB (%d%%) copied"), - (int) strlen(fetch_size_str), fetch_done_str, fetch_size_str, - percent); - if (isatty(fileno(stderr))) - fprintf(stderr, "\r"); - else - fprintf(stderr, "\n"); -} diff --git a/src/bin/pg_rewind/logging.h b/src/bin/pg_rewind/logging.h deleted file mode 100644 index 81e17ac..0000000 --- a/src/bin/pg_rewind/logging.h +++ /dev/null @@ -1,25 +0,0 @@ -/*-----------------------------------------------------------------------= -- - * - * logging.h - * prototypes for logging functions - * - * - * Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group - * Portions Copyright (c) 1994, Regents of the University of California - * - *-----------------------------------------------------------------------= -- - */ -#ifndef PG_REWIND_LOGGING_H -#define PG_REWIND_LOGGING_H - -#include "fe_utils/logging.h" - -/* progress counters */ -extern uint64 fetch_size; -extern uint64 fetch_done; - -extern void progress_report(bool force); - -#define pg_fatal(...) do { pg_log_fatal(__VA_ARGS__); exit(1); } while(0) - -#endif /* PG_REWIND_LOGGING_H */ diff --git a/src/bin/pg_rewind/nls.mk b/src/bin/pg_rewind/nls.mk index 58c9ede..ba29d62 100644 --- a/src/bin/pg_rewind/nls.mk +++ b/src/bin/pg_rewind/nls.mk @@ -1,7 +1,7 @@ # src/bin/pg_rewind/nls.mk CATALOG_NAME =3D pg_rewind AVAIL_LANGUAGES =3Dde es fr it ja ko pl pt_BR ru sv tr zh_CN -GETTEXT_FILES =3D $(FRONTEND_COMMON_GETTEXT_FILES) copy_fetch.c datapa= gemap.c fetch.c file_ops.c filemap.c libpq_fetch.c logging.c parsexlog.c p= g_rewind.c timeline.c ../../common/fe_memutils.c ../../common/restricted_t= oken.c xlogreader.c +GETTEXT_FILES =3D $(FRONTEND_COMMON_GETTEXT_FILES) copy_fetch.c datapa= gemap.c fetch.c file_ops.c filemap.c libpq_fetch.c parsexlog.c pg_rewind.c= timeline.c ../../common/fe_memutils.c ../../common/restricted_token.c xlo= greader.c GETTEXT_TRIGGERS =3D $(FRONTEND_COMMON_GETTEXT_TRIGGERS) pg_fatal report_= invalid_record:2 GETTEXT_FLAGS =3D $(FRONTEND_COMMON_GETTEXT_FLAGS) \ pg_fatal:1:c-format \ diff --git a/src/bin/pg_rewind/parsexlog.c b/src/bin/pg_rewind/parsexlog.c index 7709b96..65e523f 100644 --- a/src/bin/pg_rewind/parsexlog.c +++ b/src/bin/pg_rewind/parsexlog.c @@ -15,7 +15,6 @@ = #include "pg_rewind.h" #include "filemap.h" -#include "logging.h" = #include "access/rmgr.h" #include "access/xlog_internal.h" diff --git a/src/bin/pg_rewind/pg_rewind.c b/src/bin/pg_rewind/pg_rewind.c index 3702efa..29a0eb5 100644 --- a/src/bin/pg_rewind/pg_rewind.c +++ b/src/bin/pg_rewind/pg_rewind.c @@ -18,7 +18,6 @@ #include "fetch.h" #include "file_ops.h" #include "filemap.h" -#include "logging.h" = #include "access/timeline.h" #include "access/xlog_internal.h" @@ -28,7 +27,6 @@ #include "common/file_perm.h" #include "common/file_utils.h" #include "common/restricted_token.h" -#include "fe_utils/logging.h" #include "getopt_long.h" #include "storage/bufpage.h" = @@ -63,6 +61,12 @@ bool do_sync =3D true; TimeLineHistoryEntry *targetHistory; int targetNentries; = +/* Progress counters */ +uint64 fetch_size; +uint64 fetch_done; +static pg_time_t last_progress_report =3D 0; + + static void usage(const char *progname) { @@ -446,6 +450,60 @@ sanityChecks(void) } = /* + * Print a progress report based on the global variables. + * + * Progress report is written at maximum once per second, unless the + * force parameter is set to true. + */ +void +progress_report(bool force) +{ + int percent; + char fetch_done_str[32]; + char fetch_size_str[32]; + pg_time_t now; + + if (!showprogress) + return; + + now =3D time(NULL); + if (now =3D=3D last_progress_report && !force) + return; /* Max once per second */ + + last_progress_report =3D now; + percent =3D fetch_size ? (int) ((fetch_done) * 100 / fetch_size) : 0; + + /* + * Avoid overflowing past 100% or the full size. This may make the total + * size number change as we approach the end of the backup (the estimate + * will always be wrong if WAL is included), but that's better than havi= ng + * the done column be bigger than the total. + */ + if (percent > 100) + percent =3D 100; + if (fetch_done > fetch_size) + fetch_size =3D fetch_done; + + /* + * Separate step to keep platform-dependent format code out of + * translatable strings. And we only test for INT64_FORMAT availability + * in snprintf, not fprintf. + */ + snprintf(fetch_done_str, sizeof(fetch_done_str), INT64_FORMAT, + fetch_done / 1024); + snprintf(fetch_size_str, sizeof(fetch_size_str), INT64_FORMAT, + fetch_size / 1024); + + fprintf(stderr, _("%*s/%s kB (%d%%) copied"), + (int) strlen(fetch_size_str), fetch_done_str, fetch_size_str, + percent); + if (isatty(fileno(stderr))) + fprintf(stderr, "\r"); + else + fprintf(stderr, "\n"); +} + +/* * Find minimum from two WAL locations assuming InvalidXLogRecPtr means * infinity as src/include/access/timeline.h states. This routine should * be used only when comparing WAL locations related to history files. diff --git a/src/bin/pg_rewind/pg_rewind.h b/src/bin/pg_rewind/pg_rewind.h index f2cc280..9e6f947 100644 --- a/src/bin/pg_rewind/pg_rewind.h +++ b/src/bin/pg_rewind/pg_rewind.h @@ -17,6 +17,8 @@ #include "storage/block.h" #include "storage/relfilenode.h" = +#include "fe_utils/logging.h" + /* Configuration options */ extern char *datadir_target; extern char *datadir_source; @@ -29,6 +31,13 @@ extern int WalSegSz; extern TimeLineHistoryEntry *targetHistory; extern int targetNentries; = +/* Progress counters */ +extern uint64 fetch_size; +extern uint64 fetch_done; + +/* logging support */ +#define pg_fatal(...) do { pg_log_fatal(__VA_ARGS__); exit(1); } while(0) + /* in parsexlog.c */ extern void extractPageMap(const char *datadir, XLogRecPtr startpoint, int tliIndex, XLogRecPtr endpoint); @@ -39,6 +48,9 @@ extern void findLastCheckpoint(const char *datadir, XLog= RecPtr searchptr, extern XLogRecPtr readOneRecord(const char *datadir, XLogRecPtr ptr, int tliIndex); = +/* in pg_rewind.c */ +extern void progress_report(bool force); + /* in timeline.c */ extern TimeLineHistoryEntry *rewind_parseTimeLineHistory(char *buffer, TimeLineID targetTLI, int *nentries); diff --git a/src/bin/pg_rewind/timeline.c b/src/bin/pg_rewind/timeline.c index 12b19b3..987452c 100644 --- a/src/bin/pg_rewind/timeline.c +++ b/src/bin/pg_rewind/timeline.c @@ -13,7 +13,6 @@ = #include "access/timeline.h" #include "access/xlog_internal.h" -#include "fe_utils/logging.h" = /* * This is copy-pasted from the backend readTimeLineHistory, modified to ------- =_aaaaaaaaaa0--