public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 01/10] Allow alternate compression methods for wal_compression 5+ messages / 4 participants [nested] [flat]
* [PATCH 01/10] Allow alternate compression methods for wal_compression @ 2021-02-27 04:03 Andrey Borodin <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Andrey Borodin @ 2021-02-27 04:03 UTC (permalink / raw) TODO: bump XLOG_PAGE_MAGIC --- doc/src/sgml/config.sgml | 17 +++++ src/backend/Makefile | 2 +- src/backend/access/transam/xlog.c | 10 +++ src/backend/access/transam/xloginsert.c | 52 +++++++++++++-- src/backend/access/transam/xlogreader.c | 63 ++++++++++++++++++- src/backend/utils/misc/guc.c | 11 ++++ src/backend/utils/misc/postgresql.conf.sample | 1 + src/include/access/xlog.h | 1 + src/include/access/xlog_internal.h | 8 +++ src/include/access/xlogreader.h | 1 + src/include/access/xlogrecord.h | 9 +-- 11 files changed, 163 insertions(+), 12 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index a218d78bef..7fb2a84626 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -3072,6 +3072,23 @@ include_dir 'conf.d' </listitem> </varlistentry> + <varlistentry id="guc-wal-compression-method" xreflabel="wal_compression_method"> + <term><varname>wal_compressionion_method</varname> (<type>enum</type>) + <indexterm> + <primary><varname>wal_compression_method</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + This parameter selects the compression method used to compress WAL when + <varname>wal_compression</varname> is enabled. + The supported methods are pglz and zlib. + The default value is <literal>pglz</literal>. + Only superusers can change this setting. + </para> + </listitem> + </varlistentry> + <varlistentry id="guc-wal-init-zero" xreflabel="wal_init_zero"> <term><varname>wal_init_zero</varname> (<type>boolean</type>) <indexterm> diff --git a/src/backend/Makefile b/src/backend/Makefile index 0da848b1fd..3af216ddfc 100644 --- a/src/backend/Makefile +++ b/src/backend/Makefile @@ -48,7 +48,7 @@ OBJS = \ LIBS := $(filter-out -lpgport -lpgcommon, $(LIBS)) $(LDAP_LIBS_BE) $(ICU_LIBS) # The backend doesn't need everything that's in LIBS, however -LIBS := $(filter-out -lz -lreadline -ledit -ltermcap -lncurses -lcurses, $(LIBS)) +LIBS := $(filter-out -lreadline -ledit -ltermcap -lncurses -lcurses, $(LIBS)) ifeq ($(with_systemd),yes) LIBS += -lsystemd diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index e04250f4e9..04192b7add 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -99,6 +99,7 @@ bool EnableHotStandby = false; bool fullPageWrites = true; bool wal_log_hints = false; bool wal_compression = false; +int wal_compression_method = WAL_COMPRESSION_PGLZ; char *wal_consistency_checking_string = NULL; bool *wal_consistency_checking = NULL; bool wal_init_zero = true; @@ -180,6 +181,15 @@ const struct config_enum_entry recovery_target_action_options[] = { {NULL, 0, false} }; +/* Note that due to conditional compilation, offsets within the array are not static */ +const struct config_enum_entry wal_compression_options[] = { + {"pglz", WAL_COMPRESSION_PGLZ, false}, +#ifdef HAVE_LIBZ + {"zlib", WAL_COMPRESSION_ZLIB, false}, +#endif + {NULL, 0, false} +}; + /* * Statistics for current checkpoint are collected in this global struct. * Because only the checkpointer or a stand-alone backend can perform diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c index 7052dc245e..34e1227381 100644 --- a/src/backend/access/transam/xloginsert.c +++ b/src/backend/access/transam/xloginsert.c @@ -33,6 +33,10 @@ #include "storage/proc.h" #include "utils/memutils.h" +#ifdef HAVE_LIBZ +#include <zlib.h> +#endif + /* Buffer size required to store a compressed version of backup block image */ #define PGLZ_MAX_BLCKSZ PGLZ_MAX_OUTPUT(BLCKSZ) @@ -113,7 +117,8 @@ static XLogRecData *XLogRecordAssemble(RmgrId rmid, uint8 info, XLogRecPtr RedoRecPtr, bool doPageWrites, XLogRecPtr *fpw_lsn, int *num_fpi); static bool XLogCompressBackupBlock(char *page, uint16 hole_offset, - uint16 hole_length, char *dest, uint16 *dlen); + uint16 hole_length, char *dest, + uint16 *dlen, WalCompression compression); /* * Begin constructing a WAL record. This must be called before the @@ -630,11 +635,12 @@ XLogRecordAssemble(RmgrId rmid, uint8 info, */ if (wal_compression) { + bimg.compression_method = wal_compression_method; is_compressed = XLogCompressBackupBlock(page, bimg.hole_offset, cbimg.hole_length, regbuf->compressed_page, - &compressed_len); + &compressed_len, bimg.compression_method); } /* @@ -827,7 +833,7 @@ XLogRecordAssemble(RmgrId rmid, uint8 info, */ static bool XLogCompressBackupBlock(char *page, uint16 hole_offset, uint16 hole_length, - char *dest, uint16 *dlen) + char *dest, uint16 *dlen, WalCompression compression) { int32 orig_len = BLCKSZ - hole_length; int32 len; @@ -853,12 +859,48 @@ XLogCompressBackupBlock(char *page, uint16 hole_offset, uint16 hole_length, else source = page; + switch (compression) + { + case WAL_COMPRESSION_PGLZ: + len = pglz_compress(source, orig_len, dest, PGLZ_strategy_default); + break; + +#ifdef HAVE_LIBZ + case WAL_COMPRESSION_ZLIB: + { + unsigned long len_l = PGLZ_MAX_BLCKSZ; + int ret; + ret = compress2((Bytef*)dest, &len_l, (Bytef*)source, orig_len, 1); + if (ret != Z_OK) + { + // XXX: using an interface other than compress() would allow giving a better error message + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("failed compressing zlib (%d)", ret))); + len_l = -1; + } + len = len_l; + break; + } +#endif + + default: + /* + * It should be impossible to get here for unsupported algorithms, + * which cannot be assigned if they're not enabled at compile time. + */ + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("unknown compression method requested: %d(%s)", + compression, wal_compression_name(compression)))); + + } + /* - * We recheck the actual size even if pglz_compress() reports success and + * We recheck the actual size even if compression reports success and * see if the number of bytes saved by compression is larger than the * length of extra data needed for the compressed version of block image. */ - len = pglz_compress(source, orig_len, dest, PGLZ_strategy_default); if (len >= 0 && len + extra_bytes < orig_len) { diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c index 42738eb940..afca22a26c 100644 --- a/src/backend/access/transam/xlogreader.c +++ b/src/backend/access/transam/xlogreader.c @@ -33,6 +33,10 @@ #include "utils/memutils.h" #endif +#ifdef HAVE_LIBZ +#include <zlib.h> +#endif + static void report_invalid_record(XLogReaderState *state, const char *fmt,...) pg_attribute_printf(2, 3); static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength); @@ -1286,6 +1290,7 @@ DecodeXLogRecord(XLogReaderState *state, XLogRecord *record, char **errormsg) { COPY_HEADER_FIELD(&blk->bimg_len, sizeof(uint16)); COPY_HEADER_FIELD(&blk->hole_offset, sizeof(uint16)); + COPY_HEADER_FIELD(&blk->compression_method, sizeof(uint8)); COPY_HEADER_FIELD(&blk->bimg_info, sizeof(uint8)); blk->apply_image = ((blk->bimg_info & BKPIMAGE_APPLY) != 0); @@ -1535,6 +1540,29 @@ XLogRecGetBlockData(XLogReaderState *record, uint8 block_id, Size *len) } } +/* + * Return a statically allocated string associated with the given compression + * method. This is similar to the guc, but isn't subject to conditional + * compilation. + */ +const char * +wal_compression_name(WalCompression compression) +{ + /* + * This could index into the guc array, except that it's compiled + * conditionally and unsupported methods are elided. + */ + switch (compression) + { + case WAL_COMPRESSION_PGLZ: + return "pglz"; + case WAL_COMPRESSION_ZLIB: + return "zlib"; + default: + return "???"; + } +} + /* * Restore a full-page image from a backup block attached to an XLOG record. * @@ -1558,8 +1586,39 @@ RestoreBlockImage(XLogReaderState *record, uint8 block_id, char *page) if (bkpb->bimg_info & BKPIMAGE_IS_COMPRESSED) { /* If a backup block image is compressed, decompress it */ - if (pglz_decompress(ptr, bkpb->bimg_len, tmp.data, - BLCKSZ - bkpb->hole_length, true) < 0) + int32 decomp_result = -1; + switch (bkpb->compression_method) + { + case WAL_COMPRESSION_PGLZ: + decomp_result = pglz_decompress(ptr, bkpb->bimg_len, tmp.data, + BLCKSZ - bkpb->hole_length, true); + break; + +#ifdef HAVE_LIBZ + case WAL_COMPRESSION_ZLIB: + { + unsigned long decomp_result_l; + decomp_result_l = BLCKSZ - bkpb->hole_length; + if (uncompress((Bytef*)tmp.data, &decomp_result_l, + (Bytef*)ptr, bkpb->bimg_len) == Z_OK) + decomp_result = decomp_result_l; + else + decomp_result = -1; + break; + } +#endif + + default: + report_invalid_record(record, "image at %X/%X is compressed with unsupported codec, block %d (%d/%s)", + (uint32) (record->ReadRecPtr >> 32), + (uint32) record->ReadRecPtr, + block_id, + bkpb->compression_method, + wal_compression_name(bkpb->compression_method)); + return false; + } + + if (decomp_result < 0) { report_invalid_record(record, "invalid compressed image at %X/%X, block %d", LSN_FORMAT_ARGS(record->ReadRecPtr), diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 855076b1fd..8084027465 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -508,6 +508,7 @@ extern const struct config_enum_entry archive_mode_options[]; extern const struct config_enum_entry recovery_target_action_options[]; extern const struct config_enum_entry sync_method_options[]; extern const struct config_enum_entry dynamic_shared_memory_options[]; +extern const struct config_enum_entry wal_compression_options[]; /* * GUC option variables that are exported from this module @@ -4721,6 +4722,16 @@ static struct config_enum ConfigureNamesEnum[] = NULL, NULL, NULL }, + { + {"wal_compression_method", PGC_SIGHUP, WAL_SETTINGS, + gettext_noop("Set the method used to compress full page images in the WAL."), + NULL + }, + &wal_compression_method, + WAL_COMPRESSION_PGLZ, wal_compression_options, + NULL, NULL, NULL + }, + { {"dynamic_shared_memory_type", PGC_POSTMASTER, RESOURCES_MEM, gettext_noop("Selects the dynamic shared memory implementation used."), diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index f46c2dd7a8..ef69a94492 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -213,6 +213,7 @@ # open_sync #full_page_writes = on # recover from partial page writes #wal_compression = off # enable compression of full-page writes +#wal_compression_method = pglz # pglz, zlib #wal_log_hints = off # also do full page writes of non-critical updates # (change requires restart) #wal_init_zero = on # zero-fill new WAL files diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h index 6d384d3ce6..fa2e5c611f 100644 --- a/src/include/access/xlog.h +++ b/src/include/access/xlog.h @@ -117,6 +117,7 @@ extern bool EnableHotStandby; extern bool fullPageWrites; extern bool wal_log_hints; extern bool wal_compression; +extern int wal_compression_method; extern bool wal_init_zero; extern bool wal_recycle; extern bool *wal_consistency_checking; diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h index b23e286406..d653839b97 100644 --- a/src/include/access/xlog_internal.h +++ b/src/include/access/xlog_internal.h @@ -324,4 +324,12 @@ extern bool InArchiveRecovery; extern bool StandbyMode; extern char *recoveryRestoreCommand; +typedef enum WalCompression +{ + WAL_COMPRESSION_PGLZ, + WAL_COMPRESSION_ZLIB, +} WalCompression; + +extern const char *wal_compression_name(WalCompression compression); + #endif /* XLOG_INTERNAL_H */ diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h index 21d200d3df..3d19c315d7 100644 --- a/src/include/access/xlogreader.h +++ b/src/include/access/xlogreader.h @@ -133,6 +133,7 @@ typedef struct bool apply_image; /* has image that should be restored */ char *bkp_image; uint16 hole_offset; + uint8 compression_method; uint16 hole_length; uint16 bimg_len; uint8 bimg_info; diff --git a/src/include/access/xlogrecord.h b/src/include/access/xlogrecord.h index 80c92a2498..0d4c212f15 100644 --- a/src/include/access/xlogrecord.h +++ b/src/include/access/xlogrecord.h @@ -114,7 +114,7 @@ typedef struct XLogRecordBlockHeader * present is (BLCKSZ - <length of "hole" bytes>). * * Additionally, when wal_compression is enabled, we will try to compress full - * page images using the PGLZ compression algorithm, after removing the "hole". + * page images, after removing the "hole". * This can reduce the WAL volume, but at some extra cost of CPU spent * on the compression during WAL logging. In this case, since the "hole" * length cannot be calculated by subtracting the number of page image bytes @@ -129,9 +129,10 @@ typedef struct XLogRecordBlockHeader */ typedef struct XLogRecordBlockImageHeader { - uint16 length; /* number of page image bytes */ - uint16 hole_offset; /* number of bytes before "hole" */ - uint8 bimg_info; /* flag bits, see below */ + uint16 length; /* number of page image bytes */ + uint16 hole_offset; /* number of bytes before "hole" */ + uint8 compression_method; /* compression method used for image */ + uint8 bimg_info; /* flag bits, see below */ /* * If BKPIMAGE_HAS_HOLE and BKPIMAGE_IS_COMPRESSED, an -- 2.17.0 --0qVF/w3MHQqLSynd Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0002-Run-011_crash_recovery.pl-with-wal_level-minimal.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: a wrong index choose when statistics is out of date @ 2024-03-04 05:33 David Rowley <[email protected]> 0 siblings, 2 replies; 5+ messages in thread From: David Rowley @ 2024-03-04 05:33 UTC (permalink / raw) To: Andy Fan <[email protected]>; +Cc: pgsql-hackers On Sun, 3 Mar 2024 at 20:08, Andy Fan <[email protected]> wrote: > The issue can be reproduced with the following steps: > > create table x_events (.., created_at timestamp, a int, b int); > > create index idx_1 on t(created_at, a); > create index idx_2 on t(created_at, b); > > query: > select * from t where create_at = current_timestamp and b = 1; > > index (created_at, a) rather than (created_at, b) may be chosen for the > above query if the statistics think "create_at = current_timestamp" has > no rows, then both index are OK, actually it is true just because > statistics is out of date. I don't think there's really anything too special about the fact that the created_at column is always increasing. We commonly get 1-row estimates after multiplying the selectivities from individual stats. Your example just seems like yet another reason that this could happen. I've been periodically talking about introducing "risk" as a factor that the planner should consider. I did provide some detail in [1] about the design that was in my head at that time. I'd not previously thought that it could also solve this problem, but after reading your email, I think it can. I don't think it would be right to fudge the costs in any way, but I think the risk factor for IndexPaths could take into account the number of unmatched index clauses and increment the risk factor, or "certainty_factor" as it is currently in my brain-based design. That way add_path() would be more likely to prefer the index that matches the most conditions. The exact maths to calculate the certainty_factor for this case I don't quite have worked out yet. I plan to work on documenting the design of this and try and get a prototype patch out sometime during this coming southern hemisphere winter so that there's at least a full cycle of feedback opportunity before the PG18 freeze. We should do anything like add column options in the meantime. Those are hard to remove once added. David [1] https://www.postgresql.org/message-id/CAApHDvo2sMPF9m%3Di%2BYPPUssfTV1GB%3DZ8nMVa%2B9Uq4RZJ8sULeQ%40... ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: a wrong index choose when statistics is out of date @ 2024-03-04 11:20 Andy Fan <[email protected]> parent: David Rowley <[email protected]> 1 sibling, 1 reply; 5+ messages in thread From: Andy Fan @ 2024-03-04 11:20 UTC (permalink / raw) To: David Rowley <[email protected]>; +Cc: pgsql-hackers David Rowley <[email protected]> writes: > On Sun, 3 Mar 2024 at 20:08, Andy Fan <[email protected]> wrote: >> The issue can be reproduced with the following steps: >> >> create table x_events (.., created_at timestamp, a int, b int); >> >> create index idx_1 on t(created_at, a); >> create index idx_2 on t(created_at, b); >> >> query: >> select * from t where create_at = current_timestamp and b = 1; >> >> index (created_at, a) rather than (created_at, b) may be chosen for the >> above query if the statistics think "create_at = current_timestamp" has >> no rows, then both index are OK, actually it is true just because >> statistics is out of date. > > I don't think there's really anything too special about the fact that > the created_at column is always increasing. We commonly get 1-row > estimates after multiplying the selectivities from individual stats. > Your example just seems like yet another reason that this could > happen. You are right about there are more cases which lead this happen. However this is the only case where the created_at = $1 trick can works, which was the problem I wanted to resove when I was writing. > I've been periodically talking about introducing "risk" as a factor > that the planner should consider. I did provide some detail in [1] > about the design that was in my head at that time. I'd not previously > thought that it could also solve this problem, but after reading your > email, I think it can. Haha, I remeber you were against "risk factor" before at [1], and at that time we are talking about the exact same topic as here, and I proposaled another risk factor. Without an agreement, I did it in my own internal version and get hurted then, something like I didn't pay enough attention to Bitmap Index Scan and Index scan. Then I forget the "risk factor". > > I don't think it would be right to fudge the costs in any way, but I > think the risk factor for IndexPaths could take into account the > number of unmatched index clauses and increment the risk factor, or > "certainty_factor" as it is currently in my brain-based design. That > way add_path() would be more likely to prefer the index that matches > the most conditions. This is somehow similar with my proposal at [1]? What do you think about the treat 'col op const' as 'col op $1' for the marked column? This could just resolve a subset of questions in your mind, but the method looks have a solid reason. Currently I treat the risk factor as what you did before, but this maybe another time for me to switch my mind again. [1] https://www.postgresql.org/message-id/CAApHDvovVWCbeR4v%2BA4Dkwb%3DYS_GuJG9OyCm8jZu%2B%2BcP2xsY_A%40... -- Best Regards Andy Fan ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: a wrong index choose when statistics is out of date @ 2024-03-04 17:03 Tomas Vondra <[email protected]> parent: David Rowley <[email protected]> 1 sibling, 0 replies; 5+ messages in thread From: Tomas Vondra @ 2024-03-04 17:03 UTC (permalink / raw) To: David Rowley <[email protected]>; Andy Fan <[email protected]>; +Cc: pgsql-hackers On 3/4/24 06:33, David Rowley wrote: > On Sun, 3 Mar 2024 at 20:08, Andy Fan <[email protected]> wrote: >> The issue can be reproduced with the following steps: >> >> create table x_events (.., created_at timestamp, a int, b int); >> >> create index idx_1 on t(created_at, a); >> create index idx_2 on t(created_at, b); >> >> query: >> select * from t where create_at = current_timestamp and b = 1; >> >> index (created_at, a) rather than (created_at, b) may be chosen for the >> above query if the statistics think "create_at = current_timestamp" has >> no rows, then both index are OK, actually it is true just because >> statistics is out of date. > > I don't think there's really anything too special about the fact that > the created_at column is always increasing. We commonly get 1-row > estimates after multiplying the selectivities from individual stats. > Your example just seems like yet another reason that this could > happen. > > I've been periodically talking about introducing "risk" as a factor > that the planner should consider. I did provide some detail in [1] > about the design that was in my head at that time. I'd not previously > thought that it could also solve this problem, but after reading your > email, I think it can. > > I don't think it would be right to fudge the costs in any way, but I > think the risk factor for IndexPaths could take into account the > number of unmatched index clauses and increment the risk factor, or > "certainty_factor" as it is currently in my brain-based design. That > way add_path() would be more likely to prefer the index that matches > the most conditions. > > The exact maths to calculate the certainty_factor for this case I > don't quite have worked out yet. I plan to work on documenting the > design of this and try and get a prototype patch out sometime during > this coming southern hemisphere winter so that there's at least a full > cycle of feedback opportunity before the PG18 freeze. > I've been thinking about this stuff too, so I'm curious to hear what kind of plan you come up with. Every time I tried to formalize a more concrete plan, I ended up with a very complex (and possible yet more fragile) approach. I think we'd need to consider a couple things: 1) reliability of cardinality estimates I think this is pretty much the same concept as confidence intervals, i.e. knowing not just the regular estimate, but also a range where the actual value lies with high confidence (e.g. 90%). For a single clauses this might not be terribly difficult, but for more complex cases (multiple conditions, ...) it seems far more complex :-( For example, let's say we know confidence intervals for two conditions. What's the confidence interval when combined using AND or OR? 2) robustness of the paths Knowing just the confidence intervals does not seem sufficient, though. The other thing that matters is how this affects the paths, how robust the paths are. I mean, if we have alternative paths with costs that flip somewhere in the confidence interval - which one to pick? Surely one thing to consider is how fast the costs change for each path. 3) complexity of the model I suppose we'd prefer a systematic approach (and not some ad hoc solution for one particular path/plan type). So this would be somehow integrated into the cost model, making it yet more complex. I'm quite worried about this (not necessarily because of performance reasons). I wonder if trying to improve the robustness solely by changes in the planning phase is a very promising approach. I mean - sure, we should improve that, but by definition it relies on a priori information. And not only the stats may be stale - it's a very lossy approximation of the actual data. Even if the stats are perfectly up to date / very detailed, there's still going to be a lot of uncertainty. I wonder if we'd be better off if we experimented with more robust plans, like SmoothScan [1] or g-join [2]. regards [1] https://stratos.seas.harvard.edu/sites/scholar.harvard.edu/files/stratos/files/smooth_vldbj.pdf [2] http://wwwlgis.informatik.uni-kl.de/cms/fileadmin/users/haerder/2011/JoinAndGrouping.pdf -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: a wrong index choose when statistics is out of date @ 2024-03-04 23:13 David Rowley <[email protected]> parent: Andy Fan <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: David Rowley @ 2024-03-04 23:13 UTC (permalink / raw) To: Andy Fan <[email protected]>; +Cc: pgsql-hackers On Tue, 5 Mar 2024 at 00:37, Andy Fan <[email protected]> wrote: > > David Rowley <[email protected]> writes: > > I don't think it would be right to fudge the costs in any way, but I > > think the risk factor for IndexPaths could take into account the > > number of unmatched index clauses and increment the risk factor, or > > "certainty_factor" as it is currently in my brain-based design. That > > way add_path() would be more likely to prefer the index that matches > > the most conditions. > > This is somehow similar with my proposal at [1]? What do you think > about the treat 'col op const' as 'col op $1' for the marked column? > This could just resolve a subset of questions in your mind, but the > method looks have a solid reason. Do you mean this? > + /* > + * To make the planner more robust to handle some inaccurate statistics > + * issue, we will add a extra cost to qpquals so that the less qpquals > + * the lower cost it has. > + */ > + cpu_run_cost += 0.01 * list_length(qpquals); I don't think it's a good idea to add cost penalties like you proposed there. This is what I meant by "I don't think it would be right to fudge the costs in any way". If you modify the costs to add some small penalty so that the planner is more likely to favour some other plan, what happens if we then decide the other plan has some issue and we want to penalise that for some other reason? Adding the 2nd penalty might result in the original plan choice again. Which one should be penalised more? I think the uncertainty needs to be tracked separately. Fudging the costs like this is also unlikely to play nicely with add_path's use of STD_FUZZ_FACTOR. There'd be an incentive to do things like total_cost *= STD_FUZZ_FACTOR; to ensure we get a large enough penalty. David > [1] https://www.postgresql.org/message-id/CAApHDvovVWCbeR4v%2BA4Dkwb%3DYS_GuJG9OyCm8jZu%2B%2BcP2xsY_A%40... ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2024-03-04 23:13 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-02-27 04:03 [PATCH 01/10] Allow alternate compression methods for wal_compression Andrey Borodin <[email protected]> 2024-03-04 05:33 Re: a wrong index choose when statistics is out of date David Rowley <[email protected]> 2024-03-04 11:20 ` Re: a wrong index choose when statistics is out of date Andy Fan <[email protected]> 2024-03-04 23:13 ` Re: a wrong index choose when statistics is out of date David Rowley <[email protected]> 2024-03-04 17:03 ` Re: a wrong index choose when statistics is out of date Tomas Vondra <[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