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 20c6e08c02..dffe70208b 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 c4956745ce..780c0e785f 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 9bac79b579..5a8447a525 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 --0qVF/w3MHQqLSynd 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: Feature: Add reloption support for table access method @ 2023-05-09 20:59 Andres Freund <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Andres Freund @ 2023-05-09 20:59 UTC (permalink / raw) To: 吴昊 <[email protected]>; +Cc: pgsql-hackers Hi, On 2023-05-05 16:44:39 +0800, 吴昊 wrote: > When I wrote an extension to implement a new storage by table access method. I found some issues > that the existing code has strong assumptions for heap tables now. Here are 3 issues that I currently have: > > > 1. Index access method has a callback to handle reloptions, but table access method hasn't. We can't > add storage-specific parameters by `WITH` clause when creating a table. Makes sense to add that. > 2. Existing code strongly assumes that the data file of a table structures by a serial physical files named > in a hard coded rule: <relfilenode>[.<segno>]. It may only fit for heap like tables. A new storage may have its > owner structure on how the data files are organized. The problem happens when dropping a table. I agree that it's not great, but I don't think it's particularly easy to fix (because things like transactional DDL require fairly tight integration). Most of the time it should be possible can also work around the limitations though. > 3. The existing code also assumes that the data file consists of a series of fix-sized block. It may not > be desired by other storage. Is there any suggestions on this situation? That's a requirement of using the buffer manager, but if you don't want to rely on that, you can use a different pattern. There's some limitations (format of TIDs, most prominently), but you should be able to deal with that. I don't think it would make sense to support other block sizes in the buffer manager. > The rest of this mail is to talk about the first issue. It looks reasonable to add a similar callback in > struct TableAmRoutine, and parse reloptions by the callback. This patch is in the attachment file. Why did you add relkind to the callbacks? The callbacks are specific to a certain relkind, so I don't think that makes sense. I don't think we really need GetTableAmRoutineByAmId() that raises nice errors etc - as the AM has already been converted to an oid, we shouldn't need to recheck? > +bytea * > +table_reloptions_am(Oid accessMethodId, Datum reloptions, char relkind, bool validate) > { > ... > > + /* built-in table access method put here to fetch TAM fast */ > + case HEAP_TABLE_AM_OID: > + tam = GetHeapamTableAmRoutine(); > + break; > default: > - /* other relkinds are not supported */ > - return NULL; > + tam = GetTableAmRoutineByAmId(accessMethodId); > + break; Why do we need this fastpath? This shouldn't be something called at a meaningful frequency? > } > + return table_reloptions(tam->amoptions, reloptions, relkind, validate); > } I'd just pass the tam, instead of an individual function. > @@ -866,6 +866,11 @@ typedef struct TableAmRoutine > struct SampleScanState *scanstate, > TupleTableSlot *slot); > > + /* > + * This callback is used to parse reloptions for relation/matview/toast. > + */ > + bytea *(*amoptions)(Datum reloptions, char relkind, bool validate); > + > } TableAmRoutine; Did you mean table instead of relation in the comment? > Another thing about reloption is that the current API passes a parameter `validate` to tell the parse > functioin to check and whether to raise an error. It doesn't have enough context when these reloptioins > are used: > 1. CREATE TABLE ... WITH(...) > 2. ALTER TABLE ... SET ... > 3. ALTER TABLE ... RESET ... > The reason why the context matters is that some reloptions are disallowed to change after creating > the table, while some reloptions are allowed. What kind of reloption are you thinking of here? Greetings, Andres Freund ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Feature: Add reloption support for table access method @ 2023-05-10 09:47 Jelte Fennema <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Jelte Fennema @ 2023-05-10 09:47 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: 吴昊 <[email protected]>; pgsql-hackers I'm definitely in favor of this general idea of supporting custom rel options, we could use that for Citus its columnar TableAM. There have been at least two other discussions on this topic: 1. https://www.postgresql.org/message-id/flat/CAFF0-CG4KZHdtYHMsonWiXNzj16gWZpduXAn8yF7pDDub%2BGQMg%40m... 2. https://www.postgresql.org/message-id/flat/429fb58fa3218221bb17c7bf9e70e1aa6cfc6b5d.camel%40j-davis.... I haven't looked at the patch in detail, but it's probably good to check what part of those previous discussions apply to it. And if there's any ideas from those previous attempts that this patch could borrow. On Tue, 9 May 2023 at 22:59, Andres Freund <[email protected]> wrote: > > Hi, > > On 2023-05-05 16:44:39 +0800, 吴昊 wrote: > > When I wrote an extension to implement a new storage by table access method. I found some issues > > that the existing code has strong assumptions for heap tables now. Here are 3 issues that I currently have: > > > > > > 1. Index access method has a callback to handle reloptions, but table access method hasn't. We can't > > add storage-specific parameters by `WITH` clause when creating a table. > > Makes sense to add that. > > > > 2. Existing code strongly assumes that the data file of a table structures by a serial physical files named > > in a hard coded rule: <relfilenode>[.<segno>]. It may only fit for heap like tables. A new storage may have its > > owner structure on how the data files are organized. The problem happens when dropping a table. > > I agree that it's not great, but I don't think it's particularly easy to fix > (because things like transactional DDL require fairly tight integration). Most > of the time it should be possible can also work around the limitations though. > > > > 3. The existing code also assumes that the data file consists of a series of fix-sized block. It may not > > be desired by other storage. Is there any suggestions on this situation? > > That's a requirement of using the buffer manager, but if you don't want to > rely on that, you can use a different pattern. There's some limitations > (format of TIDs, most prominently), but you should be able to deal with that. > > I don't think it would make sense to support other block sizes in the buffer > manager. > > > > The rest of this mail is to talk about the first issue. It looks reasonable to add a similar callback in > > struct TableAmRoutine, and parse reloptions by the callback. This patch is in the attachment file. > > Why did you add relkind to the callbacks? The callbacks are specific to a > certain relkind, so I don't think that makes sense. > > I don't think we really need GetTableAmRoutineByAmId() that raises nice > errors etc - as the AM has already been converted to an oid, we shouldn't need > to recheck? > > > > > +bytea * > > +table_reloptions_am(Oid accessMethodId, Datum reloptions, char relkind, bool validate) > > { > > ... > > > > + /* built-in table access method put here to fetch TAM fast */ > > + case HEAP_TABLE_AM_OID: > > + tam = GetHeapamTableAmRoutine(); > > + break; > > default: > > - /* other relkinds are not supported */ > > - return NULL; > > + tam = GetTableAmRoutineByAmId(accessMethodId); > > + break; > > Why do we need this fastpath? This shouldn't be something called at a > meaningful frequency? > > > > } > > + return table_reloptions(tam->amoptions, reloptions, relkind, validate); > > } > > I'd just pass the tam, instead of an individual function. > > > @@ -866,6 +866,11 @@ typedef struct TableAmRoutine > > struct SampleScanState *scanstate, > > TupleTableSlot *slot); > > > > + /* > > + * This callback is used to parse reloptions for relation/matview/toast. > > + */ > > + bytea *(*amoptions)(Datum reloptions, char relkind, bool validate); > > + > > } TableAmRoutine; > > Did you mean table instead of relation in the comment? > > > > > Another thing about reloption is that the current API passes a parameter `validate` to tell the parse > > functioin to check and whether to raise an error. It doesn't have enough context when these reloptioins > > are used: > > 1. CREATE TABLE ... WITH(...) > > 2. ALTER TABLE ... SET ... > > 3. ALTER TABLE ... RESET ... > > The reason why the context matters is that some reloptions are disallowed to change after creating > > the table, while some reloptions are allowed. > > What kind of reloption are you thinking of here? > > Greetings, > > Andres Freund > > ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2023-05-10 09:47 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]> 2023-05-09 20:59 Re: Feature: Add reloption support for table access method Andres Freund <[email protected]> 2023-05-10 09:47 ` Re: Feature: Add reloption support for table access method Jelte Fennema <[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