Translatable strings with formatting of 64bit values
Hi,
apparently gettext can't properly identify strings when 64bit values
formatted with macros like INT64_FORMAT and UINT64_FORMAT. I did
some research and found out that gettext can work with PRId64 and
PRIu64. My suggestion is to use these macro for such strings.
The problem is here that PRIu64 is not accessible on all platforms but
this is easy solvable if it will be specified using INT64_MODIFIER in
c.h.
I attached a sample patch that adds PRIu64, PRId64 and makes few strings
translatable.
Using PRId64 will simplify the code like this:
charbufv[100],
bufm[100],
bufx[100];
snprintf(bufv, sizeof(bufv), INT64_FORMAT, next);
snprintf(bufm, sizeof(bufm), INT64_FORMAT, minv);
snprintf(bufx, sizeof(bufx), INT64_FORMAT, maxv);
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("setval: value %s is out of
bounds for sequence \"%s\" (%s..%s)", bufv,
RelationGetRelationName(seqrel), bufm, bufx)));
To:
if ((next < minv) || (next > maxv))
ereport(ERROR,
(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
errmsg("setval: value %s is out of
bounds for sequence \"%" PRId64 "\" (%" PRId64 "..%" PRId64
")", next, RelationGetRelationName(seqrel), minv, maxv)));
In result:
#: commands/sequence.c:944
#, fuzzy, c-format
#| msgid "setval: value %s is out of bounds for sequence
\"%s\" (%s..%s)"
msgid "setval: value %s is out of bounds for sequence
\"%\" (%..%)"
msgstr "setval передано значение %s вне пределов последовательности
\"%s\" (%s..%s)"
And still this string will be translatable. I found a bunch of places
when PRIx64 macros can simplify the code.
--
---
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
diff --git a/src/bin/pg_controldata/pg_controldata.c b/src/bin/pg_controldata/pg_controldata.c
index 7d435ffa57..8f1ac7816f 100644
--- a/src/bin/pg_controldata/pg_controldata.c
+++ b/src/bin/pg_controldata/pg_controldata.c
@@ -97,7 +97,6 @@ main(int argc, char *argv[])
time_t time_tmp;
char pgctime_str[128];
char ckpttime_str[128];
- char sysident_str[32];
char mock_auth_nonce_str[MOCK_AUTH_NONCE_LEN * 2 + 1];
const char *strftime_fmt = "%c";
const char *progname;
@@ -219,8 +218,6 @@ main(int argc, char *argv[])
* keep platform-dependent format code out of the translatable message
* string.
*/
- snprintf(sysident_str, sizeof(sysident_str), UINT64_FORMAT,
- ControlFile->system_identifier);
for (i = 0; i < MOCK_AUTH_NONCE_LEN; i++)
snprintf(_auth_nonce_str[i * 2], 3, "%02x",
(unsigned char) ControlFile->mock_authentication_nonce[i]);
@@ -229,8 +226,8 @@ main(int argc, char *argv[])
ControlFile->pg_control_version);
printf(_("Catalog version number: %u\n"),
ControlFile->catalog_version_no);
- printf(_("Database system identifier: %s\n"),
- sysident_str);
+ printf(_("Database system identifier: %" PRIu64 "\n"),
+ ControlFile->system_identifier);
printf(_("Database cluster state: %s\n"),
dbState(ControlFile->state));
printf(_("pg_control last modified: %s\n"),
diff --git a/src/include/c.h b/src/include/c.h
index 95e9aeded9..8009bcc0c5 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -364,8 +364,11 @@ typedef unsigned long long int uint64;
#endif
/* snprintf format strings to use for 64-bit integers */
-#define INT64_FORMAT "%" INT64_MODIFIER "d"
-#define UINT64_FORMAT "%" INT64_MODIFIER "u"
+#define PRId64 INT64_MODIFIER "d"
+#define PRIu64 INT64_MODIFIER "u"
+
+#define INT64_FORMAT "%" PRId64
+#define UINT64_FORMAT "%" PRIu64
/*
* 128-bit signed and unsigned integers
Re: [HACKERS] Custom compression methods
On Mon, 23 Apr 2018 19:34:38 +0300 Konstantin Knizhnik <k.knizh...@postgrespro.ru> wrote: > > > Sorry, I really looking at this patch under the different angle. > And this is why I have some doubts about general idea. > Postgres allows to defined custom types, access methods,... > But do you know any production system using some special data types > or custom indexes which are not included in standard Postgres > distribution or popular extensions (like postgis)? > > IMHO end-user do not have skills and time to create their own > compression algorithms. And without knowledge of specific of > particular data set, > it is very hard to implement something more efficient than universal > compression library. > But if you think that it is not a right place and time to discuss it, > I do not insist. > > But in any case, I think that it will be useful to provide some more > examples of custom compression API usage. > From my point of view the most useful will be integration with zstd. > But if it is possible to find some example of data-specific > compression algorithms which show better results than universal > compression, it will be even more impressive. > > Ok, let me clear up the purpose of this patch. I understand that you want to compress everything by it but now the idea is just to bring basic functionality to compress toasting values with external compression algorithms. It's unlikely that compression algorithms like zstd, snappy and others will be in postgres core but with this patch it's really easy to make an extension and start to compress values using it right away. And the end-user should not be expert in compression algorithms to make such extension. One of these algorithms could end up in core if its license will allow it. I'm not trying to cover all the places in postgres which will benefit from compression, and this patch only is the first step. It's quite big already and with every new feature that will increase its size, chances of its reviewing and commiting will decrease. The API is very simple now and contains what an every compression method can do - get some block of data and return a compressed form of the data. And it can be extended with streaming and other features in the future. Maybe the reason of your confusion is that there is no GUC that changes pglz to some custom compression so all new attributes will use it. I will think about adding it. Also there was a discussion about specifying the compression for the type and it was decided that's better to do it later by a separate patch. As an example of specialized compression could be time series compression described in [1]. [2] contains an example of an extension that adds lz4 compression using this patch. [1] http://www.vldb.org/pvldb/vol8/p1816-teller.pdf [2] https://github.com/zilder/pg_lz4 -- Regards, Ildus Kurbangaliev
Re: [HACKERS] Custom compression methods
On Sun, 22 Apr 2018 16:21:31 +0300 Alexander Korotkov <a.korot...@postgrespro.ru> wrote: > On Fri, Apr 20, 2018 at 7:45 PM, Konstantin Knizhnik < > k.knizh...@postgrespro.ru> wrote: > > > On 30.03.2018 19:50, Ildus Kurbangaliev wrote: > > > >> On Mon, 26 Mar 2018 20:38:25 +0300 > >> Ildus Kurbangaliev <i.kurbangal...@postgrespro.ru> wrote: > >> > >> Attached rebased version of the patch. Fixed conflicts in > >> pg_class.h. > >>> > >>> New rebased version due to conflicts in master. Also fixed few > >>> errors > >> and removed cmdrop method since it couldnt be tested. > >> > >> I seems to be useful (and not so difficult) to use custom > >> compression > > methods also for WAL compression: replace direct calls of > > pglz_compress in xloginsert.c > > > I'm going to object this at point, and I've following arguments for > that: > > 1) WAL compression is much more critical for durability than datatype > compression. Imagine, compression algorithm contains a bug which > cause decompress method to issue a segfault. In the case of datatype > compression, that would cause crash on access to some value which > causes segfault; but in the rest database will be working giving you > a chance to localize the issue and investigate that. In the case of > WAL compression, recovery would cause a server crash. That seems > to be much more serious disaster. You wouldn't be able to make > your database up and running and the same happens on the standby. > > 2) Idea of custom compression method is that some columns may > have specific data distribution, which could be handled better with > particular compression method and particular parameters. In the > WAL compression you're dealing with the whole WAL stream containing > all the values from database cluster. Moreover, if custom compression > method are defined for columns, then in WAL stream you've values > already compressed in the most efficient way. However, it might > appear that some compression method is better for WAL in general > case (there are benchmarks showing our pglz is not very good in > comparison to the alternatives). But in this case I would prefer to > just switch our WAL to different compression method one day. > Thankfully we don't preserve WAL compatibility between major releases. > > 3) This patch provides custom compression methods recorded in > the catalog. During recovery you don't have access to the system > catalog, because it's not recovered yet, and can't fetch compression > method metadata from there. The possible thing is to have GUC, > which stores shared module and function names for WAL compression. > But that seems like quite different mechanism from the one present > in this patch. > > Taking into account all of above, I think we would give up with custom > WAL compression method. Or, at least, consider it unrelated to this > patch. I agree with these points. I also think this should be done in another patch. It's not so hard to implement but would make sense if there will be few more builtin compression methods suitable for wal compression. Some static array could contain function pointers for direct calls. -- --- Ildus Kurbangaliev Postgres Professional: http://www.postgrespro.com Russian Postgres Company
Re: Prefix operator for text and spgist support
On Mon, 16 Apr 2018 12:45:23 +0200 Emre Hasegeli <e...@hasegeli.com> wrote: > > Thank you, pushed with some editorization and renaming > > text_startswith to starts_with > > I am sorry for not noticing this before, but what is the point of this > operator? It seems to me we are only making the prefix searching > business, which is already complicated, more complicated. Hi. > > Also, the new operator is not documented on SQL String Functions and > Operators table. It is not supported by btree text_pattern_ops or > btree indexes with COLLATE "C". It is not defined for "citext", so > people would get wrong results. It doesn't use pg_trgm indexes > whereas LIKE can. It is mentioned in documentation, look for "starts_with" function. Currently it's working with spgist indexes which fact is pointed out in the documentation too. I was going to add btree support but it would require a new strategy so it will be matter of another patch. I think this operator could be used in LIKE instead of current weird comparison operators. -- --- Ildus Kurbangaliev Postgres Professional: http://www.postgrespro.com Russian Postgres Company
Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors
On Wed, 04 Apr 2018 16:07:25 +0300 Marina Polyakova <m.polyak...@postgrespro.ru> wrote: > Hello, hackers! > > Here there's a seventh version of the patch for error handling and > retrying of transactions with serialization/deadlock failures in > pgbench (based on the commit > a08dc711952081d63577fc182fcf955958f70add). I added the option > --max-tries-time which is an implemetation of Fabien Coelho's > proposal in [1]: the transaction with serialization or deadlock > failure can be retried if the total time of all its tries is less > than this limit (in ms). This option can be combined with the option > --max-tries. But if none of them are used, failed transactions are > not retried at all. > > Also: > * Now when the first failure occurs in the transaction it is always > reported as a failure since only after the remaining commands of this > transaction are executed we find out whether we can try again or not. > Therefore add the messages about retrying or ending the failed > transaction to the "fails" debugging level so you can distinguish > failures (which are retried) and errors (which are not retried). > * Fix a report on the latency average because the total time includes > time for both errors and successful transactions. > * Code cleanup (including tests). > > [1] > https://www.postgresql.org/message-id/alpine.DEB.2.20.1803292134380.16472%40lancre > > > Maybe the max retry should rather be expressed in time rather than > > number > > of attempts, or both approach could be implemented? > Hi, I did a little review of your patch. It seems to work as expected, documentation and tests are there. Still I have few comments. There is a lot of checks like "if (debug_level >= DEBUG_FAILS)" with corresponding fprintf(stderr..) I think it's time to do it like in the main code, wrap with some function like log(level, msg). In CSTATE_RETRY state used_time is used only in printing but calculated more than needed. In my opinion Debuglevel should be renamed to DebugLevel that looks nicer, also there DEBUGLEVEl (where last letter is in lower case) which is very confusing. I have checked overall functionality of this patch, but haven't checked any special cases yet. -- --- Ildus Kurbangaliev Postgres Professional: http://www.postgrespro.com Russian Postgres Company
Re: Prefix operator for text and spgist support
On Fri, 23 Mar 2018 11:45:33 +0300
Alexander Korotkov <a.korot...@postgrespro.ru> wrote:
> On Thu, Mar 22, 2018 at 7:09 PM, Teodor Sigaev <teo...@sigaev.ru>
> wrote:
>
> > Patch looks resonable, but I see some place to improvement:
> > spg_text_leaf_consistent() only needs to check with
> > text_startswith() if reconstucted value came to leaf consistent is
> > shorter than given prefix. For example, if level >= length of
> > prefix then we guarantee that fully reconstracted is matched too.
> > But do not miss that you may need to return value for index only
> > scan, consult returnData field
> >
> > In attachment rebased and minorly edited version of your patch.
>
>
> I took a look at this patch. In addition to Teodor's comments I can
> note following.
>
> * This line looks strange for me.
>
> - if (strategy > 10)
> + if (strategy > 10 && strategy !=
> RTPrefixStrategyNumber)
>
> It's not because we added strategy != RTPrefixStrategyNumber condition
> there.
> It's because we already used magic number here and now have a mix of
> magic number and macro constant in one line. Once we anyway touch
> this place, could we get rid of magic numbers here?
>
> * I'm a little concern about operator name. We're going to choose @^
> operator for
> prefix search without any preliminary discussion. However,
> personally I don't
> have better ideas :)
Teodor, Alexander, thanks for review. In new version I have added the
optimization in spgist using level variable and also got rid of magic
numbers.
About the operator it's actually ^@ (not @^ :)), I thought about it and
don't really have any idea what operator can be used instead.
Attached version 5 of the patch.
--
---
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 9d1772f349..a1b4724a88 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -2274,6 +2274,21 @@
ph
+
+
+
+ text_startswith
+
+text_startswith(string, prefix)
+
+ bool
+
+Returns true if string starts with prefix.
+
+ text_startswith('alphabet', 'alph')
+ t
+
+
@@ -4033,6 +4048,12 @@ cast(-44 as bit(12)) 11010100
ILIKE, respectively. All of these operators are
PostgreSQL-specific.
+
+
+There is also the prefix operator ^@ and corresponding
+text_startswith function which covers cases when only
+searching by beginning of the string is needed.
+
diff --git a/doc/src/sgml/spgist.sgml b/doc/src/sgml/spgist.sgml
index e47f70be89..06b7519052 100644
--- a/doc/src/sgml/spgist.sgml
+++ b/doc/src/sgml/spgist.sgml
@@ -161,6 +161,7 @@
~~
~=~
~~
+ ^@
diff --git a/src/backend/access/spgist/spgtextproc.c b/src/backend/access/spgist/spgtextproc.c
index f156b2166e..c5e27dd7aa 100644
--- a/src/backend/access/spgist/spgtextproc.c
+++ b/src/backend/access/spgist/spgtextproc.c
@@ -67,6 +67,20 @@
*/
#define SPGIST_MAX_PREFIX_LENGTH Max((int) (BLCKSZ - 258 * 16 - 100), 32)
+/*
+ * Strategy for collation aware operator on text is equal to btree strategy
+ * plus value of 10.
+ *
+ * Current collation aware strategies and their corresponding btree strategies:
+ * 11 BTLessStrategyNumber
+ * 12 BTLessEqualStrategyNumber
+ * 14 BTGreaterEqualStrategyNumber
+ * 15 BTGreaterStrategyNumber
+ */
+#define SPG_STRATEGY_ADDITION (10)
+#define SPG_IS_COLLATION_AWARE_STRATEGY(s) ((s) > SPG_STRATEGY_ADDITION \
+ && (s) != RTPrefixStrategyNumber)
+
/* Struct for sorting values in picksplit */
typedef struct spgNodePtr
{
@@ -496,10 +510,10 @@ spg_text_inner_consistent(PG_FUNCTION_ARGS)
* well end with a partial multibyte character, so that applying
* any encoding-sensitive test to it would be risky anyhow.)
*/
- if (strategy > 10)
+ if (SPG_IS_COLLATION_AWARE_STRATEGY(strategy))
{
if (collate_is_c)
- strategy -= 10;
+ strategy -= SPG_STRATEGY_ADDITION;
else
continue;
}
@@ -526,6 +540,10 @@ spg_text_inner_consistent(PG_FUNCTION_ARGS)
if (r < 0)
res = false;
break;
+case RTPrefixStrategyNumber:
+ if (r != 0)
+ res = false;
+ break;
default:
elog(ERROR, "unrecognized strategy number: %d",
in->scankeys[j].sk_strategy);
@@ -605,10 +623,31 @@ spg_text_leaf_consistent(PG_FUNCTION_ARGS)
int queryLen = VARSIZE_ANY_EXHDR(query);
int r;
- if (strategy > 10)
+ if (strategy == RTPrefixStrategyNumber)
+ {
+ if (!in->returnData && level >= queryLen)
+ {
+/*
+ * If we got to the leaf
Re: committing inside cursor loop
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation:tested, passed I have checked new version. Although I can miss something, the patch looks good to me. The new status of this patch is: Ready for Committer
Re: [PROPOSAL] Shared Ispell dictionaries
On Mon, 19 Mar 2018 14:06:50 +0300 Arthur Zakirov <a.zaki...@postgrespro.ru> wrote: > > I beleive mmap requires completely rewrite 0003 part of the patch and > a little changes in 0005. > > > In any case, I suggest to polish the dsm-based patch, and see if we > > can get that one into PG11. > > Yes we have more time in future commitfests if dsm-based patch won't > be approved. > Hi, I'm not sure about mmap approach, it would just bring another problems. I like the dsm approach because it's not inventing any new files in the database, when mmap approach will possibly require new folder in data directory and management above bunch of new files, with additional issues related with pg_upgrade and etc. Also in dsm approach if someone needs to update dictionaries then he (or his package manager) can just replace files and be done with it. -- --- Ildus Kurbangaliev Postgres Professional: http://www.postgrespro.com Russian Postgres Company
Re: committing inside cursor loop
On Tue, 13 Mar 2018 11:08:36 -0400 Peter Eisentraut <peter.eisentr...@2ndquadrant.com> wrote: > On 3/6/18 07:48, Ildus Kurbangaliev wrote: > > Although as was discussed before it seems inconsistent without > > ROLLBACK support. There was a little discussion about it, but no > > replies. Maybe the status of the patch should be changed to > > 'Waiting on author' until the end of discussion. > > I'm wondering what the semantics of it should be. > > For example, consider this: > > drop table test1; > create table test1 (a int, b int); > insert into test1 values (1, 11), (2, 22), (3, 33); > > do > language plpgsql > $$ > declare > x int; > begin > for x in update test1 set b = b + 1 where b > 20 returning a loop > raise info 'x = %', x; > if x = 2 then >rollback; > end if; > end loop; > end; > $$; > > The ROLLBACK call in the first loop iteration undoes the UPDATE > command that drives the loop. Is it then sensible to continue the > loop? > I think that in the first place ROLLBACK was prohibited because of cases like this, but it seems to safe to continue the loop when portal strategy is PORTAL_ONE_SELECT. -- --- Ildus Kurbangaliev Postgres Professional: http://www.postgrespro.com Russian Postgres Company
Re: Prefix operator for text and spgist support
On Tue, 6 Mar 2018 19:27:21 +0300
Arthur Zakirov <a.zaki...@postgrespro.ru> wrote:
> On Mon, Feb 19, 2018 at 05:19:15PM +0300, Ildus Kurbangaliev wrote:
> > At brief look at this place seems better to move this block into
> > pattern_fixed_prefix function. But there is also `vartype` variable
> > which used to in prefix construction, and it would require pass this
> > variable too. And since pattern_fixed_prefix called in 10 other
> > places and vartype is required only for this ptype it seems better
> > just keep this block outside of this function.
>
> Understood.
>
> > I've added documentation in current version of the patch.
>
> Thank you.
>
> Can you rebase the patch due to changes within pg_proc.h?
>
> Also here
>
> +
> +There is also the prefix operator ^@ and
> corresponding
> +text_startswith function which covers cases
> when only
> +searching by beginning of the string is needed.
> +
>
> I think text_startswith should be enclosed with the tag.
> I'm not sure, but I think used for operators, keywords,
> etc. I haven't found a manual which describes how to use tags, but
> after looking at the documentation where is used, I think
> that for function should be used.
>
Hi, thanks for the review. I've fixed documentation as you said and
also rebased to current master.
--
---
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 2f59af25a6..4dc11d8df2 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -2274,6 +2274,21 @@
ph
+
+
+
+ text_startswith
+
+text_startswith(string, prefix)
+
+ bool
+
+Returns true if string starts with prefix.
+
+ text_startswith('alphabet', 'alph')
+ t
+
+
@@ -4033,6 +4048,12 @@ cast(-44 as bit(12)) 11010100
ILIKE, respectively. All of these operators are
PostgreSQL-specific.
+
+
+There is also the prefix operator ^@ and corresponding
+text_startswith function which covers cases when only
+searching by beginning of the string is needed.
+
diff --git a/doc/src/sgml/spgist.sgml b/doc/src/sgml/spgist.sgml
index e47f70be89..06b7519052 100644
--- a/doc/src/sgml/spgist.sgml
+++ b/doc/src/sgml/spgist.sgml
@@ -161,6 +161,7 @@
~~
~=~
~~
+ ^@
diff --git a/src/backend/access/spgist/spgtextproc.c b/src/backend/access/spgist/spgtextproc.c
index f156b2166e..5beb9e33a1 100644
--- a/src/backend/access/spgist/spgtextproc.c
+++ b/src/backend/access/spgist/spgtextproc.c
@@ -496,7 +496,7 @@ spg_text_inner_consistent(PG_FUNCTION_ARGS)
* well end with a partial multibyte character, so that applying
* any encoding-sensitive test to it would be risky anyhow.)
*/
- if (strategy > 10)
+ if (strategy > 10 && strategy != RTPrefixStrategyNumber)
{
if (collate_is_c)
strategy -= 10;
@@ -526,6 +526,10 @@ spg_text_inner_consistent(PG_FUNCTION_ARGS)
if (r < 0)
res = false;
break;
+case RTPrefixStrategyNumber:
+ if (r != 0)
+ res = false;
+ break;
default:
elog(ERROR, "unrecognized strategy number: %d",
in->scankeys[j].sk_strategy);
@@ -601,10 +605,21 @@ spg_text_leaf_consistent(PG_FUNCTION_ARGS)
for (j = 0; j < in->nkeys; j++)
{
StrategyNumber strategy = in->scankeys[j].sk_strategy;
- text *query = DatumGetTextPP(in->scankeys[j].sk_argument);
- int queryLen = VARSIZE_ANY_EXHDR(query);
+ text *query;
+ int queryLen;
int r;
+ /* for this strategy collation is not important */
+ if (strategy == RTPrefixStrategyNumber)
+ {
+ res = DatumGetBool(DirectFunctionCall2(text_startswith,
+out->leafValue, in->scankeys[j].sk_argument));
+ goto next;
+ }
+
+ query = DatumGetTextPP(in->scankeys[j].sk_argument);
+ queryLen = VARSIZE_ANY_EXHDR(query);
+
if (strategy > 10)
{
/* Collation-aware comparison */
@@ -655,6 +670,7 @@ spg_text_leaf_consistent(PG_FUNCTION_ARGS)
break;
}
+next:
if (!res)
break;/* no need to consider remaining conditions */
}
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index bf240aa9c5..50b2583ca0 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -1317,8 +1317,18 @@ patternsel(PG_FUNCTION_ARGS, Pattern_Type ptype, bool negate)
* right cache key, so that they can be re-used at runtime.
*/
patt = (Const *) other;
- pstatus = pattern_fixed_prefix(patt, ptype, collation,
- , _selec);
+
+ if (ptype == Pattern_Type_Prefix)
+ {
+ char *s = TextDatumGetCString(constval);
+
Re: committing inside cursor loop
On Tue, 20 Feb 2018 09:11:50 -0500 Peter Eisentraut <peter.eisentr...@2ndquadrant.com> wrote: > Here is a patch that allows COMMIT inside cursor loops in PL/pgSQL. > As alluded to in earlier threads, this is done by converting such > cursors to holdable automatically. A special flag "auto-held" marks > such cursors, so we know to clean them up on exceptions. > > This is currently only for PL/pgSQL, but the same technique could be > applied easily to other PLs. > Hi, The patch still applies, tests passed. The code looks nice, documentation and tests are there. Although as was discussed before it seems inconsistent without ROLLBACK support. There was a little discussion about it, but no replies. Maybe the status of the patch should be changed to 'Waiting on author' until the end of discussion. -- --- Ildus Kurbangaliev Postgres Professional: http://www.postgrespro.com Russian Postgres Company
Re: 2018-03 Commitfest Summary (Andres #3)
On Thu, 1 Mar 2018 20:34:11 -0800 Andres Freund <and...@anarazel.de> wrote: > > - Custom compression methods > > RFC. While marked as ready for committer, I think this is mostly > because some high level design input is needed. It's huge, and so > far only seems to add already existing compression support. > > Not seing this for v11. > Hi, This patch is not about adding new compression algorithms, it's about adding a new access method type which could be used for new compression methods. It's quite big but the large part of it are changes in regression tests (becauses it adds new field in \d+) and new tests. -- --- Ildus Kurbangaliev Postgres Professional: http://www.postgrespro.com Russian Postgres Company
Re: autovacuum: change priority of the vacuumed tables
On Thu, 1 Mar 2018 23:39:34 -0800 Andres Freund <and...@anarazel.de> wrote: > Hi, > > On 2018-02-19 17:00:34 +0100, Tomas Vondra wrote: > > I have a hard time understanding how adding yet another autovacuum > > table-level knob makes the DBA's life any easier. Especially when > > the behavior is so unreliable and does not really guarantee when the > > high-priority table will be cleaned up ... > > Based on the criticism voiced and a quick skim of the proposal, this > CF entry appears to still be in its design phase. In my opinion this > thus isn't v11 material, and should be marked as 'returned with > feedback'? Hi, I agree, this patch definitely needs more thinking. -- --- Ildus Kurbangaliev Postgres Professional: http://www.postgrespro.com Russian Postgres Company
Re: autovacuum: change priority of the vacuumed tables
On Fri, 16 Feb 2018 21:48:14 +0900 Masahiko Sawada <sawada.m...@gmail.com> wrote: > On Fri, Feb 16, 2018 at 7:50 PM, Ildus Kurbangaliev > <i.kurbangal...@postgrespro.ru> wrote: > > On Fri, 16 Feb 2018 17:42:34 +0900 > > Masahiko Sawada <sawada.m...@gmail.com> wrote: > > > >> On Thu, Feb 15, 2018 at 10:16 PM, Grigory Smolkin > >> <g.smol...@postgrespro.ru> wrote: > >> > On 02/15/2018 09:28 AM, Masahiko Sawada wrote: > >> > > >> >> Hi, > >> >> > >> >> On Thu, Feb 8, 2018 at 11:01 PM, Ildus Kurbangaliev > >> >> <i.kurbangal...@postgrespro.ru> wrote: > >> >>> > >> >>> Hi, > >> >>> > >> >>> Attached patch adds 'autovacuum_table_priority' to the current > >> >>> list of automatic vacuuming settings. It's used in sorting of > >> >>> vacuumed tables in autovacuum worker before actual vacuum. > >> >>> > >> >>> The idea is to give possibility to the users to prioritize > >> >>> their tables in autovacuum process. > >> >>> > >> >> Hmm, I couldn't understand the benefit of this patch. Would you > >> >> elaborate it a little more? > >> >> > >> >> Multiple autovacuum worker can work on one database. So even if > >> >> a table that you want to vacuum first is the back of the list > >> >> and there other worker would pick up it. If the vacuuming the > >> >> table gets delayed due to some big tables are in front of that > >> >> table I think you can deal with it by increasing the number of > >> >> autovacuum workers. > >> >> > >> >> Regards, > >> >> > >> >> -- > >> >> Masahiko Sawada > >> >> NIPPON TELEGRAPH AND TELEPHONE CORPORATION > >> >> NTT Open Source Software Center > >> >> > >> > > >> > Database can contain thousands of tables and often > >> > updates/deletes concentrate mostly in only a handful of tables. > >> > Going through thousands of less bloated tables can take ages. > >> > Currently autovacuum know nothing about prioritizing it`s work > >> > with respect to user`s understanding of his data and > >> > application. > >> > >> Understood. I have a question; please imagine the following case. > >> > >> Suppose that there are 1000 tables in a database, and one table of > >> them (table-A) has the highest priority while other 999 tables have > >> same priority. Almost tables (say 800 tables) including table-A > >> need to get vacuumed at some point, so with your patch an AV > >> worker listed 800 tables and table-A will be at the head of the > >> list. Table-A will get vacuumed first but this AV worker has to > >> vacuum other 799 tables even if table-A requires vacuum later > >> again. > >> > >> If an another AV worker launches during table-A being vacuumed, the > >> new AV worker would include table-A but would not process it > >> because concurrent AV worker is processing it. So it would vacuum > >> other tables instead. Similarly, this AV worker can not get the > >> new table list until finish to vacuum all other tables. (Note that > >> it might skip some tables if they are already vacuumed by other AV > >> worker.) On the other hand, if another new AV worker launches > >> after table-A got vacuumed and requires vacuuming again, the new > >> AV worker puts the table-A at the head of list. It processes > >> table-A first but, again, it has to vacuum other tables before > >> getting new table list next time that might include table-A. > >> > >> Is this the expected behavior? I'd rather expect postgres to > >> vacuum it before other lower priority tables whenever the table > >> having the highest priority requires vacuuming, but it wouldn't. > > > > Yes, this is the expected behavior. The patch is the way to give the > > user at least some control of the sorting, later it could be > > extended with something more sophisticated. > > > > Since user doesn't know that each AV worker processes tables based on > its table list that is different from lists that other worker has, I > think it's hard for user to understand this parameter. I'd say that > user would expect that high priority table can get vacuumed any time. Yes, very good point. It coul
Re: autovacuum: change priority of the vacuumed tables
On Fri, 16 Feb 2018 17:42:34 +0900 Masahiko Sawada <sawada.m...@gmail.com> wrote: > On Thu, Feb 15, 2018 at 10:16 PM, Grigory Smolkin > <g.smol...@postgrespro.ru> wrote: > > On 02/15/2018 09:28 AM, Masahiko Sawada wrote: > > > >> Hi, > >> > >> On Thu, Feb 8, 2018 at 11:01 PM, Ildus Kurbangaliev > >> <i.kurbangal...@postgrespro.ru> wrote: > >>> > >>> Hi, > >>> > >>> Attached patch adds 'autovacuum_table_priority' to the current > >>> list of automatic vacuuming settings. It's used in sorting of > >>> vacuumed tables in autovacuum worker before actual vacuum. > >>> > >>> The idea is to give possibility to the users to prioritize their > >>> tables in autovacuum process. > >>> > >> Hmm, I couldn't understand the benefit of this patch. Would you > >> elaborate it a little more? > >> > >> Multiple autovacuum worker can work on one database. So even if a > >> table that you want to vacuum first is the back of the list and > >> there other worker would pick up it. If the vacuuming the table > >> gets delayed due to some big tables are in front of that table I > >> think you can deal with it by increasing the number of autovacuum > >> workers. > >> > >> Regards, > >> > >> -- > >> Masahiko Sawada > >> NIPPON TELEGRAPH AND TELEPHONE CORPORATION > >> NTT Open Source Software Center > >> > > > > Database can contain thousands of tables and often updates/deletes > > concentrate mostly in only a handful of tables. > > Going through thousands of less bloated tables can take ages. > > Currently autovacuum know nothing about prioritizing it`s work with > > respect to user`s understanding of his data and application. > > Understood. I have a question; please imagine the following case. > > Suppose that there are 1000 tables in a database, and one table of > them (table-A) has the highest priority while other 999 tables have > same priority. Almost tables (say 800 tables) including table-A need > to get vacuumed at some point, so with your patch an AV worker listed > 800 tables and table-A will be at the head of the list. Table-A will > get vacuumed first but this AV worker has to vacuum other 799 tables > even if table-A requires vacuum later again. > > If an another AV worker launches during table-A being vacuumed, the > new AV worker would include table-A but would not process it because > concurrent AV worker is processing it. So it would vacuum other tables > instead. Similarly, this AV worker can not get the new table list > until finish to vacuum all other tables. (Note that it might skip some > tables if they are already vacuumed by other AV worker.) On the other > hand, if another new AV worker launches after table-A got vacuumed and > requires vacuuming again, the new AV worker puts the table-A at the > head of list. It processes table-A first but, again, it has to vacuum > other tables before getting new table list next time that might > include table-A. > > Is this the expected behavior? I'd rather expect postgres to vacuum it > before other lower priority tables whenever the table having the > highest priority requires vacuuming, but it wouldn't. Yes, this is the expected behavior. The patch is the way to give the user at least some control of the sorting, later it could be extended with something more sophisticated. -- --- Ildus Kurbangaliev Postgres Professional: http://www.postgrespro.com Russian Postgres Company
autovacuum: change priority of the vacuumed tables
Hi,
Attached patch adds 'autovacuum_table_priority' to the current list of
automatic vacuuming settings. It's used in sorting of vacuumed tables in
autovacuum worker before actual vacuum.
The idea is to give possibility to the users to prioritize their tables
in autovacuum process.
--
---
Regards,
Ildus Kurbangaliev
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index c45979dee4..b7383a7136 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -6250,6 +6250,21 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
+
+ autovacuum_table_priority (integer)
+
+ autovacuum_table_priority configuration parameter
+
+
+
+
+Specifies the priority of the table in automatic
+VACUUM operations. 0 by default, bigger value gives
+more priority.
+
+
+
+
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 46276ceff1..2476dfe943 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -291,6 +291,15 @@ static relopt_int intRelOpts[] =
},
-1, -1, INT_MAX
},
+ {
+ {
+ "autovacuum_table_priority",
+ "Sets the priority of the table in autovacuum process",
+ RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+ ShareUpdateExclusiveLock
+ },
+ 0, INT_MIN, INT_MAX
+ },
{
{
"toast_tuple_target",
@@ -1353,6 +1362,8 @@ default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, multixact_freeze_table_age)},
{"log_autovacuum_min_duration", RELOPT_TYPE_INT,
offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, log_min_duration)},
+ {"autovacuum_table_priority", RELOPT_TYPE_INT,
+ offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, priority)},
{"toast_tuple_target", RELOPT_TYPE_INT,
offsetof(StdRdOptions, toast_tuple_target)},
{"autovacuum_vacuum_scale_factor", RELOPT_TYPE_REAL,
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 702f8d8188..174de4a08c 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -174,15 +174,22 @@ typedef struct avw_dbase
PgStat_StatDBEntry *adw_entry;
} avw_dbase;
-/* struct to keep track of tables to vacuum and/or analyze, in 1st pass */
+/* struct for vacuumed or analyzed relation */
typedef struct av_relation
+{
+ Oid ar_relid;
+ int ar_priority; /* bigger- more important, used for sorting */
+} av_relation;
+
+/* struct to keep track of toast tables to vacuum and/or analyze, in 1st pass */
+typedef struct av_toastrelation
{
Oid ar_toastrelid; /* hash key - must be first */
Oid ar_relid;
bool ar_hasrelopts;
AutoVacOpts ar_reloptions; /* copy of AutoVacOpts from the main table's
* reloptions, or NULL if none */
-} av_relation;
+} av_toastrelation;
/* struct to keep track of tables to vacuum and/or analyze, after rechecking */
typedef struct autovac_table
@@ -1919,6 +1926,16 @@ get_database_list(void)
return dblist;
}
+/* qsort comparator for av_relation, using priority */
+static int
+autovac_comparator(const void *a, const void *b)
+{
+ av_relation *r1 = (av_relation *) lfirst(*(ListCell **) a);
+ av_relation *r2 = (av_relation *) lfirst(*(ListCell **) b);
+
+ return r2->ar_priority - r1->ar_priority;
+}
+
/*
* Process a database table-by-table
*
@@ -1932,7 +1949,7 @@ do_autovacuum(void)
HeapTuple tuple;
HeapScanDesc relScan;
Form_pg_database dbForm;
- List *table_oids = NIL;
+ List *optables = NIL;
List *orphan_oids = NIL;
HASHCTL ctl;
HTAB *table_toast_map;
@@ -2021,7 +2038,7 @@ do_autovacuum(void)
/* create hash table for toast <-> main relid mapping */
MemSet(, 0, sizeof(ctl));
ctl.keysize = sizeof(Oid);
- ctl.entrysize = sizeof(av_relation);
+ ctl.entrysize = sizeof(av_toastrelation);
table_toast_map = hash_create("TOAST to main relid map",
100,
@@ -2101,9 +2118,14 @@ do_autovacuum(void)
effective_multixact_freeze_max_age,
, , );
- /* Relations that need work are added to table_oids */
+ /* Relations that need work are added to optables */
if (dovacuum || doanalyze)
- table_oids = lappend_oid(table_oids, relid);
+ {
+ av_relation *rel = (av_relation *) palloc(sizeof(av_relation));
+ rel->ar_relid = relid;
+ rel->ar_priority = relopts != NULL ? relopts->priority : 0;
+ optables = lappend(optables, rel);
+ }
/*
* Remember TOAST associations for the second pass. Note: we must do
@@ -2112,7 +2134,7 @@ do_autovacuum(void)
*/
if (OidIsValid(classForm->reltoastrelid))
{
- av_relation *hentry;
+ av_toastrelation *hentry;
bool found;
hentry = hash_search(table_toast_map,
@@ -2168,7 +2190,
Prefix operator for text and spgist support
Hi,
Attached patch introduces prefix operator ^@ for text type. For 'a ^@ b'
it returns true if 'a' starts with 'b'. Also there is spgist index
support for this operator.
It could be useful as an alternative for LIKE for 'something%'
templates. Or even used in LIKE queries instead of ~>=~ and ~<~ in the
future. But it would require new strategy for btree.
--
---
Ildus Kurbangaliev
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
diff --git a/src/backend/access/spgist/spgtextproc.c b/src/backend/access/spgist/spgtextproc.c
index f156b2166e..5beb9e33a1 100644
--- a/src/backend/access/spgist/spgtextproc.c
+++ b/src/backend/access/spgist/spgtextproc.c
@@ -496,7 +496,7 @@ spg_text_inner_consistent(PG_FUNCTION_ARGS)
* well end with a partial multibyte character, so that applying
* any encoding-sensitive test to it would be risky anyhow.)
*/
- if (strategy > 10)
+ if (strategy > 10 && strategy != RTPrefixStrategyNumber)
{
if (collate_is_c)
strategy -= 10;
@@ -526,6 +526,10 @@ spg_text_inner_consistent(PG_FUNCTION_ARGS)
if (r < 0)
res = false;
break;
+case RTPrefixStrategyNumber:
+ if (r != 0)
+ res = false;
+ break;
default:
elog(ERROR, "unrecognized strategy number: %d",
in->scankeys[j].sk_strategy);
@@ -601,10 +605,21 @@ spg_text_leaf_consistent(PG_FUNCTION_ARGS)
for (j = 0; j < in->nkeys; j++)
{
StrategyNumber strategy = in->scankeys[j].sk_strategy;
- text *query = DatumGetTextPP(in->scankeys[j].sk_argument);
- int queryLen = VARSIZE_ANY_EXHDR(query);
+ text *query;
+ int queryLen;
int r;
+ /* for this strategy collation is not important */
+ if (strategy == RTPrefixStrategyNumber)
+ {
+ res = DatumGetBool(DirectFunctionCall2(text_startswith,
+out->leafValue, in->scankeys[j].sk_argument));
+ goto next;
+ }
+
+ query = DatumGetTextPP(in->scankeys[j].sk_argument);
+ queryLen = VARSIZE_ANY_EXHDR(query);
+
if (strategy > 10)
{
/* Collation-aware comparison */
@@ -655,6 +670,7 @@ spg_text_leaf_consistent(PG_FUNCTION_ARGS)
break;
}
+next:
if (!res)
break;/* no need to consider remaining conditions */
}
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index fcc8323f62..be91082b56 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -1315,8 +1315,18 @@ patternsel(PG_FUNCTION_ARGS, Pattern_Type ptype, bool negate)
* right cache key, so that they can be re-used at runtime.
*/
patt = (Const *) other;
- pstatus = pattern_fixed_prefix(patt, ptype, collation,
- , _selec);
+
+ if (ptype == Pattern_Type_Prefix)
+ {
+ char *s = TextDatumGetCString(constval);
+ prefix = string_to_const(s, vartype);
+ pstatus = Pattern_Prefix_Partial;
+ rest_selec = 1.0; /* all */
+ pfree(s);
+ }
+ else
+ pstatus = pattern_fixed_prefix(patt, ptype, collation,
+ , _selec);
/*
* If necessary, coerce the prefix constant to the right type.
@@ -1486,6 +1496,16 @@ likesel(PG_FUNCTION_ARGS)
}
/*
+ * prefixsel - selectivity of prefix operator
+ */
+Datum
+prefixsel(PG_FUNCTION_ARGS)
+{
+ PG_RETURN_FLOAT8(patternsel(fcinfo, Pattern_Type_Prefix, false));
+}
+
+/*
+ *
* iclikesel - Selectivity of ILIKE pattern match.
*/
Datum
@@ -2904,6 +2924,15 @@ likejoinsel(PG_FUNCTION_ARGS)
PG_RETURN_FLOAT8(patternjoinsel(fcinfo, Pattern_Type_Like, false));
}
+/*
+ * prefixjoinsel - Join selectivity of prefix operator
+ */
+Datum
+prefixjoinsel(PG_FUNCTION_ARGS)
+{
+ PG_RETURN_FLOAT8(patternjoinsel(fcinfo, Pattern_Type_Prefix, false));
+}
+
/*
* iclikejoinsel - Join selectivity of ILIKE pattern match.
*/
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 304cb26691..050875e0bb 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -1762,6 +1762,34 @@ text_ge(PG_FUNCTION_ARGS)
PG_RETURN_BOOL(result);
}
+Datum
+text_startswith(PG_FUNCTION_ARGS)
+{
+ Datum arg1 = PG_GETARG_DATUM(0);
+ Datum arg2 = PG_GETARG_DATUM(1);
+ bool result;
+ Size len1,
+len2;
+
+ len1 = toast_raw_datum_size(arg1);
+ len2 = toast_raw_datum_size(arg2);
+ if (len2 > len1)
+ result = false;
+ else
+ {
+ text *targ1 = DatumGetTextPP(arg1);
+ text *targ2 = DatumGetTextPP(arg2);
+
+ result = (memcmp(VARDATA_ANY(targ1), VARDATA_ANY(targ2),
+ len2 - VARHDRSZ) == 0);
+
+ PG_FREE_IF_COPY(targ1, 0);
+ PG_FREE_IF_COPY(targ2, 1);
+ }
+
+ PG_RETURN_BOOL(result);
+}
+
Datum
bttextcmp(PG_FUNCTION_ARGS)
{
diff --git a/src/include/access/stratnum.h b/src/include/access/stratnum.h
index bddfac4c10..0db11a1117 100644
--- a/src/include/access/stratnum.h
+++ b/src/include/access/stratnum.h
@@ -68,8 +68,9 @@ typedef uint16 StrategyNumber;
#define RTSubEqualStrategyNumber 25 /* for inet <<= */
#define RTSuperStrategy
Re: [HACKERS] Custom compression methods
On Mon, 29 Jan 2018 17:29:29 +0300 Ildar Musin <i.mu...@postgrespro.ru> wrote: > > Patch applies cleanly, builds without any warnings, documentation > builds ok, all tests pass. > > A remark for the committers. The patch is quite big, so I really wish > more reviewers looked into it for more comprehensive review. Also a > native english speaker should check the documentation and comments. > Another thing is that tests don't cover cmdrop method because the > built-in pglz compression doesn't use it (I know there is an jsonbd > extension [1] based on this patch and which should benefit from > cmdrop method, but it doesn't test it either yet). > > I think I did what I could and so passing this patch to committers > for the review. Changed status to "Ready for committer". > > > [1] https://github.com/postgrespro/jsonbd > Thank you! About cmdrop, I checked that's is called manually, but going to check it thoroughly in my extension. -- --- Ildus Kurbangaliev Postgres Professional: http://www.postgrespro.com Russian Postgres Company
Re: [PROPOSAL] Shared Ispell dictionaries
On Wed, 24 Jan 2018 20:20:41 +0300 Arthur Zakirov <a.zaki...@postgrespro.ru> wrote: Hi, I did some review of the patch. In 0001 there are few lines where is only indentation has changed. 0002: - TsearchShmemSize - calculating size using hash_estimate_size seems redundant since you use DSA hash now. - ts_dict_shmem_release - LWLockAcquire in the beginning makes no sense, since dict_table couldn't change anyway. 0003: - ts_dict_shmem_location could return IspellDictData, it makes more sense. 0006: It's very subjective, but I think it would nicer to call option as Shared (as property of dictionary) or UseSharedMemory, the boolean option called SharedMemory sounds weird. Overall the patches look good, all tests passed. I tried to broke it in few places where I thought it could be unsafe but not succeeded. -- --- Ildus Kurbangaliev Postgres Professional: http://www.postgrespro.com Russian Postgres Company
Re: [HACKERS] Custom compression methods
On Thu, 14 Dec 2017 10:29:10 -0500 Robert Haas <robertmh...@gmail.com> wrote: > On Wed, Dec 13, 2017 at 7:18 AM, Ildus Kurbangaliev > <i.kurbangal...@postgrespro.ru> wrote: > > Since we agreed on ALTER syntax, i want to clear things about > > CREATE. Should it be CREATE ACCESS METHOD .. TYPE СOMPRESSION or > > CREATE COMPRESSION METHOD? I like the access method approach, and it > > simplifies the code, but I'm just not sure a compression is an > > access method or not. > > +1 for ACCESS METHOD. An access method then. > > > Current implementation > > -- > > > > To avoid extra patches I also want to clear things about current > > implementation. Right now there are two tables, "pg_compression" and > > "pg_compression_opt". When compression method is linked to a column > > it creates a record in pg_compression_opt. This record's Oid is > > stored in the varlena. These Oids kept in first column so I can > > move them in pg_upgrade but in all other aspects they behave like > > usual Oids. Also it's easy to restore them. > > pg_compression_opt -> pg_attr_compression, maybe. > > > Compression options linked to a specific column. When tuple is > > moved between relations it will be decompressed. > > Can we do this only if the compression method isn't OK for the new > column? For example, if the old column is COMPRESS foo PRESERVE bar > and the new column is COMPRESS bar PRESERVE foo, we don't need to > force decompression in any case. Thanks, sounds right, i will add it to the patch. -- --- Ildus Kurbangaliev Postgres Professional: http://www.postgrespro.com Russian Postgres Company
Re: [HACKERS] Custom compression methods
On Tue, 12 Dec 2017 15:52:01 -0500 Robert Haas <robertmh...@gmail.com> wrote: > > Yes. I wonder if \d or \d+ can show it somehow. > Yes, in current version of the patch, \d+ shows current compression. It can be extended to show a list of current compression methods. Since we agreed on ALTER syntax, i want to clear things about CREATE. Should it be CREATE ACCESS METHOD .. TYPE СOMPRESSION or CREATE COMPRESSION METHOD? I like the access method approach, and it simplifies the code, but I'm just not sure a compression is an access method or not. Current implementation -- To avoid extra patches I also want to clear things about current implementation. Right now there are two tables, "pg_compression" and "pg_compression_opt". When compression method is linked to a column it creates a record in pg_compression_opt. This record's Oid is stored in the varlena. These Oids kept in first column so I can move them in pg_upgrade but in all other aspects they behave like usual Oids. Also it's easy to restore them. Compression options linked to a specific column. When tuple is moved between relations it will be decompressed. Also in current implementation SET COMPRESSION contains WITH syntax which is used to provide extra options to compression method. What could be changed - As Alvaro mentioned COMPRESSION METHOD is practically an access method, so it could be created as CREATE ACCESS METHOD .. TYPE COMPRESSION. This approach simplifies the patch and "pg_compression" table could be removed. So compression method is created with something like: CREATE ACCESS METHOD .. TYPE COMPRESSION HANDLER awesome_compression_handler; Syntax of SET COMPRESSION changes to SET COMPRESSION .. PRESERVE which is useful to control rewrites and for pg_upgrade to make dependencies between moved compression options and compression methods from pg_am table. Default compression is always pglz and if users want to change they run: ALTER COLUMN SET COMPRESSION awesome PRESERVE pglz; Without PRESERVE it will rewrite the whole relation using new compression. Also the rewrite removes all unlisted compression options so their compresssion methods could be safely dropped. "pg_compression_opt" table could be renamed to "pg_compression", and compression options will be stored there. I'd like to keep extra compression options, for example pglz can be configured with them. Syntax would be slightly changed: SET COMPRESSION pglz WITH (min_comp_rate=25) PRESERVE awesome; Setting the same compression method with different options will create new compression options record for future tuples but will not rewrite table. -- Regards, Ildus Kurbangaliev
Re: [HACKERS] Custom compression methods
On Mon, 11 Dec 2017 20:53:29 +0100 Tomas Vondra <tomas.von...@2ndquadrant.com> wrote: > But let me play the devil's advocate for a while and question the > usefulness of this approach to compression. Some of the questions were > mentioned in the thread before, but I don't think they got the > attention they deserve. Hi. I will try to explain why this approach could be better than others. > > > Replacing the algorithm used to compress all varlena values (in a way > that makes it transparent for the data type code). > -- > > While pglz served us well over time, it was repeatedly mentioned that > in some cases it becomes the bottleneck. So supporting other state of > the art compression algorithms seems like a good idea, and this patch > is one way to do that. > > But perhaps we should simply make it an initdb option (in which case > the whole cluster would simply use e.g. lz4 instead of pglz)? > > That seems like a much simpler approach - it would only require some > ./configure options to add --with-lz4 (and other compression > libraries), an initdb option to pick compression algorithm, and > probably noting the choice in cluster controldata. Replacing pglz for all varlena values wasn't the goal of the patch, but it's possible to do with it and I think that's good. And as Robert mentioned pglz could appear as builtin undroppable compresssion method so the others could be added too. And in the future it can open the ways to specify compression for specific database or cluster. > > Custom datatype-aware compression (e.g. the tsvector) > -- > > Exploiting knowledge of the internal data type structure is a > promising way to improve compression ratio and/or performance. > > The obvious question of course is why shouldn't this be done by the > data type code directly, which would also allow additional benefits > like operating directly on the compressed values. > > Another thing is that if the datatype representation changes in some > way, the compression method has to change too. So it's tightly coupled > to the datatype anyway. > > This does not really require any new infrastructure, all the pieces > are already there. > > In some cases that may not be quite possible - the datatype may not be > flexible enough to support alternative (compressed) representation, > e.g. because there are no bits available for "compressed" flag, etc. > > Conclusion: IMHO if we want to exploit the knowledge of the data type > internal structure, perhaps doing that in the datatype code directly > would be a better choice. It could be, but let's imagine there will be internal compression for tsvector. It means that tsvector has two formats now and minus one bit somewhere in the header. After a while we found a better compression but we can't add it because there is already one and it's not good to have three different formats for one type. Or, the compression methods were implemented and we decided to use dictionaries for tsvector (if the user going to store limited number of words). But it will mean that tsvector will go two compression stages (for its internal and for dictionaries). > > > Custom datatype-aware compression with additional column-specific > metadata (e.g. the jsonb with external dictionary). > -- > > Exploiting redundancy in multiple values in the same column (instead > of compressing them independently) is another attractive way to help > the compression. It is inherently datatype-aware, but currently can't > be implemented directly in datatype code as there's no concept of > column-specific storage (e.g. to store dictionary shared by all values > in a particular column). > > I believe any patch addressing this use case would have to introduce > such column-specific storage, and any solution doing that would > probably need to introduce the same catalogs, etc. > > The obvious disadvantage of course is that we need to decompress the > varlena value before doing pretty much anything with it, because the > datatype is not aware of the compression. > > So I wonder if the patch should instead provide infrastructure for > doing that in the datatype code directly. > > The other question is if the patch should introduce some > infrastructure for handling the column context (e.g. column > dictionary). Right now, whoever implements the compression has to > implement this bit too. Column specific storage sounds optional to me. For example compressing timestamp[] using some delta compression will not require it. -- Regards, Ildus Kurbangaliev
Re: [HACKERS] Custom compression methods
On Fri, 8 Dec 2017 15:12:42 -0500 Robert Haas <robertmh...@gmail.com> wrote: > > Maybe a better idea is ALTER COLUMN SET COMPRESSION x1, x2, x3 ... > meaning that x1 is the default for new tuples but x2, x3, etc. are > still allowed if present. If you issue a command that only adds > things to the list, no table rewrite happens, but if you remove > anything, then it does. > I like this idea, but maybe it should be something like ALTER COLUMN SET COMPRESSION x1 [ PRESERVE x2, x3 ]? 'PRESERVE' is already used in syntax and this syntax will show better which one is current and which ones should be kept. -- Regards, Ildus Kurbangaliev
Re: [HACKERS] Custom compression methods
On Fri, 1 Dec 2017 21:47:43 +0100 Tomas Vondra <tomas.von...@2ndquadrant.com> wrote: > > +1 to do the rewrite, just like for other similar ALTER TABLE commands Ok. What about the following syntax: ALTER COLUMN DROP COMPRESSION - removes compression from the column with the rewrite and removes related compression options, so the user can drop compression method. ALTER COLUMN SET COMPRESSION NONE for the cases when the users want to just disable compression for future tuples. After that they can keep compressed tuples, or in the case when they have a large table they can decompress tuples partially using e.g. UPDATE, and then use ALTER COLUMN DROP COMPRESSION which will be much faster then. ALTER COLUMN SET COMPRESSION WITH will change compression for new tuples but will not touch old ones. If the users want the recompression they can use DROP/SET COMPRESSION combination. I don't think that SET COMPRESSION with the rewrite of the whole table will be useful enough on any somewhat big tables and same time big tables is where the user needs compression the most. I understand that ALTER with the rewrite sounds logical and much easier to implement (and it doesn't require Oids in tuples), but it could be unusable. -- ---- Regards, Ildus Kurbangaliev
Re: [HACKERS] Custom compression methods
On Fri, 1 Dec 2017 16:38:42 -0300 Alvaro Herrera <alvhe...@alvh.no-ip.org> wrote: > > To me it makes sense to say "let's create this method which is for > data compression" (CREATE ACCESS METHOD hyperz TYPE COMPRESSION) > followed by either "let's use this new compression method for the > type tsvector" (ALTER TYPE tsvector SET COMPRESSION hyperz) or "let's > use this new compression method for the column tc" (ALTER TABLE ALTER > COLUMN tc SET COMPRESSION hyperz). > Hi, I think if CREATE ACCESS METHOD can be used for compression, then it could be nicer than CREATE COMPRESSION METHOD. I just don't know that compression could go as access method or not. Anyway it's easy to change syntax and I don't mind to do it, if it will be neccessary for the patch to be commited. -- Regards, Ildus Kurbangaliev
Re: [HACKERS] Custom compression methods
On Thu, 23 Nov 2017 21:54:32 +0100 Tomas Vondra <tomas.von...@2ndquadrant.com> wrote: > > Hmm, this seems to have fixed it, but only in one direction. Consider > this: > > create table t_pglz (v text); > create table t_lz4 (v text compressed lz4); > > insert into t_pglz select repeat(md5(i::text),300) > from generate_series(1,10) s(i); > > insert into t_lz4 select repeat(md5(i::text),300) > from generate_series(1,10) s(i); > > \d+ > > Schema | Name | Type | Owner | Size | Description > ++---+---+---+- > public | t_lz4 | table | user | 12 MB | > public | t_pglz | table | user | 18 MB | > (2 rows) > > truncate t_pglz; > insert into t_pglz select * from t_lz4; > > \d+ > > Schema | Name | Type | Owner | Size | Description > ++---+---+---+- > public | t_lz4 | table | user | 12 MB | > public | t_pglz | table | user | 18 MB | > (2 rows) > > which is fine. But in the other direction, this happens > > truncate t_lz4; > insert into t_lz4 select * from t_pglz; > > \d+ >List of relations > Schema | Name | Type | Owner | Size | Description > ++---+---+---+- > public | t_lz4 | table | user | 18 MB | > public | t_pglz | table | user | 18 MB | > (2 rows) > > which means the data is still pglz-compressed. That's rather strange, > I guess, and it should compress the data using the compression method > set for the target table instead. That's actually an interesting issue. It happens because if tuple fits to page then postgres just moves it as is. I've just added recompression if it has custom compressed datums to keep dependencies right. But look: create table t1(a text); create table t2(a text); alter table t2 alter column a set storage external; insert into t1 select repeat(md5(i::text),300) from generate_series(1,10) s(i); \d+ List of relations Schema | Name | Type | Owner |Size| Description +--+---+---++- public | t1 | table | ildus | 18 MB | public | t2 | table | ildus | 8192 bytes | (2 rows) insert into t2 select * from t1; \d+ List of relations Schema | Name | Type | Owner | Size | Description +--+---+---+---+- public | t1 | table | ildus | 18 MB | public | t2 | table | ildus | 18 MB | (2 rows) That means compressed datums now in the column with storage specified as external. I'm not sure that's a bug or a feature. Lets insert them usual way: delete from t2; insert into t2 select repeat(md5(i::text),300) from generate_series(1,10) s(i); \d+ List of relations Schema | Name | Type | Owner | Size | Description +--+---+---+-+- public | t1 | table | ildus | 18 MB | public | t2 | table | ildus | 1011 MB | Maybe there should be more common solution like comparison of attribute properties? -- --- Ildus Kurbangaliev Postgres Professional: http://www.postgrespro.com Russian Postgres Company
Re: [HACKERS] Custom compression methods
On Mon, 20 Nov 2017 16:29:11 +0100 Tomas Vondra <tomas.von...@2ndquadrant.com> wrote: > On 11/20/2017 04:21 PM, Евгений Шишкин wrote: > > > > > >> On Nov 20, 2017, at 18:18, Tomas Vondra > >> <tomas.von...@2ndquadrant.com > >> <mailto:tomas.von...@2ndquadrant.com>> wrote: > >> > >> > >> I don't think we need to do anything smart here - it should behave > >> just like dropping a data type, for example. That is, error out if > >> there are columns using the compression method (without CASCADE), > >> and drop all the columns (with CASCADE). > > > > What about instead of dropping column we leave data uncompressed? > > > > That requires you to go through the data and rewrite the whole table. > And I'm not aware of a DROP command doing that, instead they just drop > the dependent objects (e.g. DROP TYPE, ...). So per PLOS the DROP > COMPRESSION METHOD command should do that too. > > But I'm wondering if ALTER COLUMN ... SET NOT COMPRESSED should do > that (currently it only disables compression for new data). If the table is big, decompression could take an eternity. That's why i decided to only to disable it and the data could be decompressed using compression options. My idea was to keep compression options forever, since there will not be much of them in one database. Still that requires that extension is not removed. I will try to find a way how to recompress data first in case it moves to another table. -- --- Ildus Kurbangaliev Postgres Professional: http://www.postgrespro.com Russian Postgres Company
Re: Repetitive code in RI triggers
On Fri, 17 Nov 2017 15:05:31 + Ildus Kurbangaliev <i.kurbangal...@gmail.com> wrote: > The following review has been posted through the commitfest > application: make installcheck-world: tested, failed > Implements feature: tested, failed > Spec compliant: tested, failed > Documentation:tested, failed > > The patch looks good. It just removes repetitive code and I think > it's ready to commit. > > The new status of this patch is: Ready for Committer "tested, failed" should be read as "tested, passed". Forgot to check them. -- --- Ildus Kurbangaliev Postgres Professional: http://www.postgrespro.com Russian Postgres Company
Re: Repetitive code in RI triggers
The following review has been posted through the commitfest application: make installcheck-world: tested, failed Implements feature: tested, failed Spec compliant: tested, failed Documentation:tested, failed The patch looks good. It just removes repetitive code and I think it's ready to commit. The new status of this patch is: Ready for Committer
