agora inbox for [email protected]help / color / mirror / Atom feed
merging duplicate definitions of adjust_relid_set 5+ messages / 4 participants [nested] [flat]
* merging duplicate definitions of adjust_relid_set @ 2017-03-17 17:33 Robert Haas <[email protected]> 2017-03-17 17:50 ` Re: merging duplicate definitions of adjust_relid_set Tom Lane <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Robert Haas @ 2017-03-17 17:33 UTC (permalink / raw) To: pgsql-hackers While reviewing Ashutosh Bapat's partitionwise join code, I noticed he'd run up against the problem that adjust_relid_set() is defined as static in two different source files, and he wanted to call it from a third file. I didn't much like his solution to that problem, which was to rename one of them and make that definition non-static; I think it would be better to keep the existing name and stop defining it in multiple places. However, I discovered that there wasn't really an obviously-good place to put the function; neither prepunion.c nor rewriteManip.c, the two files that contain static versions as of now, seem like an appropriate place from which to expose it, and I didn't find anything else that I was wildly in love with, either. The attached patch puts it in var.c, because it didn't look horrible and I thought it wasn't worth creating a new file just for this. Objections, better ideas? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers Attachments: [application/octet-stream] merge-adjust-relid-sets.patch (4.0K, ../../CA+TgmoZ_1bUAADs5oHhU07_t=3zdf_foKGDAFGWYvmcCbwn63Q@mail.gmail.com/2-merge-adjust-relid-sets.patch) download | inline diff: diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c index 1389db1..c1bbf7b 100644 --- a/src/backend/optimizer/prep/prepunion.c +++ b/src/backend/optimizer/prep/prepunion.c @@ -45,6 +45,7 @@ #include "optimizer/planner.h" #include "optimizer/prep.h" #include "optimizer/tlist.h" +#include "optimizer/var.h" #include "parser/parse_coerce.h" #include "parser/parsetree.h" #include "utils/lsyscache.h" @@ -107,7 +108,6 @@ static Bitmapset *translate_col_privs(const Bitmapset *parent_privs, List *translated_vars); static Node *adjust_appendrel_attrs_mutator(Node *node, adjust_appendrel_attrs_context *context); -static Relids adjust_relid_set(Relids relids, Index oldrelid, Index newrelid); static List *adjust_inherited_tlist(List *tlist, AppendRelInfo *context); @@ -1976,23 +1976,6 @@ adjust_appendrel_attrs_mutator(Node *node, } /* - * Substitute newrelid for oldrelid in a Relid set - */ -static Relids -adjust_relid_set(Relids relids, Index oldrelid, Index newrelid) -{ - if (bms_is_member(oldrelid, relids)) - { - /* Ensure we have a modifiable copy */ - relids = bms_copy(relids); - /* Remove old, add new */ - relids = bms_del_member(relids, oldrelid); - relids = bms_add_member(relids, newrelid); - } - return relids; -} - -/* * Adjust the targetlist entries of an inherited UPDATE operation * * The expressions have already been fixed, but we have to make sure that diff --git a/src/backend/optimizer/util/var.c b/src/backend/optimizer/util/var.c index cf326ae..ccbed7a 100644 --- a/src/backend/optimizer/util/var.c +++ b/src/backend/optimizer/util/var.c @@ -840,3 +840,20 @@ alias_relid_set(PlannerInfo *root, Relids relids) } return result; } + +/* + * Substitute newrelid for oldrelid in a Relid set + */ +Relids +adjust_relid_set(Relids relids, Index oldrelid, Index newrelid) +{ + if (bms_is_member(oldrelid, relids)) + { + /* Ensure we have a modifiable copy */ + relids = bms_copy(relids); + /* Remove old, add new */ + relids = bms_del_member(relids, oldrelid); + relids = bms_add_member(relids, newrelid); + } + return relids; +} diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c index b23a3b7..55248c7 100644 --- a/src/backend/rewrite/rewriteManip.c +++ b/src/backend/rewrite/rewriteManip.c @@ -18,6 +18,7 @@ #include "nodes/nodeFuncs.h" #include "nodes/plannodes.h" #include "optimizer/clauses.h" +#include "optimizer/var.h" #include "parser/parse_coerce.h" #include "parser/parse_relation.h" #include "parser/parsetree.h" @@ -49,7 +50,6 @@ static bool locate_windowfunc_walker(Node *node, locate_windowfunc_context *context); static bool checkExprHasSubLink_walker(Node *node, void *context); static Relids offset_relid_set(Relids relids, int offset); -static Relids adjust_relid_set(Relids relids, int oldrelid, int newrelid); /* @@ -655,23 +655,6 @@ ChangeVarNodes(Node *node, int rt_index, int new_index, int sublevels_up) } /* - * Substitute newrelid for oldrelid in a Relid set - */ -static Relids -adjust_relid_set(Relids relids, int oldrelid, int newrelid) -{ - if (bms_is_member(oldrelid, relids)) - { - /* Ensure we have a modifiable copy */ - relids = bms_copy(relids); - /* Remove old, add new */ - relids = bms_del_member(relids, oldrelid); - relids = bms_add_member(relids, newrelid); - } - return relids; -} - -/* * IncrementVarSublevelsUp - adjust Var nodes when pushing them down in tree * * Find all Var nodes in the given tree having varlevelsup >= min_sublevels_up, diff --git a/src/include/optimizer/var.h b/src/include/optimizer/var.h index ae1f856..4e24ab5 100644 --- a/src/include/optimizer/var.h +++ b/src/include/optimizer/var.h @@ -36,5 +36,6 @@ extern bool contain_vars_of_level(Node *node, int levelsup); extern int locate_var_of_level(Node *node, int levelsup); extern List *pull_var_clause(Node *node, int flags); extern Node *flatten_join_alias_vars(PlannerInfo *root, Node *node); +extern Relids adjust_relid_set(Relids relids, Index oldrelid, Index newrelid); #endif /* VAR_H */ ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: merging duplicate definitions of adjust_relid_set 2017-03-17 17:33 merging duplicate definitions of adjust_relid_set Robert Haas <[email protected]> @ 2017-03-17 17:50 ` Tom Lane <[email protected]> 2017-03-17 18:18 ` Re: merging duplicate definitions of adjust_relid_set Robert Haas <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Tom Lane @ 2017-03-17 17:50 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: pgsql-hackers Robert Haas <[email protected]> writes: > While reviewing Ashutosh Bapat's partitionwise join code, I noticed > he'd run up against the problem that adjust_relid_set() is defined as > static in two different source files, and he wanted to call it from a > third file. I didn't much like his solution to that problem, which > was to rename one of them and make that definition non-static; I think > it would be better to keep the existing name and stop defining it in > multiple places. However, I discovered that there wasn't really an > obviously-good place to put the function; neither prepunion.c nor > rewriteManip.c, the two files that contain static versions as of now, > seem like an appropriate place from which to expose it, and I didn't > find anything else that I was wildly in love with, either. The > attached patch puts it in var.c, because it didn't look horrible and I > thought it wasn't worth creating a new file just for this. > Objections, better ideas? I think it might be better to define it as a fundamental Bitmapset operation in nodes/bitmapset.c, along the lines of Bitmapset *bms_replace_member(const Bitmapset *bms, int member, int repl); This API would probably require giving up the property of not copying the set unless it changes, but I doubt that that's performance critical. regards, tom lane -- Sent via pgsql-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: merging duplicate definitions of adjust_relid_set 2017-03-17 17:33 merging duplicate definitions of adjust_relid_set Robert Haas <[email protected]> 2017-03-17 17:50 ` Re: merging duplicate definitions of adjust_relid_set Tom Lane <[email protected]> @ 2017-03-17 18:18 ` Robert Haas <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Robert Haas @ 2017-03-17 18:18 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: pgsql-hackers On Fri, Mar 17, 2017 at 1:50 PM, Tom Lane <[email protected]> wrote: > Robert Haas <[email protected]> writes: >> While reviewing Ashutosh Bapat's partitionwise join code, I noticed >> he'd run up against the problem that adjust_relid_set() is defined as >> static in two different source files, and he wanted to call it from a >> third file. I didn't much like his solution to that problem, which >> was to rename one of them and make that definition non-static; I think >> it would be better to keep the existing name and stop defining it in >> multiple places. However, I discovered that there wasn't really an >> obviously-good place to put the function; neither prepunion.c nor >> rewriteManip.c, the two files that contain static versions as of now, >> seem like an appropriate place from which to expose it, and I didn't >> find anything else that I was wildly in love with, either. The >> attached patch puts it in var.c, because it didn't look horrible and I >> thought it wasn't worth creating a new file just for this. > >> Objections, better ideas? > > I think it might be better to define it as a fundamental Bitmapset > operation in nodes/bitmapset.c, along the lines of > > Bitmapset *bms_replace_member(const Bitmapset *bms, int member, int repl); > > This API would probably require giving up the property of not copying the > set unless it changes, but I doubt that that's performance critical. I thought of that, but I wasn't sure it was good in terms of clarity to replace a function that works in terms of Relids with one that works in terms of Bitmapset *. I know they're the same, so it's just a style thing. But actually, I think my email was premature. Actually, his patch series first changes adjust_relid_set to take different arguments so that it can do multiple translations at once, and then a later patch in the series renames it. So there's actually no need to merge the definitions, because one of them ends up going away anyway. So, uh, never mind. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list ([email protected]) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers ^ permalink raw reply [nested|flat] 5+ messages in thread
* [PATCH 05/20] Use cf* abstraction in archiver and tar @ 2020-12-25 06:34 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Justin Pryzby @ 2020-12-25 06:34 UTC (permalink / raw) ..rather than direct, conditional calls to gzopen/fopen. See also: bf9aa490db24b2334b3595ee33653bf2fe39208c --- src/bin/pg_dump/compress_io.c | 53 +++++++++++++ src/bin/pg_dump/compress_io.h | 5 ++ src/bin/pg_dump/pg_backup_archiver.c | 109 +++++++++------------------ src/bin/pg_dump/pg_backup_archiver.h | 16 ++-- src/bin/pg_dump/pg_backup_tar.c | 85 +++++---------------- 5 files changed, 117 insertions(+), 151 deletions(-) diff --git a/src/bin/pg_dump/compress_io.c b/src/bin/pg_dump/compress_io.c index 21957d68f3..d66d6f60f5 100644 --- a/src/bin/pg_dump/compress_io.c +++ b/src/bin/pg_dump/compress_io.c @@ -540,6 +540,58 @@ cfopen(const char *path, const char *mode, Compress *compression) } } +/* + * Open a file descriptor, with specified compression. + * Returns an opaque cfp object. + */ +cfp * +cfdopen(int fd, const char *mode, Compress *compression) +{ + cfp *fp = pg_malloc0(sizeof(cfp)); + + switch (compression->alg) + { +#ifdef HAVE_LIBZ + case COMPR_ALG_LIBZ: + if (compression->level != Z_DEFAULT_COMPRESSION) + { + /* user has specified a compression level, so tell zlib to use it */ + char mode_compression[32]; + + snprintf(mode_compression, sizeof(mode_compression), "%s%d", + mode, compression->level); + fp->compressedfp = gzdopen(fd, mode_compression); + } + else + { + /* don't specify a level, just use the zlib default */ + fp->compressedfp = gzdopen(fd, mode); + } + + if (fp->compressedfp == NULL) + { + free_keep_errno(fp); + fp = NULL; + } + return fp; +#endif + + case COMPR_ALG_NONE: + fp->uncompressedfp = fdopen(fd, mode); + if (fp->uncompressedfp == NULL) + { + free_keep_errno(fp); + fp = NULL; + } + else + setvbuf(fp->uncompressedfp, NULL, _IONBF, 0); + return fp; + + default: + /* Should not happen */ + fatal("requested compression not available in this installation"); + } +} int cfread(void *ptr, int size, cfp *fp) @@ -616,6 +668,7 @@ cfgets(cfp *fp, char *buf, int len) return fgets(buf, len, fp->uncompressedfp); } +/* Close the given compressed or uncompressed stream; return 0 on success. */ int cfclose(cfp *fp) { diff --git a/src/bin/pg_dump/compress_io.h b/src/bin/pg_dump/compress_io.h index fb9d659acc..318a6b5340 100644 --- a/src/bin/pg_dump/compress_io.h +++ b/src/bin/pg_dump/compress_io.h @@ -21,6 +21,10 @@ #define ZLIB_OUT_SIZE 4096 #define ZLIB_IN_SIZE 4096 +/* Forward declaration */ +struct ArchiveHandle; +typedef struct _archiveHandle ArchiveHandle; + /* Prototype for callback function to WriteDataToArchive() */ typedef void (*WriteFunc) (ArchiveHandle *AH, const char *buf, size_t len); @@ -58,6 +62,7 @@ extern const struct compressLibs compresslibs[]; typedef struct cfp cfp; extern cfp *cfopen(const char *path, const char *mode, Compress *compression); +extern cfp *cfdopen(int fd, const char *mode, Compress *compression); extern cfp *cfopen_read(const char *path, const char *mode, Compress *compression); extern cfp *cfopen_write(const char *path, const char *mode, Compress *compression); extern int cfread(void *ptr, int size, cfp *fp); diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c index 3eb6c55600..bd06fbb787 100644 --- a/src/bin/pg_dump/pg_backup_archiver.c +++ b/src/bin/pg_dump/pg_backup_archiver.c @@ -39,17 +39,11 @@ #include "pg_backup_archiver.h" #include "pg_backup_db.h" #include "pg_backup_utils.h" +#include "compress_io.h" #define TEXT_DUMP_HEADER "--\n-- PostgreSQL database dump\n--\n\n" #define TEXT_DUMPALL_HEADER "--\n-- PostgreSQL database cluster dump\n--\n\n" -/* state needed to save/restore an archive's output target */ -typedef struct _outputContext -{ - void *OF; - int gzOut; -} OutputContext; - /* * State for tracking TocEntrys that are ready to process during a parallel * restore. (This used to be a list, and we still call it that, though now @@ -99,8 +93,8 @@ static int RestoringToDB(ArchiveHandle *AH); static void dump_lo_buf(ArchiveHandle *AH); static void dumpTimestamp(ArchiveHandle *AH, const char *msg, time_t tim); static void SetOutput(ArchiveHandle *AH, const char *filename, Compress *compress); -static OutputContext SaveOutput(ArchiveHandle *AH); -static void RestoreOutput(ArchiveHandle *AH, OutputContext savedContext); +static cfp *SaveOutput(ArchiveHandle *AH); +static void RestoreOutput(ArchiveHandle *AH, cfp *fp); static int restore_toc_entry(ArchiveHandle *AH, TocEntry *te, bool is_parallel); static void restore_toc_entries_prefork(ArchiveHandle *AH, @@ -270,10 +264,8 @@ CloseArchive(Archive *AHX) AH->ClosePtr(AH); /* Close the output */ - if (AH->gzOut) - res = GZCLOSE(AH->OF); - else if (AH->OF != stdout) - res = fclose(AH->OF); + if ((FILE *)AH->OF != stdout) + res = cfclose(AH->OF); if (res != 0) fatal("could not close output file: %m"); @@ -355,7 +347,7 @@ RestoreArchive(Archive *AHX) RestoreOptions *ropt = AH->public.ropt; bool parallel_mode; TocEntry *te; - OutputContext sav; + cfp *sav; AH->stage = STAGE_INITIALIZING; @@ -1120,7 +1112,7 @@ PrintTOCSummary(Archive *AHX) RestoreOptions *ropt = AH->public.ropt; TocEntry *te; teSection curSection; - OutputContext sav; + cfp *sav; const char *fmtName; char stamp_str[64]; @@ -1492,6 +1484,7 @@ archprintf(Archive *AH, const char *fmt,...) static void SetOutput(ArchiveHandle *AH, const char *filename, Compress *compression) { + char fmode[14]; int fn; if (filename) @@ -1511,38 +1504,22 @@ SetOutput(ArchiveHandle *AH, const char *filename, Compress *compression) else fn = fileno(stdout); - /* If compression explicitly requested, use gzopen */ -#ifdef HAVE_LIBZ - if (compression->alg != COMPR_ALG_NONE) + if (fn >= 0) { - char fmode[14]; + /* Handle output to stdout */ + sprintf(fmode, "%sb%d", + AH->mode == archModeAppend ? PG_BINARY_A : PG_BINARY_W, + compression->level); - /* Don't use PG_BINARY_x since this is zlib */ - sprintf(fmode, "wb%d", compression->level); - if (fn >= 0) - AH->OF = gzdopen(dup(fn), fmode); - else - AH->OF = gzopen(filename, fmode); - AH->gzOut = 1; + AH->OF = cfdopen(dup(fn), fmode, compression); } else -#endif - { /* Use fopen */ - if (AH->mode == archModeAppend) - { - if (fn >= 0) - AH->OF = fdopen(dup(fn), PG_BINARY_A); - else - AH->OF = fopen(filename, PG_BINARY_A); - } - else - { - if (fn >= 0) - AH->OF = fdopen(dup(fn), PG_BINARY_W); - else - AH->OF = fopen(filename, PG_BINARY_W); - } - AH->gzOut = 0; + { + Assert(filename != NULL); + sprintf(fmode, "%cb%d", + AH->mode == archModeAppend ? 'a' : 'w', + compression->level); + AH->OF = cfopen(filename, fmode, compression); } if (!AH->OF) @@ -1554,32 +1531,22 @@ SetOutput(ArchiveHandle *AH, const char *filename, Compress *compression) } } -static OutputContext +/* Return a pointer to the old FP */ +static cfp * SaveOutput(ArchiveHandle *AH) { - OutputContext sav; - - sav.OF = AH->OF; - sav.gzOut = AH->gzOut; - - return sav; + return AH->OF; } static void -RestoreOutput(ArchiveHandle *AH, OutputContext savedContext) +RestoreOutput(ArchiveHandle *AH, cfp *savedContext) { int res; - - if (AH->gzOut) - res = GZCLOSE(AH->OF); - else - res = fclose(AH->OF); - + res = cfclose(AH->OF); if (res != 0) fatal("could not close output file: %m"); - AH->gzOut = savedContext.gzOut; - AH->OF = savedContext.OF; + AH->OF = savedContext; } @@ -1703,22 +1670,14 @@ ahwrite(const void *ptr, size_t size, size_t nmemb, ArchiveHandle *AH) bytes_written = size * nmemb; } - else if (AH->gzOut) - bytes_written = GZWRITE(ptr, size, nmemb, AH->OF); else if (AH->CustomOutPtr) bytes_written = AH->CustomOutPtr(AH, ptr, size * nmemb); - + else if (RestoringToDB(AH)) + /* If we're doing a restore, and it's direct to DB, and we're + * connected then send it to the DB. */ + bytes_written = ExecuteSqlCommandBuf(&AH->public, (const char *) ptr, size * nmemb); else - { - /* - * If we're doing a restore, and it's direct to DB, and we're - * connected then send it to the DB. - */ - if (RestoringToDB(AH)) - bytes_written = ExecuteSqlCommandBuf(&AH->public, (const char *) ptr, size * nmemb); - else - bytes_written = fwrite(ptr, size, nmemb, AH->OF) * size; - } + bytes_written = cfwrite(ptr, size * nmemb, AH->OF); if (bytes_written != size * nmemb) WRITE_ERROR_EXIT; @@ -2127,6 +2086,7 @@ _discoverArchiveFormat(ArchiveHandle *AH) fh = stdin; if (!fh) fatal("could not open input file: %m"); + setvbuf(fh, NULL, _IONBF, 0); } if ((cnt = fread(sig, 1, 5, fh)) != 5) @@ -2266,6 +2226,7 @@ _allocAH(const char *FileSpec, const ArchiveFormat fmt, SetupWorkerPtrType setupWorkerPtr) { ArchiveHandle *AH; + Compress nocompression = {0}; pg_log_debug("allocating AH for %s, format %d", FileSpec ? FileSpec : "(stdio)", fmt); @@ -2319,8 +2280,8 @@ _allocAH(const char *FileSpec, const ArchiveFormat fmt, memset(&(AH->sqlparse), 0, sizeof(AH->sqlparse)); /* Open stdout with no compression for AH output handle */ - AH->gzOut = 0; - AH->OF = stdout; + AH->OF = cfdopen(fileno(stdout), "w", &nocompression); + // AH->OF = cfdopen(STDOUT_FILENO, "w", compression); // XXX /* * On Windows, we need to use binary mode to read/write non-text files, diff --git a/src/bin/pg_dump/pg_backup_archiver.h b/src/bin/pg_dump/pg_backup_archiver.h index 6e033d040e..9f511b49b9 100644 --- a/src/bin/pg_dump/pg_backup_archiver.h +++ b/src/bin/pg_dump/pg_backup_archiver.h @@ -30,6 +30,9 @@ #include "pg_backup.h" #include "pqexpbuffer.h" +/* Forward declaration XXX: CIRCULAR */ +typedef struct cfp cfp; + #define LOBBUFSIZE 16384 /* @@ -38,19 +41,11 @@ */ #ifdef HAVE_LIBZ #include <zlib.h> -#define GZCLOSE(fh) gzclose(fh) -#define GZWRITE(p, s, n, fh) gzwrite(fh, p, (n) * (s)) -#define GZREAD(p, s, n, fh) gzread(fh, p, (n) * (s)) -#define GZEOF(fh) gzeof(fh) #else -#define GZCLOSE(fh) fclose(fh) -#define GZWRITE(p, s, n, fh) (fwrite(p, s, n, fh) * (s)) -#define GZREAD(p, s, n, fh) fread(p, s, n, fh) -#define GZEOF(fh) feof(fh) + /* this is just the redefinition of a libz constant, in case zlib isn't * available */ #define Z_DEFAULT_COMPRESSION (-1) - typedef struct _z_stream { void *next_in; @@ -318,8 +313,7 @@ struct _archiveHandle char *fSpec; /* Archive File Spec */ FILE *FH; /* General purpose file handle */ - void *OF; - int gzOut; /* Output file */ + cfp *OF; /* Output file (compressed or not) */ struct _tocEntry *toc; /* Header of circular list of TOC entries */ int tocCount; /* Number of TOC entries */ diff --git a/src/bin/pg_dump/pg_backup_tar.c b/src/bin/pg_dump/pg_backup_tar.c index 4ba79ab924..16f4e0792a 100644 --- a/src/bin/pg_dump/pg_backup_tar.c +++ b/src/bin/pg_dump/pg_backup_tar.c @@ -66,12 +66,7 @@ static void _EndBlobs(ArchiveHandle *AH, TocEntry *te); typedef struct { -#ifdef HAVE_LIBZ - gzFile zFH; -#else - FILE *zFH; -#endif - FILE *nFH; + cfp *FH; FILE *tarFH; FILE *tmpFH; char *targetFile; @@ -191,7 +186,7 @@ InitArchiveFmt_Tar(ArchiveHandle *AH) * Make unbuffered since we will dup() it, and the buffers screw each * other */ - /* setvbuf(ctx->tarFH, NULL, _IONBF, 0); */ + // setvbuf(ctx->tarFH, NULL, _IONBF, 0); ctx->hasSeek = checkSeek(ctx->tarFH); @@ -223,7 +218,7 @@ InitArchiveFmt_Tar(ArchiveHandle *AH) * Make unbuffered since we will dup() it, and the buffers screw each * other */ - /* setvbuf(ctx->tarFH, NULL, _IONBF, 0); */ + setvbuf(ctx->tarFH, NULL, _IONBF, 0); ctx->tarFHpos = 0; @@ -321,10 +316,6 @@ tarOpen(ArchiveHandle *AH, const char *filename, char mode) lclContext *ctx = (lclContext *) AH->formatData; TAR_MEMBER *tm; -#ifdef HAVE_LIBZ - char fmode[14]; -#endif - if (mode == 'r') { tm = _tarPositionTo(AH, filename); @@ -345,16 +336,10 @@ tarOpen(ArchiveHandle *AH, const char *filename, char mode) } } -#ifdef HAVE_LIBZ - if (AH->compression.alg == COMPR_ALG_NONE) - tm->nFH = ctx->tarFH; + tm->FH = cfdopen(dup(fileno(ctx->tarFH)), "rb", &AH->compression); else fatal("compression is not supported by tar archive format"); - /* tm->zFH = gzdopen(dup(fileno(ctx->tarFH)), "rb"); */ -#else - tm->nFH = ctx->tarFH; -#endif } else { @@ -406,21 +391,11 @@ tarOpen(ArchiveHandle *AH, const char *filename, char mode) umask(old_umask); -#ifdef HAVE_LIBZ - - if (AH->compression.alg != COMPR_ALG_NONE) - { - sprintf(fmode, "wb%d", AH->compression.level); - tm->zFH = gzdopen(dup(fileno(tm->tmpFH)), fmode); - if (tm->zFH == NULL) - fatal("could not open temporary file"); - } - else - tm->nFH = tm->tmpFH; -#else - - tm->nFH = tm->tmpFH; -#endif + tm->FH = cfdopen(dup(fileno(tm->tmpFH)), + mode == 'r' ? "r" : "w", + &AH->compression); + if (tm->FH == NULL) + fatal("could not open temporary file"); tm->AH = AH; tm->targetFile = pg_strdup(filename); @@ -435,12 +410,14 @@ tarOpen(ArchiveHandle *AH, const char *filename, char mode) static void tarClose(ArchiveHandle *AH, TAR_MEMBER *th) { + int res; + /* * Close the GZ file since we dup'd. This will flush the buffers. */ - if (AH->compression.alg != COMPR_ALG_NONE) - if (GZCLOSE(th->zFH) != 0) - fatal("could not close tar member"); + res = cfclose(th->FH); + if (res != 0) + fatal("could not close tar member"); if (th->mode == 'w') _tarAddFile(AH, th); /* This will close the temp file */ @@ -453,8 +430,7 @@ tarClose(ArchiveHandle *AH, TAR_MEMBER *th) if (th->targetFile) free(th->targetFile); - th->nFH = NULL; - th->zFH = NULL; + th->FH = NULL; } #ifdef __NOT_USED__ @@ -540,29 +516,9 @@ _tarReadRaw(ArchiveHandle *AH, void *buf, size_t len, TAR_MEMBER *th, FILE *fh) } else if (th) { - if (th->zFH) - { - res = GZREAD(&((char *) buf)[used], 1, len, th->zFH); - if (res != len && !GZEOF(th->zFH)) - { -#ifdef HAVE_LIBZ - int errnum; - const char *errmsg = gzerror(th->zFH, &errnum); - - fatal("could not read from input file: %s", - errnum == Z_ERRNO ? strerror(errno) : errmsg); -#else - fatal("could not read from input file: %s", - strerror(errno)); -#endif - } - } - else - { - res = fread(&((char *) buf)[used], 1, len, th->nFH); - if (res != len && !feof(th->nFH)) - READ_ERROR_EXIT(th->nFH); - } + res = cfread(&((char *) buf)[used], len, th->FH); + if (res != len && !cfeof(th->FH)) + fatal("could not read from input file: %m"); } } @@ -594,10 +550,7 @@ tarWrite(const void *buf, size_t len, TAR_MEMBER *th) { size_t res; - if (th->zFH != NULL) - res = GZWRITE(buf, 1, len, th->zFH); - else - res = fwrite(buf, 1, len, th->nFH); + res = cfwrite(buf, len, th->FH); th->pos += res; return res; -- 2.17.0 --GRPZ8SYKNexpdSJ7 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0006-pg_dump-zstd-compression.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* [PATCH v2 3/3] Add FLUSH_MIXED support and implement it for RELATION stats @ 2026-01-19 06:27 Bertrand Drouvot <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Bertrand Drouvot @ 2026-01-19 06:27 UTC (permalink / raw) This commit extends the non transactional stats infrastructure to support statistics kinds with mixed transaction behavior: some fields are transactional (e.g., tuple inserts/updates/deletes) while others are non transactional (e.g., sequential scans blocks read, ...). It introduces FLUSH_MIXED as a third flush behavior type, alongside FLUSH_ANYTIME and FLUSH_AT_TXN_BOUNDARY. For FLUSH_MIXED kinds, a new flush_anytime_cb callback enables partial flushing of only the non transactional fields during running transactions. Some tests are also added. Implementation details: - Add FLUSH_MIXED to PgStat_FlushBehavior enum - Add flush_anytime_cb to PgStat_KindInfo for partial flushing callback - Update pgstat_flush_pending_entries() to call flush_anytime_cb for FLUSH_MIXED entries when in anytime_only mode - Keep FLUSH_MIXED entries in the pending list after partial flush, as transactional fields still need to be flushed at transaction boundary RELATION stats are making use of FLUSH_MIXED: - Change RELATION from TXN_ALL to FLUSH_MIXED - Implement pgstat_relation_flush_anytime_cb() to flush only read related stats: numscans, tuples_returned, tuples_fetched, blocks_fetched, blocks_hit - Clear these fields after flushing to prevent double counting when pgstat_relation_flush_cb() runs at transaction commit - Transactional stats (tuples_inserted, tuples_updated, tuples_deleted, live_tuples, dead_tuples) remain pending until transaction boundary Remark: We could also imagine adding a new flush_anytime_static_cb() callback for future FLUSH_MIXED fixed amount stats. --- doc/src/sgml/monitoring.sgml | 30 +++++++ src/backend/utils/activity/pgstat.c | 36 ++++++--- src/backend/utils/activity/pgstat_relation.c | 82 ++++++++++++++++++++ src/include/utils/pgstat_internal.h | 9 +++ src/test/isolation/expected/stats.out | 40 ++++++++++ src/test/isolation/expected/stats_1.out | 40 ++++++++++ src/test/isolation/specs/stats.spec | 12 +++ 7 files changed, 237 insertions(+), 12 deletions(-) 14.5% doc/src/sgml/ 47.2% src/backend/utils/activity/ 4.7% src/include/utils/ 29.4% src/test/isolation/expected/ 4.0% src/test/isolation/specs/ diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 817fd9f4ca7..15b55016b66 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -3730,6 +3730,16 @@ description | Waiting for a newly initialized WAL file to reach durable storage </tgroup> </table> + <note> + <para> + All the statistics are updated while the transactions are in progress, except + for <structfield>xact_commit</structfield>, <structfield>xact_rollback</structfield>, + <structfield>tup_inserted</structfield>, <structfield>tup_updated</structfield> and + <structfield>tup_deleted</structfield> that are updated only when the transactions + finish. + </para> + </note> + </sect2> <sect2 id="monitoring-pg-stat-database-conflicts-view"> @@ -4186,6 +4196,17 @@ description | Waiting for a newly initialized WAL file to reach durable storage </tgroup> </table> + <note> + <para> + The <structfield>seq_scan</structfield>, <structfield>last_seq_scan</structfield>, + <structfield>seq_tup_read</structfield>, <structfield>idx_scan</structfield>, + <structfield>last_idx_scan</structfield> and <structfield>idx_tup_fetch</structfield> + are updated while the transactions are in progress. This means that we can see + those statistics being updated without having to wait until the transaction + finishes. + </para> + </note> + </sect2> <sect2 id="monitoring-pg-stat-all-indexes-view"> @@ -4367,6 +4388,15 @@ description | Waiting for a newly initialized WAL file to reach durable storage tuples (see <xref linkend="indexes-multicolumn"/>). </para> </note> + <note> + <para> + The <structfield>idx_scan</structfield>, <structfield>last_idx_scan</structfield>, + <structfield>idx_tup_read</structfield> and <structfield>idx_tup_fetch</structfield> + are updated while the transactions are in progress. This means that we can see + those statistics being updated without having to wait until the transaction + finishes. + </para> + </note> <tip> <para> <command>EXPLAIN ANALYZE</command> outputs the total number of index diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c index 0f45a7d165e..5b93683ea9b 100644 --- a/src/backend/utils/activity/pgstat.c +++ b/src/backend/utils/activity/pgstat.c @@ -289,7 +289,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE] .fixed_amount = false, .write_to_file = true, - .flush_behavior = FLUSH_AT_TXN_BOUNDARY, + .flush_behavior = FLUSH_ANYTIME, /* so pg_stat_database entries can be seen in all databases */ .accessed_across_databases = true, @@ -307,7 +307,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE] .fixed_amount = false, .write_to_file = true, - .flush_behavior = FLUSH_AT_TXN_BOUNDARY, + .flush_behavior = FLUSH_MIXED, .shared_size = sizeof(PgStatShared_Relation), .shared_data_off = offsetof(PgStatShared_Relation, stats), @@ -315,6 +315,7 @@ static const PgStat_KindInfo pgstat_kind_builtin_infos[PGSTAT_KIND_BUILTIN_SIZE] .pending_size = sizeof(PgStat_TableStatus), .flush_pending_cb = pgstat_relation_flush_cb, + .flush_anytime_cb = pgstat_relation_flush_anytime_cb, .delete_pending_cb = pgstat_relation_delete_pending_cb, .reset_timestamp_cb = pgstat_relation_reset_timestamp_cb, }, @@ -1347,10 +1348,11 @@ pgstat_delete_pending_entry(PgStat_EntryRef *entry_ref) /* * Flush out pending variable-numbered stats. * - * If anytime_only is true, only flushes FLUSH_ANYTIME entries. + * If anytime_only is true, only flushes FLUSH_ANYTIME and FLUSH_MIXED entries, + * using flush_anytime_cb for FLUSH_MIXED. * This is safe to call inside transactions. * - * If anytime_only is false, flushes all entries. + * If anytime_only is false, flushes all entries using flush_pending_cb. */ static bool pgstat_flush_pending_entries(bool nowait, bool anytime_only) @@ -1378,6 +1380,7 @@ pgstat_flush_pending_entries(bool nowait, bool anytime_only) PgStat_Kind kind = key.kind; const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind); bool did_flush; + bool is_partial_flush = false; dlist_node *next; Assert(!kind_info->fixed_amount); @@ -1397,8 +1400,21 @@ pgstat_flush_pending_entries(bool nowait, bool anytime_only) continue; } - /* flush the stats, if possible */ - did_flush = kind_info->flush_pending_cb(entry_ref, nowait); + /* flush the stats (with the appropriate callback), if possible */ + if (anytime_only && + kind_info->flush_behavior == FLUSH_MIXED && + kind_info->flush_anytime_cb != NULL) + { + /* Partial flush of non-transactional fields only */ + did_flush = kind_info->flush_anytime_cb(entry_ref, nowait); + is_partial_flush = true; + } + else + { + /* Full flush */ + did_flush = kind_info->flush_pending_cb(entry_ref, nowait); + is_partial_flush = false; + } Assert(did_flush || nowait); @@ -1408,8 +1424,8 @@ pgstat_flush_pending_entries(bool nowait, bool anytime_only) else next = NULL; - /* if successfully flushed, remove entry */ - if (did_flush) + /* if successfull non partial flush, remove entry */ + if (did_flush && !is_partial_flush) pgstat_delete_pending_entry(entry_ref); else have_pending = true; @@ -1417,10 +1433,6 @@ pgstat_flush_pending_entries(bool nowait, bool anytime_only) cur = next; } - /* - * When in anytime_only mode, the list may not be empty because - * FLUSH_AT_TXN_BOUNDARY entries were skipped. - */ Assert(dlist_is_empty(&pgStatPending) == !have_pending); return have_pending; diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c index feae2ae5f44..6d6f333039e 100644 --- a/src/backend/utils/activity/pgstat_relation.c +++ b/src/backend/utils/activity/pgstat_relation.c @@ -887,6 +887,88 @@ pgstat_relation_flush_cb(PgStat_EntryRef *entry_ref, bool nowait) return true; } +/* + * Flush only non-transactional relation stats. + * + * This is called periodically during running transactions to make some + * statistics visible without waiting for the transaction to finish. + * + * Transactional stats (inserts/updates/deletes and their effects on live/dead + * tuple counts) remain in pending until the transaction ends, at which point + * pgstat_relation_flush_cb() will flush them. + * + * If nowait is true and the lock could not be immediately acquired, returns + * false without flushing the entry. Otherwise returns true. + */ +bool +pgstat_relation_flush_anytime_cb(PgStat_EntryRef *entry_ref, bool nowait) +{ + Oid dboid; + PgStat_TableStatus *lstats; /* pending stats entry */ + PgStatShared_Relation *shtabstats; + PgStat_StatTabEntry *tabentry; /* table entry of shared stats */ + PgStat_StatDBEntry *dbentry; /* pending database entry */ + bool has_nontxn_stats = false; + + dboid = entry_ref->shared_entry->key.dboid; + lstats = (PgStat_TableStatus *) entry_ref->pending; + shtabstats = (PgStatShared_Relation *) entry_ref->shared_stats; + + /* + * Check if there are any non-transactional stats to flush. Avoid + * unnecessarily locking the entry if nothing accumulated. + */ + if (lstats->counts.numscans > 0 || + lstats->counts.tuples_returned > 0 || + lstats->counts.tuples_fetched > 0 || + lstats->counts.blocks_fetched > 0 || + lstats->counts.blocks_hit > 0) + has_nontxn_stats = true; + + if (!has_nontxn_stats) + return true; + + if (!pgstat_lock_entry(entry_ref, nowait)) + return false; + + /* Add only the non-transactional values to the shared entry */ + tabentry = &shtabstats->stats; + + tabentry->numscans += lstats->counts.numscans; + if (lstats->counts.numscans) + { + TimestampTz t = GetCurrentTimestamp(); + + if (t > tabentry->lastscan) + tabentry->lastscan = t; + } + tabentry->tuples_returned += lstats->counts.tuples_returned; + tabentry->tuples_fetched += lstats->counts.tuples_fetched; + tabentry->blocks_fetched += lstats->counts.blocks_fetched; + tabentry->blocks_hit += lstats->counts.blocks_hit; + + pgstat_unlock_entry(entry_ref); + + /* Also update the corresponding fields in database stats */ + dbentry = pgstat_prep_database_pending(dboid); + dbentry->tuples_returned += lstats->counts.tuples_returned; + dbentry->tuples_fetched += lstats->counts.tuples_fetched; + dbentry->blocks_fetched += lstats->counts.blocks_fetched; + dbentry->blocks_hit += lstats->counts.blocks_hit; + + /* + * Clear the flushed fields from pending stats to prevent double-counting + * when pgstat_relation_flush_cb() runs at transaction boundary. + */ + lstats->counts.numscans = 0; + lstats->counts.tuples_returned = 0; + lstats->counts.tuples_fetched = 0; + lstats->counts.blocks_fetched = 0; + lstats->counts.blocks_hit = 0; + + return true; +} + void pgstat_relation_delete_pending_cb(PgStat_EntryRef *entry_ref) { diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h index 63feae640d1..c80b8162b37 100644 --- a/src/include/utils/pgstat_internal.h +++ b/src/include/utils/pgstat_internal.h @@ -233,6 +233,8 @@ typedef enum PgStat_FlushBehavior * including within transactions */ FLUSH_AT_TXN_BOUNDARY, /* All fields can only be flushed at * transaction boundary */ + FLUSH_MIXED, /* Mix of fields that can be flushed anytime + * or only at transaction boundary */ } PgStat_FlushBehavior; /* @@ -264,6 +266,12 @@ typedef struct PgStat_KindInfo /* Flush behavior */ PgStat_FlushBehavior flush_behavior; + /* + * For PGSTAT_FLUSH_MIXED kinds: callback to flush only some fields. If + * NULL for a MIXED kind, treated as PGSTAT_FLUSH_AT_TXN_BOUNDARY. + */ + bool (*flush_anytime_cb) (PgStat_EntryRef *entry_ref, bool nowait); + /* * The size of an entry in the shared stats hash table (pointed to by * PgStatShared_HashEntry->body). For fixed-numbered statistics, this is @@ -776,6 +784,7 @@ extern void AtPrepare_PgStat_Relations(PgStat_SubXactStatus *xact_state); extern void PostPrepare_PgStat_Relations(PgStat_SubXactStatus *xact_state); extern bool pgstat_relation_flush_cb(PgStat_EntryRef *entry_ref, bool nowait); +extern bool pgstat_relation_flush_anytime_cb(PgStat_EntryRef *entry_ref, bool nowait); extern void pgstat_relation_delete_pending_cb(PgStat_EntryRef *entry_ref); extern void pgstat_relation_reset_timestamp_cb(PgStatShared_Common *header, TimestampTz ts); diff --git a/src/test/isolation/expected/stats.out b/src/test/isolation/expected/stats.out index cfad309ccf3..6d62b30e4a7 100644 --- a/src/test/isolation/expected/stats.out +++ b/src/test/isolation/expected/stats.out @@ -2245,6 +2245,46 @@ seq_scan|seq_tup_read|n_tup_ins|n_tup_upd|n_tup_del|n_live_tup|n_dead_tup|vacuum (1 row) +starting permutation: s2_begin s2_table_select s1_sleep s1_table_stats s2_table_drop s2_commit +pg_stat_force_next_flush +------------------------ + +(1 row) + +step s2_begin: BEGIN; +step s2_table_select: SELECT * FROM test_stat_tab ORDER BY key, value; +key|value +---+----- +k0 | 1 +(1 row) + +step s1_sleep: SELECT pg_sleep(1.5); +pg_sleep +-------- + +(1 row) + +step s1_table_stats: + SELECT + pg_stat_get_numscans(tso.oid) AS seq_scan, + pg_stat_get_tuples_returned(tso.oid) AS seq_tup_read, + pg_stat_get_tuples_inserted(tso.oid) AS n_tup_ins, + pg_stat_get_tuples_updated(tso.oid) AS n_tup_upd, + pg_stat_get_tuples_deleted(tso.oid) AS n_tup_del, + pg_stat_get_live_tuples(tso.oid) AS n_live_tup, + pg_stat_get_dead_tuples(tso.oid) AS n_dead_tup, + pg_stat_get_vacuum_count(tso.oid) AS vacuum_count + FROM test_stat_oid AS tso + WHERE tso.name = 'test_stat_tab' + +seq_scan|seq_tup_read|n_tup_ins|n_tup_upd|n_tup_del|n_live_tup|n_dead_tup|vacuum_count +--------+------------+---------+---------+---------+----------+----------+------------ + 1| 1| 1| 0| 0| 1| 0| 0 +(1 row) + +step s2_table_drop: DROP TABLE test_stat_tab; +step s2_commit: COMMIT; + starting permutation: s1_track_counts_off s1_table_stats s1_track_counts_on pg_stat_force_next_flush ------------------------ diff --git a/src/test/isolation/expected/stats_1.out b/src/test/isolation/expected/stats_1.out index e1d937784cb..2fade10e817 100644 --- a/src/test/isolation/expected/stats_1.out +++ b/src/test/isolation/expected/stats_1.out @@ -2253,6 +2253,46 @@ seq_scan|seq_tup_read|n_tup_ins|n_tup_upd|n_tup_del|n_live_tup|n_dead_tup|vacuum (1 row) +starting permutation: s2_begin s2_table_select s1_sleep s1_table_stats s2_table_drop s2_commit +pg_stat_force_next_flush +------------------------ + +(1 row) + +step s2_begin: BEGIN; +step s2_table_select: SELECT * FROM test_stat_tab ORDER BY key, value; +key|value +---+----- +k0 | 1 +(1 row) + +step s1_sleep: SELECT pg_sleep(1.5); +pg_sleep +-------- + +(1 row) + +step s1_table_stats: + SELECT + pg_stat_get_numscans(tso.oid) AS seq_scan, + pg_stat_get_tuples_returned(tso.oid) AS seq_tup_read, + pg_stat_get_tuples_inserted(tso.oid) AS n_tup_ins, + pg_stat_get_tuples_updated(tso.oid) AS n_tup_upd, + pg_stat_get_tuples_deleted(tso.oid) AS n_tup_del, + pg_stat_get_live_tuples(tso.oid) AS n_live_tup, + pg_stat_get_dead_tuples(tso.oid) AS n_dead_tup, + pg_stat_get_vacuum_count(tso.oid) AS vacuum_count + FROM test_stat_oid AS tso + WHERE tso.name = 'test_stat_tab' + +seq_scan|seq_tup_read|n_tup_ins|n_tup_upd|n_tup_del|n_live_tup|n_dead_tup|vacuum_count +--------+------------+---------+---------+---------+----------+----------+------------ + 0| 0| 1| 0| 0| 1| 0| 0 +(1 row) + +step s2_table_drop: DROP TABLE test_stat_tab; +step s2_commit: COMMIT; + starting permutation: s1_track_counts_off s1_table_stats s1_track_counts_on pg_stat_force_next_flush ------------------------ diff --git a/src/test/isolation/specs/stats.spec b/src/test/isolation/specs/stats.spec index da16710da0f..1b0168e6176 100644 --- a/src/test/isolation/specs/stats.spec +++ b/src/test/isolation/specs/stats.spec @@ -50,6 +50,8 @@ step s1_rollback { ROLLBACK; } step s1_prepare_a { PREPARE TRANSACTION 'a'; } step s1_commit_prepared_a { COMMIT PREPARED 'a'; } step s1_rollback_prepared_a { ROLLBACK PREPARED 'a'; } +# Has to be greater than PGSTAT_ANYTIME_FLUSH_INTERVAL +step s1_sleep { SELECT pg_sleep(1.5); } # Function stats steps step s1_ff { SELECT pg_stat_force_next_flush(); } @@ -138,6 +140,7 @@ step s2_commit { COMMIT; } step s2_commit_prepared_a { COMMIT PREPARED 'a'; } step s2_rollback_prepared_a { ROLLBACK PREPARED 'a'; } step s2_ff { SELECT pg_stat_force_next_flush(); } +step s2_table_drop { DROP TABLE test_stat_tab; } # Function stats steps step s2_track_funcs_all { SET track_functions = 'all'; } @@ -435,6 +438,15 @@ permutation s1_table_drop s1_table_stats +### Check that some stats are updated (seq_scan and seq_tup_read) +### while the transaction is still running +permutation + s2_begin + s2_table_select + s1_sleep + s1_table_stats + s2_table_drop + s2_commit ### Check that we don't count changes with track counts off, but allow access ### to prior stats -- 2.34.1 --OFsrIl+bjhifp5hk-- ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2026-01-19 06:27 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2017-03-17 17:33 merging duplicate definitions of adjust_relid_set Robert Haas <[email protected]> 2017-03-17 17:50 ` Tom Lane <[email protected]> 2017-03-17 18:18 ` Robert Haas <[email protected]> 2020-12-25 06:34 [PATCH 05/20] Use cf* abstraction in archiver and tar Justin Pryzby <[email protected]> 2026-01-19 06:27 [PATCH v2 3/3] Add FLUSH_MIXED support and implement it for RELATION stats Bertrand Drouvot <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox