public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 08/10] Default to zstd.. 3+ messages / 3 participants [nested] [flat]
* [PATCH 08/10] Default to zstd.. @ 2021-03-12 21:35 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Justin Pryzby @ 2021-03-12 21:35 UTC (permalink / raw) for CI, not for merge --- configure | 6 ++++-- configure.ac | 2 +- src/backend/access/transam/xlog.c | 2 +- src/backend/utils/misc/guc.c | 2 +- 4 files changed, 7 insertions(+), 5 deletions(-) diff --git a/configure b/configure index 81e23418b2..253f028fc4 100755 --- a/configure +++ b/configure @@ -1582,7 +1582,7 @@ Optional Packages: use system time zone data in DIR --without-zlib do not use Zlib --without-lz4 build without LZ4 support - --with-zstd build with Zstd compression library + --without-zstd build without Zstd compression library --with-gnu-ld assume the C compiler uses GNU ld [default=no] --with-ssl=LIB use LIB for SSL/TLS support (openssl) --with-openssl obsolete spelling of --with-ssl=openssl @@ -8740,7 +8740,9 @@ $as_echo "#define USE_ZSTD 1" >>confdefs.h esac else - with_zstd=no + with_zstd=yes + +$as_echo "#define USE_ZSTD 1" >>confdefs.h fi diff --git a/configure.ac b/configure.ac index d6f6349067..8d72710fa7 100644 --- a/configure.ac +++ b/configure.ac @@ -1005,7 +1005,7 @@ fi # ZSTD # AC_MSG_CHECKING([whether to build with zstd support]) -PGAC_ARG_BOOL(with, zstd, no, [build with Zstd compression library], +PGAC_ARG_BOOL(with, zstd, yes, [build without Zstd compression library], [AC_DEFINE([USE_ZSTD], 1, [Define to 1 to build with zstd support. (--with-zstd)])]) AC_MSG_RESULT([$with_zstd]) AC_SUBST(with_zstd) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 307eee6626..92023de9f5 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -99,7 +99,7 @@ bool EnableHotStandby = false; bool fullPageWrites = true; bool wal_log_hints = false; bool wal_compression = false; -int wal_compression_method = WAL_COMPRESSION_LZ4; +int wal_compression_method = WAL_COMPRESSION_ZSTD; char *wal_consistency_checking_string = NULL; bool *wal_consistency_checking = NULL; bool wal_init_zero = true; diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 52f9cd0242..8031e027aa 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -4728,7 +4728,7 @@ static struct config_enum ConfigureNamesEnum[] = NULL }, &wal_compression_method, - WAL_COMPRESSION_LZ4, wal_compression_options, + WAL_COMPRESSION_ZSTD, wal_compression_options, NULL, NULL, NULL }, -- 2.17.0 --jozmn01XJZjDjM3N Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0009-Add-zstd-compression-levels.patch" ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: unnecessary executor overheads around seqscans @ 2026-01-24 06:21 Amit Langote <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Amit Langote @ 2026-01-24 06:21 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: pgsql-hackers; Amit Kapila <[email protected]>; David Rowley <[email protected]> Hi, On Sat, Jan 24, 2026 at 5:16 AM Andres Freund <[email protected]> wrote: > > Hi, > > In [1] I was looking at the profile of a seqscan with a where clause that > doesn't match any of the many rows. I was a bit saddened by where we were > spending time. > > > - The fetching of variables, as well as the null check of scandesc, in > SeqNext() is repeated in every loop iteration of ExecScanExtended, despite > that obviously not being required after the first iteration > > We could perhaps address this by moving the check to the callers of > ExecScanExtended() or by extending ExecScanExtended to have an explicit > beginscan callback that it calls after. > > Or perhaps we could just make it so that the entire if (scandesc == NULL) > branch isn't needed? Kind of like ExecProcNodeFirst(), what if we replace the variant selection in ExecInitSeqScan() with just: scanstate->ss.ps.ExecProcNode = ExecSeqScanFirst; ExecSeqScanFirst() would: - do the table_beginscan() call that's currently inside the if (scandesc == NULL) block in SeqNext() - select and install the appropriate ExecSeqScan variant based on qual/projection/EPQ - call that variant to fetch and return the first tuple. Then we can just remove the if (scandesc == NULL) block from SeqNext() entirely. > - The checkXidAlive checks that have been added to table_scan_getnextslot() > show up noticeably and in every loop iteration, despite afaict never being reachable > > It's not obvious to me that this should > a) be in table_scan_getnextslot(), rather than in beginscan - how could it > change in the middle of a scan? That would require a wrapper around > rd_tableam->scan_begin(), but that seems like it might be good anyway. > b) not just be an assertion? Haven't thought about this. > - The TupIsNull(slot) check in ExecScanExtended() is redundant with the return > value of table_scan_getnextslot(), but the compiler doesn't grok that. > > We can use a pg_assume() in table_scan_getnextslot() to make the compiler > understand. Something like this? result = sscan->rs_rd->rd_tableam->scan_getnextslot(sscan, direction, slot); pg_assume(result == !TupIsNull(slot)); return result; I assume this relies on table_scan_getnextslot() being inlined into ExecScanExtended()? > - We repeatedly store the table oid in the slot, table_scan_getnextslot() and > then again in ExecStoreBufferHeapTuple(). This shows up in the profile. > > I wish we had made the slot a property of the scan, that way the scan could > assume the slot already has the oid set... I've noticed this when working on my batching patch. I set tts_tableOid when creating the slots used in the batch themselves, so the per-tuple assignment isn't needed. > - heap_getnextslot() calls ExecStoreBufferHeapTuple() and then returns > true. That prevents the sibiling call optimization. > > We should change ExecStoreBufferHeapTuple() to return true. Nobody uses the > current return value. Alternatively we should consider just moving it to > somewhere heapam.c/heapam_handler.c can see the implementations, they're the > only ones that should use it anyway. Makes sense. Changing ExecStoreBufferHeapTuple() to return true seems like the simpler option, unless I misunderstood. -- Thanks, Amit Langote ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: unnecessary executor overheads around seqscans @ 2026-01-24 06:36 David Rowley <[email protected]> parent: Amit Langote <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: David Rowley @ 2026-01-24 06:36 UTC (permalink / raw) To: Amit Langote <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers; Amit Kapila <[email protected]> On Sat, 24 Jan 2026 at 19:21, Amit Langote <[email protected]> wrote: > On Sat, Jan 24, 2026 at 5:16 AM Andres Freund <[email protected]> wrote: > > Or perhaps we could just make it so that the entire if (scandesc == NULL) > > branch isn't needed? > > Kind of like ExecProcNodeFirst(), what if we replace the variant > selection in ExecInitSeqScan() with just: I imagined moving it to ExecInitSeqScan() and just avoid doing it when we're doing EXPLAIN or we're doing a parallel scan. Something like the attached, which is giving me a 4% speedup selecting from a million row table with a single int column running a seqscan query with a WHERE clause matching no rows. > > We should change ExecStoreBufferHeapTuple() to return true. Nobody uses the > > current return value. Alternatively we should consider just moving it to > > somewhere heapam.c/heapam_handler.c can see the implementations, they're the > > only ones that should use it anyway. > > Makes sense. Changing ExecStoreBufferHeapTuple() to return true seems > like the simpler option, unless I misunderstood. It's probably too late to change it now, but wouldn't it have been better if scan_getnextslot had been coded to return the TupleTableSlot rather than bool? That way you could get the sibling call in ExecStoreBufferHeapTuple() and in SeqNext(). I also noticed my compiler does not inline SeqNext(). Adding a pg_attribute_always_inline results in it getting inlined and gives a small speedup. David diff --git a/src/backend/executor/nodeSeqscan.c b/src/backend/executor/nodeSeqscan.c index b8119face43..87420e60dc9 100644 --- a/src/backend/executor/nodeSeqscan.c +++ b/src/backend/executor/nodeSeqscan.c @@ -63,17 +63,6 @@ SeqNext(SeqScanState *node) direction = estate->es_direction; slot = node->ss.ss_ScanTupleSlot; - if (scandesc == NULL) - { - /* - * We reach here if the scan is not parallel, or if we're serially - * executing a scan that was planned to be parallel. - */ - scandesc = table_beginscan(node->ss.ss_currentRelation, - estate->es_snapshot, - 0, NULL); - node->ss.ss_currentScanDesc = scandesc; - } /* * get the next tuple from the table @@ -258,6 +247,21 @@ ExecInitSeqScan(SeqScan *node, EState *estate, int eflags) scanstate->ss.ps.qual = ExecInitQual(node->scan.plan.qual, (PlanState *) scanstate); + /* + * Build the TableScanDesc unless we're just doing an EXPLAIN without + * ANALYZE. Parallel SeqScan's TableScanDesc is built by + * ExecSeqScanInitializeDSM or ExecSeqScanInitializeWorker. + */ + if ((eflags & EXEC_FLAG_EXPLAIN_ONLY) == 0 && + node->scan.plan.parallel_aware == false) + { + scanstate->ss.ss_currentScanDesc = + table_beginscan(scanstate->ss.ss_currentRelation, + estate->es_snapshot, + 0, + NULL); + } + /* * When EvalPlanQual() is not in use, assign ExecProcNode for this node * based on the presence of qual and projection. Each ExecSeqScan*() Attachments: [text/plain] move_table_beginscan_to_initplan.patch (1.4K, ../../CAApHDvrL7Q41B=gv+3wc8+AJGKZugGegUbBo8FPQ+3+NGTPb+w@mail.gmail.com/2-move_table_beginscan_to_initplan.patch) download | inline diff: diff --git a/src/backend/executor/nodeSeqscan.c b/src/backend/executor/nodeSeqscan.c index b8119face43..87420e60dc9 100644 --- a/src/backend/executor/nodeSeqscan.c +++ b/src/backend/executor/nodeSeqscan.c @@ -63,17 +63,6 @@ SeqNext(SeqScanState *node) direction = estate->es_direction; slot = node->ss.ss_ScanTupleSlot; - if (scandesc == NULL) - { - /* - * We reach here if the scan is not parallel, or if we're serially - * executing a scan that was planned to be parallel. - */ - scandesc = table_beginscan(node->ss.ss_currentRelation, - estate->es_snapshot, - 0, NULL); - node->ss.ss_currentScanDesc = scandesc; - } /* * get the next tuple from the table @@ -258,6 +247,21 @@ ExecInitSeqScan(SeqScan *node, EState *estate, int eflags) scanstate->ss.ps.qual = ExecInitQual(node->scan.plan.qual, (PlanState *) scanstate); + /* + * Build the TableScanDesc unless we're just doing an EXPLAIN without + * ANALYZE. Parallel SeqScan's TableScanDesc is built by + * ExecSeqScanInitializeDSM or ExecSeqScanInitializeWorker. + */ + if ((eflags & EXEC_FLAG_EXPLAIN_ONLY) == 0 && + node->scan.plan.parallel_aware == false) + { + scanstate->ss.ss_currentScanDesc = + table_beginscan(scanstate->ss.ss_currentRelation, + estate->es_snapshot, + 0, + NULL); + } + /* * When EvalPlanQual() is not in use, assign ExecProcNode for this node * based on the presence of qual and projection. Each ExecSeqScan*() ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2026-01-24 06:36 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-03-12 21:35 [PATCH 08/10] Default to zstd.. Justin Pryzby <[email protected]> 2026-01-24 06:21 Re: unnecessary executor overheads around seqscans Amit Langote <[email protected]> 2026-01-24 06:36 ` Re: unnecessary executor overheads around seqscans David Rowley <[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