Re: [PATCH] minor reloption regression tests improvement
В письме от понедельник, 7 марта 2022 г. 20:04:49 MSK пользователь Greg Stark написал: > I don't think this is worth spending time adding tests for. I get what > you're saying that this is at least semi-intentional behaviour and > desirable behaviour so it should have tests ensuring that it continues > to work. But it's not documented behaviour and the test is basically > testing that the implementation is this specific implementation. > > I don't think the test is really a bad idea but neither is it really > very useful and I think it's not worth having people spend time > reviewing and discussing. I'm leaning to saying this patch is > rejected. This is a regression test. To test if behaviour brocken or not. Actually I came to the idea of the test when I wrote my big reloption patch. I've broken this behaviour by mistake and did not notice it as it have not been properly tested. So I decided it would be goo to add test to it before adding and big patches. -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su
Re: [PATCH][PROPOSAL] Refuse setting toast.* reloptions when TOAST table does not exist
В письме от 10 апреля 2018 08:55:52 пользователь David Steele написал: > On 1/25/18 12:27 PM, Nikolay Shaplov wrote: > > В письме от 25 января 2018 11:29:34 пользователь Tom Lane написал: > >> Alvaro Herrera writes: > >>> Tom Lane wrote: > >>>> Well, maybe the right answer is to address that. It's clear to me > >>>> why that would happen if we store these things as reloptions on the > >>>> toast table, but can't they be stored on the parent table? > >>> > >>> Actually, Nikolay provided a possible solution: if you execute ALTER > >>> TABLE SET (toast.foobar = xyz), and a toast table doesn't exist, create > >>> one at that point. > >> > >> That adds a lot of overhead if you never actually need the toast table. > > > > I think this overhead case is not that terrible if it is properly warned > > ;-)> > >> Still, maybe it's an appropriate amount of effort compared to the size > >> of the use-case for this. > > > > If you came to some final conclustion, please close the commiffest item > > with "Return with feedback" resolution, and I write another patch... > > I think this patch should be marked Returned with Feedback since it > appears there is no consensus on whether it is useful or correct, so I > have done that. Exactly! But I'd like to know what kind of feedback is it :-) What conclusion have been reached (I did not got it) Otherwise I would not know how to rewrite this patch. I would suggest: create a TOAST relation whenever toast.* options is set, but give a warning it this relation will not be used for data storage (i.e. there is no toastable columns in the table) But I need some confirmation, in order not to write patch in vain again :-) > > If I got it wrong I'm happy to move it to the next CF in Waiting for > Author state instead. > > Thanks, -- Do code for fun. signature.asc Description: This is a digitally signed message part.
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от четверг, 5 сентября 2019 г. 11:42:27 MSK пользователь Alvaro
Herrera from 2ndQuadrant написал:
> After looking closer once again, I don't like this patch.
>
> I think the FOOBAR_ENUM_DEF defines serve no purpose, other than
> source-code placement next to the enum value definitions. I think for
> example check_option, living in reloptions.c, should look like this:
This sounds as a good idea, I tried it, but did not succeed.
When I do as you suggest I get a bunch of warnings, and more over it, tests do
not pass afterwards.
reloptions.c:447:3: warning: braces around scalar initializer
{
^
reloptions.c:447:3: warning: (near initialization for
‘enumRelOpts[0].members’)
reloptions.c:448:4: warning: braces around scalar initializer
{ "on", GIST_OPTION_BUFFERING_ON },
^
reloptions.c:448:4: warning: (near initialization for
‘enumRelOpts[0].members’)
reloptions.c:448:4: warning: initialization from incompatible pointer type
reloptions.c:448:4: warning: (near initialization for
‘enumRelOpts[0].members’)
and so on, see full warning list attached.
I've played with it around, and did some googling, but without much success.
If we are moving this way (an this way seems to be good one), I need you help,
because this thing is beyond my C knowledge, I will not manage it myself.
I also attached a diff of what I have done to get these warnings. It should be
applied to latest version of patch we are discussing.
reloptions.c:447:3: warning: braces around scalar initializer
{
^
reloptions.c:447:3: warning: (near initialization for ‘enumRelOpts[0].members’)
reloptions.c:448:4: warning: braces around scalar initializer
{"on", GIST_OPTION_BUFFERING_ON },
^
reloptions.c:448:4: warning: (near initialization for ‘enumRelOpts[0].members’)
reloptions.c:448:4: warning: initialization from incompatible pointer type
reloptions.c:448:4: warning: (near initialization for ‘enumRelOpts[0].members’)
reloptions.c:448:4: warning: excess elements in scalar initializer
reloptions.c:448:4: warning: (near initialization for ‘enumRelOpts[0].members’)
reloptions.c:449:4: warning: braces around scalar initializer
{"off", GIST_OPTION_BUFFERING_OFF },
^
reloptions.c:449:4: warning: (near initialization for ‘enumRelOpts[0].members’)
reloptions.c:449:4: warning: initialization from incompatible pointer type
reloptions.c:449:4: warning: (near initialization for ‘enumRelOpts[0].members’)
reloptions.c:449:4: warning: excess elements in scalar initializer
reloptions.c:449:4: warning: (near initialization for ‘enumRelOpts[0].members’)
reloptions.c:449:4: warning: excess elements in scalar initializer
reloptions.c:449:4: warning: (near initialization for ‘enumRelOpts[0].members’)
reloptions.c:450:4: warning: braces around scalar initializer
{"auto", GIST_OPTION_BUFFERING_AUTO },
^
reloptions.c:450:4: warning: (near initialization for ‘enumRelOpts[0].members’)
reloptions.c:450:4: warning: initialization from incompatible pointer type
reloptions.c:450:4: warning: (near initialization for ‘enumRelOpts[0].members’)
reloptions.c:450:4: warning: excess elements in scalar initializer
reloptions.c:450:4: warning: (near initialization for ‘enumRelOpts[0].members’)
reloptions.c:450:4: warning: excess elements in scalar initializer
reloptions.c:450:4: warning: (near initialization for ‘enumRelOpts[0].members’)
reloptions.c:451:4: warning: braces around scalar initializer
{ NULL }
^
reloptions.c:451:4: warning: (near initialization for ‘enumRelOpts[0].members’)
reloptions.c:451:4: warning: excess elements in scalar initializer
reloptions.c:451:4: warning: (near initialization for ‘enumRelOpts[0].members’)
diff --git a/.gitignore b/.gitignore
index 794e35b..37331c2 100644
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index fffab3a..fcf4766 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -433,8 +433,6 @@ static relopt_real realRelOpts[] =
{{NULL}}
};
-static relopt_enum_elt_def gist_buffering_enum_def[] =
-GIST_OPTION_BUFFERING_ENUM_DEF;
static relopt_enum_elt_def view_check_option_enum_def[] =
VIEW_OPTION_CHECK_OPTION_ENUM_DEF;
static relopt_enum enumRelOpts[] =
@@ -446,8 +444,13 @@ static relopt_enum enumRelOpts[] =
RELOPT_KIND_GIST,
AccessExclusiveLock
},
- gist_buffering_enum_def,
- GIST_OPTION_BUFFERING_AUTO
+ {
+ {"on", GIST_OPTION_BUFFERING_ON },
+ {"off", GIST_OPTION_BUFFERING_OFF },
+ {"auto", GIST_OPTION_BUFFERING_AUTO },
+ { NULL }
+ },
+ GIST_OPTION_BUFFERING_AUTO
},
{
{
diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
index d586b04..047490a 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -379,24 +379,15 @@ typedef struct GISTBuildBuffers
} GISTBuildBuffers;
/*
- * Buffering is an enum option
- * gist_option_buffering_numeric_values defines a numeric representatio
Re: [PATCH] src/test/modules/dummy_index -- way to test reloptions from inside of access method
В Fri, 2 Aug 2019 11:12:35 +1200 Thomas Munro пишет: > While moving this to the September CF, I noticed this failure: > > test reloptions ... FAILED 32 ms Do you have any idea, how to reproduce this? I tried this patch on current master, and did not get result you are talking about. Is it still there for you BTW?
Re: [PATCH] src/test/modules/dummy_index -- way to test reloptions from inside of access method
В письме от четверг, 19 сентября 2019 г. 17:32:03 MSK пользователь Michael Paquier написал: > > src/test/modules/dummy_index_am directory check" I see a similar > > failure with "ERROR: unrecognized lock mode: 2139062143". I changed > > > that to a PANIC and got a core file containing this stack: > A simple make check on the module reproduces the failure, so that's > hard to miss. For some reason it does not reproduce on my dev environment, but it not really important, since the core of the problem is found. > > From what I can see it is not a problem caused directly by this > module, specifically knowing that this test module is actually copying > what bloom is doing when declaring its reloptions. Take this example: > CREATE EXTENSION bloom; > CREATE TABLE tbloom AS > SELECT > (random() * 100)::int as i1, > (random() * 100)::int as i2, > (random() * 100)::int as i3, > (random() * 100)::int as i4, > (random() * 100)::int as i5, > (random() * 100)::int as i6 > FROM >generate_series(1,100); > CREATE INDEX bloomidx ON tbloom USING bloom (i1,i2,i3) >WITH (length=80, col1=2, col2=2, col3=4); > ALTER INDEX bloomidx SET (length=100); > > And then you get that: > ERROR: XX000: unrecognized lock mode: 2139062143 > LOCATION: LockAcquireExtended, lock.c:756 > > So the options are registered in the relOpts array managed by > reloptions.c but the data is not properly initialized. Hence when > looking at the lock needed we have an option match, but the lock > number is incorrect, causing the failure. It looks like there is no > direct way to enforce the lockmode used for a reloption added via > add_int_reloption which does the allocation to add the option to > add_reloption(), but enforcing the value to be initialized fixes the > issue: > --- a/src/backend/access/common/reloptions.c > +++ b/src/backend/access/common/reloptions.c > @@ -658,6 +658,7 @@ allocate_reloption(bits32 kinds, int type, const > char *name, const char *desc) > newoption->kinds = kinds; > newoption->namelen = strlen(name); > newoption->type = type; > + newoption->lockmode = AccessExclusiveLock; > MemoryContextSwitchTo(oldcxt); What a good catch! dummy_index already proved to be useful ;-) > I would think that initializing that to a sane default is something > that we should do anyway, or is there some trick allowing the > manipulation of relOpts I am missing? Yes I think AccessExclusiveLock is quite good default I think. Especially in the case when these options are not really used in real world ;-) > Changing the relopts APIs in > back-branches is a no-go of course, but we could improve that for > 13~. As you know I have plans for rewriting options engine and there would be same options code both for core Access Methods and for options for AM from extensions. So there would be API for setting lockmode... But the way it is going right now, I am not sure it will reviewed to reach 13... PS. Michael, who will submit this lock mode patch? Hope you will do it? It should go separately from dummy_index for sure...
Re: [PATCH] src/test/modules/dummy_index -- way to test reloptions from inside of access method
В Fri, 20 Sep 2019 20:58:27 +0900 Michael Paquier пишет: Sorry I am really slow to answer... I hope it is not too late. > The GUCs are also basically not necessary, as you can just replace the > various WARNING calls (please don't call elog on anything which can be > reached by the user!) by lookups at reloptions in pg_class. Once this > is removed, the whole code gets more simple, and there is no point in > having neither a separate header nor a different set of files and the > size of the whole module gets really reduced. Reading options from pg_class is not a good idea. We already do this in reloption regression test. Here the thing is almost same... My point of testing was to read these values from bytea right from inside of the module. This is not exactly the same value as in pg_class. It _should_ be the same. But nobody promised is _is_ the same. That is why I was reading it right from relotions in-memory bytea, the same way real access methods do it. And then we came to a GUC variables. Because it we have five reloptions and we print them all each time we change something, there would be quite huge output. It is ok when everything goes well. Comparing with 'expected' is cheap. But is something goes wrong, then it would be very difficult to find proper place in this output to deal with it. So I created GUCs so we can get only one output in a row, not a whole bunch. These are my points.
Re: [PATCH] src/test/modules/dummy_index -- way to test reloptions from inside of access method
В письме от вторник, 24 сентября 2019 г. 9:25:54 MSK пользователь Alvaro Herrera написал: > 0003 looks useful, thanks for completing it. I think it would be a good > idea to test invalid values for each type of reloption too (passing > floating point to integers, strings to floating point, and so on). We already do it in reloption regression tests. My idea was to test here only the things that can't be tested in regression tests, on in real indexes like bloom.
Re: [PATCH] Do not use StdRdOptions in Access Methods
В Fri, 27 Sep 2019 17:24:49 +0900
Michael Paquier пишет:
> > Looks like some good actionable feedback. I've moved this patch to
> > September, and set it to "Waiting on Author".
>
> The patch is in this state for two months now, so I have switched it
> to "returned with feedback". The latest patch does not apply, and it
> would require an update for the new test module dummy_index_am.
I've been thinking about this patch and came to a conclusion that it
would be better to split it to even smaller parts, so they can be
easily reviewed, one by one. May be even leaving most complex
Heap/Toast part for later.
This is first part of this patch. Here we give each Access Method it's
own option binary representation instead of StdRdOptions.
I think this change is obvious. Because, first, Access Methods do not
use most of the values defined in StdRdOptions.
Second this patch make Options structure code unified for all core
Access Methods. Before some AM used StdRdOptions, some AM used it's own
structure, now all AM uses own structures and code is written in the
same style, so it would be more easy to update it in future.
John Dent, would you join reviewing this part of the patch? I hope it
will be more easy now...
--
Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da
Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index b5072c0..33f770b 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -23,7 +23,7 @@
#include "access/htup_details.h"
#include "access/nbtree.h"
#include "access/reloptions.h"
-#include "access/spgist.h"
+#include "access/spgist_private.h"
#include "catalog/pg_type.h"
#include "commands/defrem.h"
#include "commands/tablespace.h"
diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c
index 838ee68..eec2100 100644
--- a/src/backend/access/hash/hashpage.c
+++ b/src/backend/access/hash/hashpage.c
@@ -359,7 +359,7 @@ _hash_init(Relation rel, double num_tuples, ForkNumber forkNum)
data_width = sizeof(uint32);
item_width = MAXALIGN(sizeof(IndexTupleData)) + MAXALIGN(data_width) +
sizeof(ItemIdData); /* include the line pointer */
- ffactor = RelationGetTargetPageUsage(rel, HASH_DEFAULT_FILLFACTOR) / item_width;
+ ffactor = (BLCKSZ * HashGetFillFactor(rel) / 100) / item_width;
/* keep to a sane range */
if (ffactor < 10)
ffactor = 10;
diff --git a/src/backend/access/hash/hashutil.c b/src/backend/access/hash/hashutil.c
index 3fb92ea..5170634 100644
--- a/src/backend/access/hash/hashutil.c
+++ b/src/backend/access/hash/hashutil.c
@@ -289,7 +289,28 @@ _hash_checkpage(Relation rel, Buffer buf, int flags)
bytea *
hashoptions(Datum reloptions, bool validate)
{
- return default_reloptions(reloptions, validate, RELOPT_KIND_HASH);
+ relopt_value *options;
+ HashRelOptions *rdopts;
+ int numoptions;
+ static const relopt_parse_elt tab[] = {
+ {"fillfactor", RELOPT_TYPE_INT, offsetof(HashRelOptions, fillfactor)},
+ };
+
+ options = parseRelOptions(reloptions, validate, RELOPT_KIND_HASH,
+ &numoptions);
+
+ /* if none set, we're done */
+ if (numoptions == 0)
+ return NULL;
+
+ rdopts = allocateReloptStruct(sizeof(HashRelOptions), options, numoptions);
+
+ fillRelOptions((void *) rdopts, sizeof(HashRelOptions), options, numoptions,
+ validate, tab, lengthof(tab));
+
+ pfree(options);
+
+ return (bytea *) rdopts;
}
/*
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 4cfd528..b460d13 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -816,7 +816,7 @@ _bt_vacuum_needs_cleanup(IndexVacuumInfo *info)
}
else
{
- StdRdOptions *relopts;
+ BTRelOptions *relopts;
float8 cleanup_scale_factor;
float8 prev_num_heap_tuples;
@@ -827,7 +827,7 @@ _bt_vacuum_needs_cleanup(IndexVacuumInfo *info)
* tuples exceeds vacuum_cleanup_index_scale_factor fraction of
* original tuples count.
*/
- relopts = (StdRdOptions *) info->index->rd_options;
+ relopts = (BTRelOptions *) info->index->rd_options;
cleanup_scale_factor = (relopts &&
relopts->vacuum_cleanup_index_scale_factor >= 0)
? relopts->vacuum_cleanup_index_scale_factor
diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c
index ab19692..f7c2377 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -725,8 +725,9 @@ _bt_pagestate(BTWriteState *wstate, uint32 level)
if (level > 0)
state->btps_full = (BLCKSZ * (100 - BTREE_NONLEAF_FILLFACTOR) / 100);
else
- state->btps_full = RelationGetTargetPageFreeSpace(wstate->index,
- BTREE_DEFAULT_FILLFACTOR);
+ state->btps_full = (BLCKSZ * (100 - BTGetFillFactor(wstate->index))
+ / 100);
+
/* no parent level, yet */
state->btps_next = NULL;
diff --gi
[PATCH] Add some useful asserts into View Options macroses
This thread is a follow up to the thread https://www.postgresql.org/message-id/2620882.s52SJui4ql@x200m where I've been trying to remove StdRdOptions structure and replace it with unique structure for each relation kind. I've decided to split that patch into smaller parts. This part adds some asserts to ViewOptions macroses. Since an option pointer there is converted into (ViewOptions *) it would be really good to make sure that this macros is called in proper context, and we do the convertation properly. At least when running tests with asserts turned on. -- Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h index a5cf804..b24fb1d 100644 --- a/src/include/utils/rel.h +++ b/src/include/utils/rel.h @@ -351,9 +351,10 @@ typedef struct ViewOptions * Returns whether the relation is security view, or not. Note multiple * eval of argument! */ -#define RelationIsSecurityView(relation) \ - ((relation)->rd_options ?\ - ((ViewOptions *) (relation)->rd_options)->security_barrier : false) +#define RelationIsSecurityView(relation) \ + (AssertMacro(relation->rd_rel->relkind == RELKIND_VIEW),\ + ((relation)->rd_options ?\ + ((ViewOptions *) (relation)->rd_options)->security_barrier : false)) /* * RelationHasCheckOption @@ -361,9 +362,10 @@ typedef struct ViewOptions * or the cascaded check option. Note multiple eval of argument! */ #define RelationHasCheckOption(relation) \ + (AssertMacro(relation->rd_rel->relkind == RELKIND_VIEW),\ ((relation)->rd_options &&\ ((ViewOptions *) (relation)->rd_options)->check_option !=\ - VIEW_OPTION_CHECK_OPTION_NOT_SET) + VIEW_OPTION_CHECK_OPTION_NOT_SET)) /* * RelationHasLocalCheckOption @@ -371,9 +373,10 @@ typedef struct ViewOptions * option. Note multiple eval of argument! */ #define RelationHasLocalCheckOption(relation)\ + (AssertMacro(relation->rd_rel->relkind == RELKIND_VIEW),\ ((relation)->rd_options &&\ ((ViewOptions *) (relation)->rd_options)->check_option ==\ - VIEW_OPTION_CHECK_OPTION_LOCAL) + VIEW_OPTION_CHECK_OPTION_LOCAL)) /* * RelationHasCascadedCheckOption @@ -381,9 +384,10 @@ typedef struct ViewOptions * option. Note multiple eval of argument! */ #define RelationHasCascadedCheckOption(relation) \ + (AssertMacro(relation->rd_rel->relkind == RELKIND_VIEW),\ ((relation)->rd_options &&\ ((ViewOptions *) (relation)->rd_options)->check_option ==\ - VIEW_OPTION_CHECK_OPTION_CASCADED) + VIEW_OPTION_CHECK_OPTION_CASCADED)) /* * RelationIsValid signature.asc Description: This is a digitally signed message part.
[PATCH] use separate PartitionedRelOptions structure to store partitioned table options
Hi!
This message is follow up to the "Get rid of the StdRdOptions" patch thread:
https://www.postgresql.org/message-id/2620882.s52SJui4ql@x200m
I've split patch into even smaller parts and commitfest want each patch in
separate thread. So it is new thread.
The idea of this patch is following: If you read the code, partitioned tables
do not have any options (you will not find RELOPT_KIND_PARTITIONED in
boolRelOpts, intRelOpts, realRelOpts, stringRelOpts and enumRelOpts in
reloption.c), but it uses StdRdOptions to store them (these no options).
If partitioned table is to have it's own option set (even if it is empty now)
it would be better to save it into separate structure, like it is done for
Views, not adding them to StdRdOptions, like current code hints to do.
So in this patch I am creating a structure that would store partitioned table
options (it is empty for now as there are no options for this relation kind),
and all other code that would use this structure as soon as the first option
comes.
I think it is bad idea to suggest option adder to ad it to StdRdOption, we
already have a big mess there. Better if he add it to an new empty structure.
--
Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da
Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index b5072c0..f219ad3 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -1099,9 +1099,11 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
case RELKIND_RELATION:
case RELKIND_TOASTVALUE:
case RELKIND_MATVIEW:
- case RELKIND_PARTITIONED_TABLE:
options = heap_reloptions(classForm->relkind, datum, false);
break;
+ case RELKIND_PARTITIONED_TABLE:
+ options = partitioned_reloptions(datum, false);
+ break;
case RELKIND_VIEW:
options = view_reloptions(datum, false);
break;
@@ -1538,6 +1540,25 @@ default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
}
/*
+ * Option parser for partitioned relations
+ */
+bytea *
+partitioned_reloptions(Datum reloptions, bool validate)
+{
+ int numoptions;
+
+ /*
+ * Since there is no options for patitioned table for now, we just do
+ * validation to report incorrect option error and leave.
+ */
+
+ if (validate)
+ parseRelOptions(reloptions, validate, RELOPT_KIND_PARTITIONED,
+ &numoptions);
+ return NULL;
+}
+
+/*
* Option parser for views
*/
bytea *
@@ -1593,9 +1614,6 @@ heap_reloptions(char relkind, Datum reloptions, bool validate)
case RELKIND_RELATION:
case RELKIND_MATVIEW:
return default_reloptions(reloptions, validate, RELOPT_KIND_HEAP);
- case RELKIND_PARTITIONED_TABLE:
- return default_reloptions(reloptions, validate,
- RELOPT_KIND_PARTITIONED);
default:
/* other relkinds are not supported */
return NULL;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 05593f3..db27f30 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -720,10 +720,17 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
reloptions = transformRelOptions((Datum) 0, stmt->options, NULL, validnsps,
true, false);
- if (relkind == RELKIND_VIEW)
- (void) view_reloptions(reloptions, true);
- else
- (void) heap_reloptions(relkind, reloptions, true);
+ switch ((int)relkind)
+ {
+ case RELKIND_VIEW:
+ (void) view_reloptions(reloptions, true);
+ break;
+ case RELKIND_PARTITIONED_TABLE:
+ (void) partitioned_reloptions(reloptions, true);
+ break;
+ default:
+ (void) heap_reloptions(relkind, reloptions, true);
+ }
if (stmt->ofTypename)
{
@@ -12158,9 +12165,11 @@ ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation,
case RELKIND_RELATION:
case RELKIND_TOASTVALUE:
case RELKIND_MATVIEW:
- case RELKIND_PARTITIONED_TABLE:
(void) heap_reloptions(rel->rd_rel->relkind, newOptions, true);
break;
+ case RELKIND_PARTITIONED_TABLE:
+ (void) partitioned_reloptions(newOptions, true);
+ break;
case RELKIND_VIEW:
(void) view_reloptions(newOptions, true);
break;
diff --git a/src/include/access/reloptions.h b/src/include/access/reloptions.h
index 6bde209..0ad5c74 100644
--- a/src/include/access/reloptions.h
+++ b/src/include/access/reloptions.h
@@ -301,6 +301,7 @@ extern bytea *default_reloptions(Datum reloptions, bool validate,
relopt_kind kind);
extern bytea *heap_reloptions(char relkind, Datum reloptions, bool validate);
extern bytea *view_reloptions(Datum reloptions, bool validate);
+extern bytea *partitioned_reloptions(Datum reloptions, bool validate);
extern bytea *index_reloptions(amoptions_function amoptions, Datum reloptions,
bool validate);
extern bytea *attribute_reloptions(Datum reloptions, bool validate);
diff --git a/src/include/utils/rel.h b/src/include/utils/r
Re: [PATCH] Do not use StdRdOptions in Access Methods
Hi! I am starting a new thread as commitfest wants new thread for new patch.
This new thread is a follow up to an https://www.postgresql.org/message-id/
2620882.s52SJui4ql%40x200m thread, where I've been trying to get rid of
StdRdOpions structure, and now I've splitted the patch into smaller parts.
Read the quote below, to get what this patch is about
> I've been thinking about this patch and came to a conclusion that it
> would be better to split it to even smaller parts, so they can be
> easily reviewed, one by one. May be even leaving most complex
> Heap/Toast part for later.
>
> This is first part of this patch. Here we give each Access Method it's
> own option binary representation instead of StdRdOptions.
>
> I think this change is obvious. Because, first, Access Methods do not
> use most of the values defined in StdRdOptions.
>
> Second this patch make Options structure code unified for all core
> Access Methods. Before some AM used StdRdOptions, some AM used it's own
> structure, now all AM uses own structures and code is written in the
> same style, so it would be more easy to update it in future.
>
> John Dent, would you join reviewing this part of the patch? I hope it
> will be more easy now...
And now I have a newer version of the patch, as I forgot to remove
vacuum_cleanup_index_scale_factor from StdRdOptions as it was used only in
Btree index and now do not used at all. New version fixes it.
--
Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da
Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index b5072c0..02ee3c9 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -23,7 +23,7 @@
#include "access/htup_details.h"
#include "access/nbtree.h"
#include "access/reloptions.h"
-#include "access/spgist.h"
+#include "access/spgist_private.h"
#include "catalog/pg_type.h"
#include "commands/defrem.h"
#include "commands/tablespace.h"
@@ -1513,8 +1513,6 @@ default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
offsetof(StdRdOptions, user_catalog_table)},
{"parallel_workers", RELOPT_TYPE_INT,
offsetof(StdRdOptions, parallel_workers)},
- {"vacuum_cleanup_index_scale_factor", RELOPT_TYPE_REAL,
- offsetof(StdRdOptions, vacuum_cleanup_index_scale_factor)},
{"vacuum_index_cleanup", RELOPT_TYPE_BOOL,
offsetof(StdRdOptions, vacuum_index_cleanup)},
{"vacuum_truncate", RELOPT_TYPE_BOOL,
diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c
index 838ee68..eec2100 100644
--- a/src/backend/access/hash/hashpage.c
+++ b/src/backend/access/hash/hashpage.c
@@ -359,7 +359,7 @@ _hash_init(Relation rel, double num_tuples, ForkNumber forkNum)
data_width = sizeof(uint32);
item_width = MAXALIGN(sizeof(IndexTupleData)) + MAXALIGN(data_width) +
sizeof(ItemIdData); /* include the line pointer */
- ffactor = RelationGetTargetPageUsage(rel, HASH_DEFAULT_FILLFACTOR) / item_width;
+ ffactor = (BLCKSZ * HashGetFillFactor(rel) / 100) / item_width;
/* keep to a sane range */
if (ffactor < 10)
ffactor = 10;
diff --git a/src/backend/access/hash/hashutil.c b/src/backend/access/hash/hashutil.c
index 3fb92ea..5170634 100644
--- a/src/backend/access/hash/hashutil.c
+++ b/src/backend/access/hash/hashutil.c
@@ -289,7 +289,28 @@ _hash_checkpage(Relation rel, Buffer buf, int flags)
bytea *
hashoptions(Datum reloptions, bool validate)
{
- return default_reloptions(reloptions, validate, RELOPT_KIND_HASH);
+ relopt_value *options;
+ HashRelOptions *rdopts;
+ int numoptions;
+ static const relopt_parse_elt tab[] = {
+ {"fillfactor", RELOPT_TYPE_INT, offsetof(HashRelOptions, fillfactor)},
+ };
+
+ options = parseRelOptions(reloptions, validate, RELOPT_KIND_HASH,
+ &numoptions);
+
+ /* if none set, we're done */
+ if (numoptions == 0)
+ return NULL;
+
+ rdopts = allocateReloptStruct(sizeof(HashRelOptions), options, numoptions);
+
+ fillRelOptions((void *) rdopts, sizeof(HashRelOptions), options, numoptions,
+ validate, tab, lengthof(tab));
+
+ pfree(options);
+
+ return (bytea *) rdopts;
}
/*
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 4cfd528..b460d13 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -816,7 +816,7 @@ _bt_vacuum_needs_cleanup(IndexVacuumInfo *info)
}
else
{
- StdRdOptions *relopts;
+ BTRelOptions *relopts;
float8 cleanup_scale_factor;
float8 prev_num_heap_tuples;
@@ -827,7 +827,7 @@ _bt_vacuum_needs_cleanup(IndexVacuumInfo *info)
* tuples exceeds vacuum_cleanup_index_scale_factor fraction of
* original tuples count.
*/
- relopts = (StdRdOptions *) info->index->rd_options;
+ relopts = (BTRelOptions *) info->index->rd_options;
cleanup_scale_factor = (relopts &&
relopts->vacuum_cleanup_index_scale_factor
Re: [PATCH] get rid of StdRdOptions, use individual binary reloptions representation for each relation kind instead
В письме от пятница, 27 сентября 2019 г. 17:24:49 MSK пользователь Michael Paquier написал: > The patch is in this state for two months now, so I have switched it > to "returned with feedback". So I've split this patch into even smaller parts, so it would be more easy to review. Do not use StdRdOptions in Access Methods https://www.postgresql.org/message-id/4127670.gFlpRb6XCm@x200m Use empty PartitionedRelOption structure for storing partitioned table options instead of StdRdOption https://www.postgresql.org/message-id/1627387.Qykg9O6zpu@x200m and Some useful asserts in ViewOptions Macroses https://www.postgresql.org/message-id/3634983.eHpMQ1mJnI@x200m as for removing StdRdOptions for Heap and Toast, I would suggest for commit it later. It is not critical for my reloptions refactoring plans. I can do it having one structure for two relation kinds. So we can split it into two later, or do not split at all... -- Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian) signature.asc Description: This is a digitally signed message part.
Re: [PATCH] use separate PartitionedRelOptions structure to store partitioned table options
В письме от понедельник, 7 октября 2019 г. 14:57:14 MSK пользователь Michael Paquier написал: > On Sun, Oct 06, 2019 at 03:47:46PM +0300, Nikolay Shaplov wrote: > > This message is follow up to the "Get rid of the StdRdOptions" patch > > thread: https://www.postgresql.org/message-id/2620882.s52SJui4ql@x200m > > > > I've split patch into even smaller parts and commitfest want each patch in > > separate thread. So it is new thread. > > Splitting concepts into different threads may be fine, and usually > recommended. Splitting a set of patches into multiple entries to ease > review and your goal to get a patch integrated and posted all these > into the same thread is usually recommended. Now posting a full set > of patches across multiple threads, in way so as they have > dependencies with each other, is what I would call a confusing > situation. That's hard to follow. I understand that. I've tried to add new patches to original thread, but commitfest did not accept that for some reason. You can try to add patch from this letter https://www.postgresql.org/message-id/2620882.s52SJui4ql@x200m just to see how it works. Since discussion actually did not started yet, it can be moved anywhere you suggest, but please tell how exactly it should be done, because I do not understand what is the better way. > > The idea of this patch is following: If you read the code, partitioned > > tables do not have any options (you will not find RELOPT_KIND_PARTITIONED > > in boolRelOpts, intRelOpts, realRelOpts, stringRelOpts and enumRelOpts in > > reloption.c), but it uses StdRdOptions to store them (these no options). > I am not even sure that we actually need that. What kind of reloption > you would think would suit into this subset? Actually I do not know. But the author of partitioned patch, added a stub for partitioned tables to have some reloptions in future. But this stub is designed to use StdRdOptions. Which is not correct, as I presume. So here I am correcting the stub. -- Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian) signature.asc Description: This is a digitally signed message part.
Re: [PATCH] Do not use StdRdOptions in Access Methods
В письме от понедельник, 7 октября 2019 г. 18:55:20 MSK пользователь Dent John написал: > I like the new approach. And I agree with the ambition — to split out the > representation from StdRdOptions. Thanks. > However, with that change, in the AM’s *options() function, it looks as if > you could simply add new fields to the relopt_parse_elt list. That’s still > not true, because parseRelOptions() will fail to find a matching entry, > causing numoptions to be left zero, and an early exit made. (At least, > that’s if I correctly understand how things work.) > > I think that is fine as an interim limitation, because your change has not > yet fully broken the connection to the boolRelOpts, intRelOpts, realRelOpts > and stringRelOpts strutures. But perhaps a comment would help to make it > clear. Perhaps add something like this above the tab[]: "When adding or > changing a relopt in the relopt_parse_elt tab[], ensure the corresponding > change is made to boolRelOpts, intRelOpts, realRelOpts or stringRelOpts." Whoa-whoa! I am not inventing something new here. Same code is already used in brin (brin.c:820), gin (ginutils.c:602) and gist (gistutils.c:909) indexes. I've just copied the idea, to do all index code uniform. This does not mean that these code can't be improved, but as far as I can guess it is better to do it in small steps, first make option code uniform, and then improve all of it this way or another... So I here I would suggest to discuss it I copied this code correctly, without going very deeply into discussion how we can improve code I've used as a source for cloning. And then I have ideas how to do it better. But this will come later... -- Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)
Re: [PATCH] Add some useful asserts into View Options macroses
В письме от понедельник, 7 октября 2019 г. 12:59:27 MSK пользователь Robert Haas написал: > > This thread is a follow up to the thread > > https://www.postgresql.org/message-id/2620882.s52SJui4ql@x200m where I've > > been trying to remove StdRdOptions structure and replace it with unique > > structure for each relation kind. > > > > I've decided to split that patch into smaller parts. > > > > This part adds some asserts to ViewOptions macroses. > > Since an option pointer there is converted into (ViewOptions *) it would > > be > > really good to make sure that this macros is called in proper context, and > > we do the convertation properly. At least when running tests with asserts > > turned on. > Seems like a good idea. Should we try to do something similar for the > macros in that header file that cast to StdRdOptions? That would not be as easy as for ViewOptions. For example as for the current master code, fillfactor from StdRdOptions is used in Toast, Heap, Hash index, nbtree index, and spgist index. This will make RelationGetFillFactor macros a bit complicated for example. Now I have patches that limits usage of StdRdOptions to Heap and Toast. When StdRdOptions is not that widely used, we whould be able to add asserts for it, it will not make the code too complex. So I would suggest to do ViewOptions asserts now, and keep dealing with StdRdOptions for later. When we are finished with my current patches, I will take care about it. -- Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)
Re: [PATCH] use separate PartitionedRelOptions structure to store partitioned table options
В письме от вторник, 8 октября 2019 г. 16:00:49 MSK пользователь Amit Langote написал: > > > > The idea of this patch is following: If you read the code, partitioned > > > > tables do not have any options (you will not find > > > > RELOPT_KIND_PARTITIONED > > > > in boolRelOpts, intRelOpts, realRelOpts, stringRelOpts and enumRelOpts > > > > in > > > > reloption.c), but it uses StdRdOptions to store them (these no > > > > options). > > > > > > I am not even sure that we actually need that. What kind of reloption > > > you would think would suit into this subset? > > > > Actually I do not know. But the author of partitioned patch, added a stub > > for partitioned tables to have some reloptions in future. But this stub > > is designed to use StdRdOptions. Which is not correct, as I presume. So > > here I am correcting the stub. > > I wrote the patch that introduced RELOPT_KIND_PARTITIONED. Yes, it > was added as a stub relopt_kind to eventually apply to reloptions that > could be sensibly applied to partitioned tables. We never got around > to working on making the existing reloptions relevant to partitioned > tables, nor did we add any new partitioned table-specific reloptions, > so it remained an unused relopt_kind. Thank you for clarifying thing. > IIUC, this patch invents PartitionedRelOptions as the binary > representation for future RELOPT_KIND_PARTITIONED parameters. As long > as others are on board with using different *Options structs for > different object kinds, I see no problem with this idea. Yes, this is correct. Some Access Methods already use it's own Options structure. As far as I can guess StdRdOptions still exists only for historical reasons, and became quite a mess because of adding all kind of stuff there. Better to separate it. BTW, as far as you are familiar with this part of the code, may be you will join the review if this particular patch? -- Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)
Re: [PATCH] Do not use StdRdOptions in Access Methods
В письме от среда, 9 октября 2019 г. 20:26:14 MSK пользователь Dent John написал: > I didn’t spot it was an existing pattern. Sorry, this might be my mistake I should explicitly mention it in the first place. > And I agree — making the code uniform will make it easier to evolve in > future. > > Gets my vote. Thanks! Can you please check, I did not do any mistake while copying and adapting the code. -- Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)
Re: [PATCH] use separate PartitionedRelOptions structure to store partitioned table options
В Thu, 10 Oct 2019 15:50:05 +0900
Amit Langote пишет:
> > I think it is bad idea to suggest option adder to ad it to
> > StdRdOption, we already have a big mess there. Better if he add it
> > to an new empty structure.
>
> I tend to agree that this improves readability of the reloptions code
> a bit.
>
> Some comments on the patch:
>
> How about naming the function partitioned_table_reloptions() instead
> of partitioned_reloptions()?
I have my doubts about using word table here... In relational model
there are no such concept as "table", it uses concept "relation". And
in postgres there were no tables, there were only relations. Heap
relation, toast relation, all kind of index relations... I do not
understand when and why word "table" appeared when we speak about some
virtual relation that is made of several real heap relation. That is
why I am not using the word table here...
But since you are the author of partition table code, and this code is
already accepted in the core, you should know better. So I will change
it the way you say.
> +switch ((int)relkind)
>
> Needs a space between (int) and relkind, but I don't think the (int)
> cast is really necessary. I don't see it in other places.
Oh. Yeh. This is my mistake... I had some strange compilation
problems, and this is a remnant of my efforts to find the cause of
it, I've forgot to clean...
Thanks!
> Speaking of partitioned relations vs tables, I see that you didn't
> touch partitioned indexes (RELKIND_PARTITIONED_INDEX relations). Is
> that because we leave option handling to the index AMs?
Because for partitioned indexes the code says "Use same options
original index does"
bytea *
extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
amoptions_function amoptions)
{
switch (classForm->relkind)
{
case RELKIND_RELATION:
case RELKIND_TOASTVALUE:
case RELKIND_MATVIEW:
options = heap_reloptions(classForm->relkind, datum, false);
break;
case RELKIND_PARTITIONED_TABLE:
options = partitioned_table_reloptions(datum, false);
break;
case RELKIND_VIEW:
options = view_reloptions(datum, false);
break;
case RELKIND_INDEX:
case RELKIND_PARTITIONED_INDEX:
options = index_reloptions(amoptions, datum, false);
break;
Here you see the function accepts amoptions method that know how to
parse options for a particular index, and passes it to index_reloptions
functions. For both indexes and partitioned indexes it is taken from AM
"method" amoptions. So options uses exactly the same code and same
options both for indexes and for partitioned indexes.
I do not know if it is correct from global point of view, but from the
point of view of reloptions engine, it does not require any attention:
change index options and get partitioned_index options for free...
Actually I would expect some problems there, because sooner or later,
somebody would want to set custom fillfactor to partitioned table, or
set some custom autovacuum options for it. But I would prefer to think
about it when I am done with reloption engine rewriting... Working in
both direction will cause more trouble then get benefits...
diff --git a/.gitignore b/.gitignore
index 794e35b..37331c2 100644
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index b5072c0..067441f 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -1099,9 +1099,11 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
case RELKIND_RELATION:
case RELKIND_TOASTVALUE:
case RELKIND_MATVIEW:
- case RELKIND_PARTITIONED_TABLE:
options = heap_reloptions(classForm->relkind, datum, false);
break;
+ case RELKIND_PARTITIONED_TABLE:
+ options = partitioned_table_reloptions(datum, false);
+ break;
case RELKIND_VIEW:
options = view_reloptions(datum, false);
break;
@@ -1538,6 +1540,25 @@ default_reloptions(Datum reloptions, bool
Re: [PATCH] Do not use StdRdOptions in Access Methods
В письме от четверг, 10 октября 2019 г. 17:17:30 MSK пользователь Amit Langote написал: > I read comments that Tomas left at: > https://www.postgresql.org/message-id/20190727173841.7ypzo4xuzizvijge%40deve > lopment > > I'd like to join Michael in reiterating one point from Tomas' review. > I think the patch can go further in trying to make the code in this > area more maintainable. > > For example, even without this patch, the following stanza is repeated > in many places: > > options = parseRelOptions(reloptions, validate, foo_relopt_kind, > &numoptions); > rdopts = allocateReloptStruct(sizeof(FooOptions), options, numoptions); > fillRelOptions((void *) rdopts, sizeof(FooOptions), options, numoptions, > validate, foo_relopt_tab, lengthof(foo_relopt_tab)); return (bytea *) > rdopts; > > and this patch adds few more instances as it's adding more Options structs. > > I think it wouldn't be hard to encapsulate the above stanza in a new > public function in reloptions.c and call it from the various places > that now have the above code. The code of reloptions is very historical and illogical. I also noticed that these lines are repeated several time. And may be it would be better to put them into reloptions.c. But could anybody clearly explain what are they doing? Just to give function a proper name. I understand what they are doing, but I am unable to give short and clear explanation. I am planning to rewrite this part completely. So we have none of this lines repeated. I had a proposal you can see it here https:// commitfest.postgresql.org/15/992/ but people on the list told be that patch is too complex and I should commit it part by part. So I am doing it now. I am almost done. But to provide clear and logical patch that introduces my concept, I need StdRdOption to be divided into separated structures. At least for AM. And I need at to be done as simply as possible because the rest of the code is going to be rewritten anyway. That is why I want to follow the steps: make the code uniform, and then improve it. I have improvement in the pocket, but I need a uniform code before revealing it. If you think it is absolutely necessary to put these line into one function, I will do it. It will not make code more clear, I guess. I See no benefits, but I can do it, but I would avoid doing it, if possible. At least at this step. -- Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)
Re: [PATCH] use separate PartitionedRelOptions structure to store partitioned table options
> > I think it is bad idea to suggest option adder to ad it to
> > StdRdOption, we already have a big mess there. Better if he add it
> > to an new empty structure.
>
> I tend to agree that this improves readability of the reloptions code
> a bit.
>
> Some comments on the patch:
>
> How about naming the function partitioned_table_reloptions() instead
> of partitioned_reloptions()?
I have my doubts about using word table here... In relational model
there are no such concept as "table", it uses concept "relation". And
in postgres there were no tables, there were only relations. Heap
relation, toast relation, all kind of index relations... I do not
understand when and why word "table" appeared when we speak about some
virtual relation that is made of several real heap relation. That is
why I am not using the word table here...
But since you are the author of partition table code, and this code is
already accepted in the core, you should know better. So I will change
it the way you say.
> +switch ((int)relkind)
>
> Needs a space between (int) and relkind, but I don't think the (int)
> cast is really necessary. I don't see it in other places.
Oh. Yeh. This is my mistake... I had some strange compilation
problems, and this is a remnant of my efforts to find the cause of
it, I've forgot to clean...
Thanks!
> Speaking of partitioned relations vs tables, I see that you didn't
> touch partitioned indexes (RELKIND_PARTITIONED_INDEX relations). Is
> that because we leave option handling to the index AMs?
Because for partitioned indexes the code says "Use same options
original index does"
bytea
* extractRelOptions(HeapTuple tuple, TupleDesc
tupdesc, amoptions_function amoptions)
{
switch
(classForm->relkind)
{ case
RELKIND_RELATION: case
RELKIND_TOASTVALUE: case
RELKIND_MATVIEW: options = heap_reloptions(classForm->relkind, datum,
false);
break; case
RELKIND_PARTITIONED_TABLE: options =
partitioned_table_reloptions(datum, false);
break; case
RELKIND_VIEW: options = view_reloptions(datum,
false);
break; case
RELKIND_INDEX: case
RELKIND_PARTITIONED_INDEX: options = index_reloptions(amoptions, datum,
false); break;
Here you see the function accepts amoptions method that know how to
parse options for a particular index, and passes it to index_reloptions
functions. For both indexes and partitioned indexes it is taken from AM
"method" amoptions. So options uses exactly the same code and same
options both for indexes and for partitioned indexes.
I do not know if it is correct from global point of view, but from the
point of view of reloptions engine, it does not require any attention:
change index options and get partitioned_index options for free...
Actually I would expect some problems there, because sooner or later,
somebody would want to set custom fillfactor to partitioned table, or
set some custom autovacuum options for it. But I would prefer to think
about it when I am done with reloption engine rewriting... Working in
both direction will cause more trouble then get benefits...diff --git a/.gitignore b/.gitignore
index 794e35b..37331c2 100644
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index b5072c0..067441f 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -1099,9 +1099,11 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
case RELKIND_RELATION:
case RELKIND_TOASTVALUE:
case RELKIND_MATVIEW:
- case RELKIND_PARTITIONED_TABLE:
options = heap_reloptions(classForm->relkind, datum, false);
break;
+ case RELKIND_PARTITIONED_TABLE:
+ options = partitioned_table_reloptions(datum, false);
+ break;
case RELKIND_VIEW:
options = view_reloptions(datum, false);
break;
@@ -1538,6 +1540,25 @@ default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
}
/*
+ * Option parser for partitioned tables
+ */
+bytea *
+partitioned_table_reloptions(Datum reloptions, bool validate)
+{
+ int numoptions;
+
+ /*
+ * Since there are no options for patitioned tables for now, we just do
+ * validation to report incorrect option error and leave.
+ */
+
+ if (validate)
+ parseRelOptions(reloptions, validate, RELOPT_KIND_PARTITIONED,
+ &numoptions);
+ return NULL;
+}
+
+/*
* Option parser for views
*/
bytea *
@@ -1593,9 +1614,6 @@ heap_reloptions(char relkind, Datum reloptions, bool validate)
case RELKIND_RELATION:
case RELKIND_MATVIEW:
return default_reloptions(reloptions, validate, RELOPT_KIND_HEAP);
- case RELKIND_PARTITIONED_TABLE:
- return default_reloptions(reloptions, validate,
- RELOPT_KIND_PARTITIONED);
default:
/* other relkinds are not supported */
return NULL;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 05593f3..f44e865 100644
--- a/src/backend/comma
[PATCH] src/test/modules/dummy_index -- way to test reloptions from inside of access method
Hi! This patch introduce a dummy_index access method module, that does not do any indexing at all, but allow to test reloptions from inside of access method extension. This patch is part of my bigger work on reloptions refactoring. It came from https://www.postgresql.org/message-id/20190220060832.GI15532%40paquier.xyz thread where I suggested to add a "enum" reloption type, and we came to conclusion that we need to test how this new option works for access method created from extension (it does not work in the same way as in-core access methods) . But we can't add this option to bloom index, so we need an index extension that can be freely used for tests. So I created src/test/modules/dummy_index, it does no real indexing, but it has all types of reloptions that can be set (reloption_int, reloption_real, reloption_bool, reloption_string and reloption_string2). It also has set of boolean GUC variables that enables test output concerning certain reloption: (do_test_reloption_int, do_test_reloption_real, do_test_reloption_bool and do_test_reloption_string and do_test_reloption_string2) also set do_test_reloptions to true to get any output at all. Dummy index will print this output when index is created, and when record is inserted (this needed to check if your ALTER TABLE did well) Then you just use normal regression tests: turns on test output, sets some reloption and check test output, that it properly reaches the access method internals. While writing this module I kept in mind the idea that this module can be also used for other am-related tests, so I separated the code into two parts: dummy_index.c has only code related to implementation of an empty access method, and all code related to reloptions tests were stored into direloptions.c. So in future somebody can add di[what_ever_he_wants].c whith his own tests code, add necessary calls to dummy_index.c, create some GUC variables, and has his own feature tested. So I kindly ask you to review and commit this module, so I would be able to continue my work on reloptions refactoring... Thanks! diff --git a/.gitignore b/.gitignore index 794e35b..37331c2 100644 diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile index 19d60a5..aa34320 100644 --- a/src/test/modules/Makefile +++ b/src/test/modules/Makefile @@ -7,6 +7,7 @@ include $(top_builddir)/src/Makefile.global SUBDIRS = \ brin \ commit_ts \ + dummy_index \ dummy_seclabel \ snapshot_too_old \ test_bloomfilter \ diff --git a/src/test/modules/dummy_index/.gitignore b/src/test/modules/dummy_index/.gitignore new file mode 100644 index 000..5dcb3ff --- /dev/null +++ b/src/test/modules/dummy_index/.gitignore @@ -0,0 +1,4 @@ +# Generated subdirectories +/log/ +/results/ +/tmp_check/ diff --git a/src/test/modules/dummy_index/Makefile b/src/test/modules/dummy_index/Makefile new file mode 100644 index 000..95c1fcc --- /dev/null +++ b/src/test/modules/dummy_index/Makefile @@ -0,0 +1,21 @@ +# contrib/bloom/Makefile + +MODULE_big = dummy_index +OBJS = dummy_index.o direloptions.o $(WIN32RES) + +EXTENSION = dummy_index +DATA = dummy_index--1.0.sql +PGFILEDESC = "dummy index access method - needed for all kinds of tests" + +REGRESS = reloptions + +ifdef USE_PGXS +PG_CONFIG = pg_config +PGXS := $(shell $(PG_CONFIG) --pgxs) +include $(PGXS) +else +subdir = src/test/modules/dummy_index +top_builddir = ../../../.. +include $(top_builddir)/src/Makefile.global +include $(top_srcdir)/contrib/contrib-global.mk +endif diff --git a/src/test/modules/dummy_index/README b/src/test/modules/dummy_index/README new file mode 100644 index 000..b704112 --- /dev/null +++ b/src/test/modules/dummy_index/README @@ -0,0 +1,34 @@ +DUMMY INDEX + +Dummy index, an index module for testing am-related internal calls that can't be +properly tested inside production indexes, but better to be tested inside actual +access method module. + +Guidelines: +1. Keep this index as simple as as possible. It should not do any real indexing +(unless you need it for your tests), if you need it make it as simple as +possible; +2. Keep all code related to feature you test inside id[name_of_a_feature].c +file. dummy_index.c file should contain code that is needed for everyone, and +make calls to feature-related code; +3. Code related to your feature tests should be enabled and disabled by GUC +variables. Create as many boolean GUC variables as you need and set them on so +your test will know when it is time to do test output. Thus output from +different features tests are not interfered; +4. Add section to this README file that describes the general idea of what your +tests do. + +RELOPTIONS TESTS + +Here we check that values of reloptions properly reaches am-internals, and that +all reloption-related code works as expected. Several GUC variables are defined +to control test's output. do_test_reloptions turns on and off any +reloptions-related output. You will also get some test out
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от среда, 20 февраля 2019 г. 15:08:32 MSK пользователь Michael Paquier написал: > > 2.5 May be this src/test/modules dummy index is subject to another patch. > > So I will start working on it right now, but we will do this work not > > dependent to any other patches. And just add there what we need when it > > is ready. I would actually add there some code that checks all types of > > options, because we actually never check that option value from reloptons > > successfully reaches extension code. I did this checks manually, when I > > wrote that bigger reloption patch, but there is no facilities to do > > regularly check is via test suit. Creating dummy index will create such > > facilities. > > bloom is a user-facing extension, while src/test/modules are normally > not installed (there is an exception for MSVC anyway..), so stressing > add_enum_reloption in src/test/modules looks more appropriate to me, > except if you can define an option which is useful for the user and is > an enum. I've created a module in src/test/modules where we would be able to add enum support when that module is commited. https://commitfest.postgresql.org/23/2064/ I've filed it as a separate patch (it is standalone work as I can feel it). Sadly I did not manage to prepare it before this commitfest, so I've added it to a next one. :-((
[PATCH][HOTFIX] vacuum_cost_delay type change from int to real have not been done everywhere
Hi!
In caf626b2 type of vacuum_cost_delay have been switched from int to real,
everywhere, but not in *RelOpts[] arrays.
For some reason it continued to build and work. But I think it is better to
move vacuum_cost_delay from int to real there too...
Patch attached.
PS. As you can see current reloption code is error-prone. To properly change
reloptions you should simultaneously change code in several different places,
and as you can see, it may not report if you missed something.
I am working on reloptions code refactoring now, please join reviewing my
patches. This work is important as you can see from this example...
diff --git a/.gitignore b/.gitignore
index 794e35b..37331c2 100644
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 3b0b138..ba90235 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -214,15 +214,6 @@ static relopt_int intRelOpts[] =
},
{
{
- "autovacuum_vacuum_cost_delay",
- "Vacuum cost delay in milliseconds, for autovacuum",
- RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
- ShareUpdateExclusiveLock
- },
- -1, 0, 100
- },
- {
- {
"autovacuum_vacuum_cost_limit",
"Vacuum cost amount available before napping, for autovacuum",
RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
@@ -366,6 +357,16 @@ static relopt_real realRelOpts[] =
},
{
{
+ "autovacuum_vacuum_cost_delay",
+ "Vacuum cost delay in milliseconds, for autovacuum",
+ RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+ ShareUpdateExclusiveLock
+ },
+ -1, 0, 100
+ },
+
+ {
+ {
"seq_page_cost",
"Sets the planner's estimate of the cost of a sequentially fetched disk page.",
RELOPT_KIND_TABLESPACE,
Re: [PATCH] get rid of StdRdOptions, use individual binary reloptions representation for each relation kind instead
В письме от понедельник, 18 марта 2019 г. 3:03:04 MSK пользователь Iwata, Aya
написал:
> Hi Nikolay,
Hi!
Sorry for long delay. Postgres is not my primary work, so sometimes it takes a
while to get to it.
> This patch does not apply.
Oh... Sorry... here goes new version
> Please refer to http://commitfest.cputube.org/
> and update it.
Oh! a nice service... Wish it can stream status changes as RSS feeds... But I
can wirte a script on top of it. It would be more easy...
> How about separating your patch by feature or units that
> can be phased commit. For example, adding assert macro at first,
> refactoring StdRdOptions by the next, etc.
No I think we should not. All Macros changes are driven by removing
StdRdOptions. (Actually AssertMarco added there, but I do not think it is a
great addition that require a singe patch)
If it is really necessary I would file AssertMacro addition as a single patch,
but I would abstain from doing it if possible...
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index b58a1f7..db5397f 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -22,7 +22,7 @@
#include "access/htup_details.h"
#include "access/nbtree.h"
#include "access/reloptions.h"
-#include "access/spgist.h"
+#include "access/spgist_private.h"
#include "access/tuptoaster.h"
#include "catalog/pg_type.h"
#include "commands/defrem.h"
@@ -46,9 +46,8 @@
* upper and lower bounds (if applicable); for strings, consider a validation
* routine.
* (ii) add a record below (or use add__reloption).
- * (iii) add it to the appropriate options struct (perhaps StdRdOptions)
- * (iv) add it to the appropriate handling routine (perhaps
- * default_reloptions)
+ * (iii) add it to the appropriate options struct
+ * (iv) add it to the appropriate handling routine
* (v) make sure the lock level is set correctly for that operation
* (vi) don't forget to document the option
*
@@ -1010,7 +1009,7 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
case RELKIND_TOASTVALUE:
case RELKIND_MATVIEW:
case RELKIND_PARTITIONED_TABLE:
- options = heap_reloptions(classForm->relkind, datum, false);
+ options = relation_reloptions(classForm->relkind, datum, false);
break;
case RELKIND_VIEW:
options = view_reloptions(datum, false);
@@ -1343,63 +1342,69 @@ fillRelOptions(void *rdopts, Size basesize,
/*
- * Option parser for anything that uses StdRdOptions.
+ * Option parsing definition for autovacuum. Used in toast and heap options.
+ */
+
+#define AUTOVACUUM_RELOPTIONS(OFFSET)\
+ {"autovacuum_enabled", RELOPT_TYPE_BOOL, \
+ OFFSET + offsetof(AutoVacOpts, enabled)},\
+ {"autovacuum_vacuum_threshold", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_threshold)}, \
+ {"autovacuum_analyze_threshold", RELOPT_TYPE_INT,\
+ OFFSET + offsetof(AutoVacOpts, analyze_threshold)}, \
+ {"autovacuum_vacuum_cost_delay", RELOPT_TYPE_REAL, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_cost_delay)}, \
+ {"autovacuum_vacuum_cost_limit", RELOPT_TYPE_INT,\
+ OFFSET + offsetof(AutoVacOpts, vacuum_cost_limit)}, \
+ {"autovacuum_freeze_min_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_min_age)}, \
+ {"autovacuum_freeze_max_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_max_age)}, \
+ {"autovacuum_freeze_table_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_table_age)}, \
+ {"autovacuum_multixact_freeze_min_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, multixact_freeze_min_age)}, \
+ {"autovacuum_multixact_freeze_max_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, multixact_freeze_max_age)}, \
+ {"autovacuum_multixact_freeze_table_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, multixact_freeze_table_age)}, \
+ {"log_autovacuum_min_duration", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, log_min_duration)}, \
+ {"autovacuum_vacuum_scale_factor", RELOPT_TYPE_REAL, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_scale_factor)},\
+ {"autovacuum_analyze_scale_factor", RELOPT_TYPE_REAL,\
+ OFFSET + offsetof(AutoVacOpts, analyze_scale_factor)}
+
+/*
+ * Option parser for heap
*/
bytea *
-default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
+heap_reloptions(Datum reloptions, bool validate)
{
relopt_value *options;
- StdRdOptions *rdopts;
+ HeapRelOptions *rdopts;
int numoptions;
static const relopt_parse_elt tab[] = {
- {"fillfactor", RELOPT_TYPE_INT, offsetof(StdRdOptions, fillfactor)},
- {"autovacuum_enabled", RELOPT_TYPE_BOOL,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, enabled)},
- {"a
Re: [PATCH] get rid of StdRdOptions, use individual binary reloptions representation for each relation kind instead
В письме от понедельник, 18 марта 2019 г. 17:00:24 MSK пользователь Kyotaro HORIGUCHI написал: > > So I change status to "Waiting for Author". > That seems to be a good oppotunity. I have some comments. > > rel.h: > -#define RelationGetToastTupleTarget(relation, defaulttarg) \ > -((relation)->rd_options ? \ > - ((StdRdOptions *) (relation)->rd_options)->toast_tuple_target : > (defaulttarg)) +#define RelationGetToastTupleTarget(relation, defaulttarg) >\ +(AssertMacro(relation->rd_rel->relkind == > RELKIND_RELATION ||\ + relation->rd_rel->relkind == > RELKIND_MATVIEW),\ + ((relation)->rd_options ? >\ + ((HeapRelOptions *) > (relation)->rd_options)->toast_tuple_target : \ + > (defaulttarg))) > > Index AMs parse reloptions by their handler > functions. HeapRelOptions in this patch are parsed in > relation_reloptions calling heap_reloptions. Maybe at least it > should be called as table_options. But I'm not sure what is the > desirable shape of relation_reloptions for the moment. If we create some TableOptions, or table_options we will create a bigger mess, then we have now. Table == relation, and index is also relation. So better to follow the rule: we have heap relation, toast relation, and all variety of index relations. Each kind of relation have it's own options set, each kind of relation has own marcoses to access it's options. This will make the situation more structured. > -saveFreeSpace = RelationGetTargetPageFreeSpace(relation, > - > HEAP_DEFAULT_FILLFACTOR); +if (IsToastRelation(relation)) > +saveFreeSpace = ToastGetTargetPageFreeSpace(); > +else > +saveFreeSpace = HeapGetTargetPageFreeSpace(relation); > > This locution appears four times in the patch and that's the all > where the two macros appear. And it might not be good that > RelationGetBufferForTuple identifies a heap directly since I > suppose that currently tost is a part of heap. Thus it'd be > better that HeapGetTargetPageFreeSpace handles the toast case. > Similary, it doesn't looks good that RelationGetBufferForTuple > consults HeapGetTargretPageFreeSpace() directly. Perhaps it > should be called via TableGetTargetPageFreeSpace(). I'm not sure > what is the right shape of the relationship among a relation and > a table and other kinds of relation. extract_autovac_opts > penetrates through the modularity boundary of toast/heap in a > similar way. So I started from the idea, I said above: each relation kind has it's own macroses to access it's option. If some options are often used combined, it might be good to create some helper-macros, that will do this combination. But Alvaro is against adding any new macroses (I understand why), and keep old one as intact as possible. So here I can suggest only one thing: keep to the rule: "Each reloption kind has it's own option access macroses" and let the developers of heap-core make their live more comfortable with a helpers function the way they need it. So I would leave this decision to Alvaro... He knows better... > plancat.c: > +if (relation->rd_rel->relkind == RELKIND_RELATION || > +relation->rd_rel->relkind == RELKIND_MATVIEW) > +rel->rel_parallel_workers = RelationGetParallelWorkers(relation, > -1); +else > +rel->rel_parallel_workers = -1; > > rel.h: > #define RelationGetParallelWorkers(relation, defaultpw) \ > (AssertMacro(relation->rd_rel->relkind == RELKIND_RELATION ||\ > relation->rd_rel->relkind == RELKIND_MATVIEW),\ > ((relation)->rd_options ? \ > ((HeapRelOptions *) (relation)->rd_options)->parallel_workers : \ > (defaultpw))) > > These function/macros are doing the same check twice at a call. No. The first check is actual check, that does in runtime in production, and it decides if we need to fetch some options or just use default value. The second check is AssertMacro check. If you look into AssertMacro code you will see that in production builds it do not perform any real check, just an empty function. AssertMacro is needed for developer builds, where it would crash if check is not passed. It is needed to make sure that function is always used in proper context. You should build postgres with --enable-cassert options to get it to work. So there is no double checks here...
Re: [PATCH] get rid of StdRdOptions, use individual binary reloptions representation for each relation kind instead
В письме от среда, 20 марта 2019 г. 6:15:38 MSK пользователь Iwata, Aya написал: > You told us "big picture" about opclass around the beginning of this thread. > In my understanding, the purpose of this refactoring is to make reloptions > more flexible to add opclass. I understand this change may be needed for > opclass, however I think that this is not the best way to refactor > reloption. > > How about discussing more about this specification including opclass, and > finding the best way to refactor reloption? I have not read the previous > thread tightly, so you may have already started preparing. The idea is the following: now there are options that are build in (and actually nailed to) the postgres core. And there are options that can be created in Access Methods in extensions. They share some code, but not all of it. And it is bad. There should be one set of option-related code for both in-core relations and indexes, and for indexes from extensions. If this code is well written it can be used for opclass options as well. So we need to unnail options code from reloptions.c, so no options are nailed to it. So you need to move options definitions (at least for indexes) inside access method code. But we can't do it just that, because some indexes uses StdRdOptions structure for options, it is big, and indexes uses only fillfactor from there... What should we do? Create an individual Options structure for each index. So we did it. And now we have StdRdOptions that is used only for Heap and Toast. And Toast also does not use all of the variables of the structure. Why everywhere we have it's own option structure, but for Heap and Toast it is joined, and in not a very effective way? To successfully apply a patch I plan to commit I need a single option structure for each relation kind. Otherwise I will have to write some hack code around it. I do not want to do so. So it is better to get rid of StdRdOptions completely. This is the only purpose of this patch. Get rid of StdRdOptions and give each relation kind it's own option set. StdRdOptions is ancient stuff. I guess it were added when there was fillfactor only. Now life is more complex. Each relation kind has it's own set of options. Let's not mix them together! PS. If you wand to have some impression of what refactioring I am planning at the end, have a look at the patch https://commitfest.postgresql.org/15/992/ It is old, but you can get an idea.
Re: Ltree syntax improvement
В письме от четверг, 7 марта 2019 г. 13:09:49 MSK пользователь Chris Travers написал: > We maintain an extension (https://github.com/adjust/wltree) > which has a fixed separator (::) and allows any utf-8 character in the tree. > > In our case we currently use our extended tree to store user-defined > hierarchies of labels, which might contain whitespace, Chinese, Japanese, > or Korean characters, etc. > > I would love to be able to deprecate our work on this extension and > eventually use stock ltree. I am afraid, that if would not be possible to have ltree with custom separator. Or we need to pass a very long way to reach there. To have custom separator we will need some place to store it. We can use GUC variable, and set separator for ltree globally. It might solve some of the problems. But will get some more. If for example we dump database, and then restore it on instance where that GUC option is not set, everything will be brocken. It is not opclass option (that are discussed here on the list), because opclass options is no about parsing, but about comparing things that was already parsed. It is not option of an attribute (if we can have custom attribute option), because we can have ltree as a variable, without creating an attribute... It should be an option of ltree type itself. I have no idea how we can implement this. And who will allow us to commit such strange thing ;-)
[PATCH][PROPOSAL] Refuse setting toast.* reloptions when TOAST table does not exist
Hi!
This patch is part of a bigger patch I've offered before
https://www.postgresql.org/message-id/flat/2146419.veIEZdk4E4@x200m#2146419.veIEZdk4E4@x200m
as we agreed I am trying to commit it by smaller bits
This patch raises error if user tries o set or change toast.* option for a
table that does not have a TOST relation.
I believe it is the only right thing to do, as now if you set toast reloption
for table that does not have TOAST table, the value of this option will be
lost without any warning. You will not get it back with pg_dump, it will not
be effective when you add varlen attributes to the table later.
So you offer DB some value to store, it accepts it without errors, and
immediately loses it. I would consider it a bad behavior.
I also think that we should not change this error to warning, as toast.*
options are usually used by very experienced users for precised DB tunning. I
hardly expect them to do TOAST tuning for tables without TOAST relations. So
chances to get problem with existing SQL code are minimal.
So I would suggest to throw an error in this case.
Possible flaws: I tied to write error messages according to guide lines. But I
suppose it is still not prefect enough as I am not so good with English. May
be somebody who knows the language well, can make it better.
--
Do code for fun.diff --git a/src/backend/catalog/toasting.c b/src/backend/catalog/toasting.c
index 0b4b5631..2551023 100644
--- a/src/backend/catalog/toasting.c
+++ b/src/backend/catalog/toasting.c
@@ -36,7 +36,7 @@
/* Potentially set by pg_upgrade_support functions */
Oid binary_upgrade_next_toast_pg_type_oid = InvalidOid;
-static void CheckAndCreateToastTable(Oid relOid, Datum reloptions,
+static bool CheckAndCreateToastTable(Oid relOid, Datum reloptions,
LOCKMODE lockmode, bool check);
static bool create_toast_table(Relation rel, Oid toastOid, Oid toastIndexOid,
Datum reloptions, LOCKMODE lockmode, bool check);
@@ -67,23 +67,26 @@ NewHeapCreateToastTable(Oid relOid, Datum reloptions, LOCKMODE lockmode)
CheckAndCreateToastTable(relOid, reloptions, lockmode, false);
}
-void
+bool
NewRelationCreateToastTable(Oid relOid, Datum reloptions)
{
- CheckAndCreateToastTable(relOid, reloptions, AccessExclusiveLock, false);
+ return CheckAndCreateToastTable(relOid, reloptions,
+ AccessExclusiveLock, false);
}
-static void
+static bool
CheckAndCreateToastTable(Oid relOid, Datum reloptions, LOCKMODE lockmode, bool check)
{
Relation rel;
+ bool success;
rel = heap_open(relOid, lockmode);
/* create_toast_table does all the work */
- (void) create_toast_table(rel, InvalidOid, InvalidOid, reloptions, lockmode, check);
+ success = create_toast_table(rel, InvalidOid, InvalidOid, reloptions, lockmode, check);
heap_close(rel, NoLock);
+ return success;
}
/*
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 3d82edb..7745aa5 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -89,6 +89,7 @@ create_ctas_internal(List *attrList, IntoClause *into)
Datum toast_options;
static char *validnsps[] = HEAP_RELOPT_NAMESPACES;
ObjectAddress intoRelationAddr;
+ bool toast_created;
/* This code supports both CREATE TABLE AS and CREATE MATERIALIZED VIEW */
is_matview = (into->viewQuery != NULL);
@@ -130,7 +131,15 @@ create_ctas_internal(List *attrList, IntoClause *into)
(void) heap_reloptions(RELKIND_TOASTVALUE, toast_options, true);
- NewRelationCreateToastTable(intoRelationAddr.objectId, toast_options);
+ toast_created = NewRelationCreateToastTable(intoRelationAddr.objectId,
+toast_options);
+ if (!toast_created && toast_options)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("can't set TOAST relation options for a new table"),
+ errdetail("TOAST relation were not created"),
+ errhint("do not specify toast.* options, or add some variable length attributes to the table")
+ ));
/* Create the "view" part of a materialized view. */
if (is_matview)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f2a928b..a169e14 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -10557,6 +10557,19 @@ ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation,
heap_close(toastrel, NoLock);
}
+ else
+ {
+ newOptions = transformRelOptions((Datum) 0, defList, "toast",
+ validnsps, false, false);
+ if (newOptions)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+errmsg("can't alter TOAST relation options for \"%s\" table",
+ RelationGetRelationName(rel)),
+ errdetail("TOAST relation does not exist"),
+ errhint("do not specify toast.* options, or add some variable length attributes to the table")
+ ));
+ }
heap_close(pgclass, RowExclusiveLock);
}
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index ec98
Re: [HACKERS] [PATCH] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind for custom AM
В письме от 3 сентября 2017 11:45:43 пользователь Alvaro Herrera написал: > The new checks around toast table creation look like they belong to a > completely independent patch also ... the error message there goes > against message style guidelines also. Offered toast relation checks as a separate patch: https://www.postgresql.org/message-id/3413542.LWpeoxCNq5@x200m -- Do code for fun. signature.asc Description: This is a digitally signed message part.
Re: [PATCH][PROPOSAL] Refuse setting toast.* reloptions when TOAST table does not exist
В письме от 18 января 2018 18:42:01 пользователь Tom Lane написал: > >> This patch raises error if user tries o set or change toast.* option for > >> a > >> table that does not have a TOST relation. > > > > I think there is a problem with this idea, which is that the rules for > > whether or not a given table has an associated TOAST table > > occasionally change from one major release to the next. Suppose that, > > in release X, a particular table definition causes a TOAST table to be > > created, but in release X+1, it does not. If a table with that > > definition has a toast.* option set, then upgrading from release X to > > release X+1 via pg_dump and restore will fail. That's bad. > > Yeah --- and it doesn't even have to be a major version change; the > same version on different hardware might make different choices too. > Not to mention that there is discussion out there about making the > toasting rules more configurable. > > There might be a case for raising a warning in this situation, > but I would disagree with making it a hard error in any case. > All that's going to accomplish is to break people's scripts and > get them mad at you. These all sound very reasonable, but still does not solve problem with loosing toast option values when you set one for table without TOAST relation. May be, if reasons for existence TOAST relation is no so much fixed thing (I thought it is almost as fixed as a rock), may be the solution would be to create a TOAST relation anyway if toast options is set, and gave a warning that may be setting this option is not the thing the user really want? This way we will not loose option values. And it would be 100% backward compatible. The main misfeature here is that we will have empty TOAST relation in this case. But first I do not think it will really harm anybody, and second, we would WARN that it is not the best way to do things, so DB user will be able to find way around. What do you think about it? -- Do code for fun. signature.asc Description: This is a digitally signed message part.
[PATCH][PROPOSAL] Add enum releation option type
Hi!
While working with my big reloption patch,
https://www.postgresql.org/message-id/flat/2146419.veIEZdk4E4@x200m#2146419.veIEZdk4E4@x200m
I found out, that all relation options of string type in current postgres, are
actually behaving as "enum" type. But each time this behavior is implemented
as validate function plus strcmp to compare option value against one of the
possible values.
I think it is not the best practice. It is better to have enum type where it
is technically enum, and keep sting type for further usage (for example for
some kind of index patterns or something like this).
Now strcmp in this case does not really slow down postgres, as both string
options are checked when index is created. One check there will not really
slow down. But if in future somebody would want to have such option checked on
regular basis, it is better to have it as enum type: once "strcmp" it while
parsing, and then just "==" when one need to check option value in runtime.
The patch is in attachment. I hope the code is quite clear.
Possible flaws:
1. I've changed error message from 'Valid values are "XXX", "YYY" and "ZZZ".'
to 'Valid values are "XXX", "YYY", "ZZZ".' to make a code a bit simpler. If it
is not acceptable, please let me know, I will add "and" to the string.
2. Also about the string with the list of acceptable values: the code that
creates this string is inside parse_one_reloption function now. It is a bit
too complex. May be there is already exists some helper function that creates
a string "XXX", "YYY", "ZZZ" from the list of XXX YYY ZZZ values, I do not
know of? Or may be it is reason to create such a function. If so where to
create it? Inside "reloptions.c"? Or it can be reused and I'd better put it
somewhere else?
I hope there would be not further difficulties with this patch. Hope it can be
committed in proper time.
--
Do code for fun.diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 274f7aa..3d12a30 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -404,7 +404,9 @@ static relopt_real realRelOpts[] {{NULL}}
};
-static relopt_string stringRelOpts[] +static const char *gist_buffering_enum_names[] = GIST_OPTION_BUFFERING_VALUE_NAMES;
+static const char *view_check_option_names[] = VIEW_OPTION_CHECK_OPTION_VALUE_NAMES;
+static relopt_enum enumRelOpts[] {
{
{
@@ -413,10 +415,8 @@ static relopt_string stringRelOpts[] RELOPT_KIND_GIST,
AccessExclusiveLock
},
- 4,
- false,
- gistValidateBufferingOption,
- "auto"
+ gist_buffering_enum_names,
+ GIST_OPTION_BUFFERING_AUTO
},
{
{
@@ -425,11 +425,14 @@ static relopt_string stringRelOpts[] RELOPT_KIND_VIEW,
AccessExclusiveLock
},
- 0,
- true,
- validateWithCheckOption,
- NULL
+ view_check_option_names,
+ VIEW_OPTION_CHECK_OPTION_NOT_SET
},
+ {{NULL}}
+};
+
+static relopt_string stringRelOpts[] +{
/* list terminator */
{{NULL}}
};
@@ -476,6 +479,12 @@ initialize_reloptions(void)
realRelOpts[i].gen.lockmode));
j++;
}
+ for (i = 0; enumRelOpts[i].gen.name; i++)
+ {
+ Assert(DoLockModesConflict(enumRelOpts[i].gen.lockmode,
+ enumRelOpts[i].gen.lockmode));
+ j++;
+ }
for (i = 0; stringRelOpts[i].gen.name; i++)
{
Assert(DoLockModesConflict(stringRelOpts[i].gen.lockmode,
@@ -514,6 +523,14 @@ initialize_reloptions(void)
j++;
}
+ for (i = 0; enumRelOpts[i].gen.name; i++)
+ {
+ relOpts[j] = &enumRelOpts[i].gen;
+ relOpts[j]->type = RELOPT_TYPE_ENUM;
+ relOpts[j]->namelen = strlen(relOpts[j]->name);
+ j++;
+ }
+
for (i = 0; stringRelOpts[i].gen.name; i++)
{
relOpts[j] = &stringRelOpts[i].gen;
@@ -611,6 +628,9 @@ allocate_reloption(bits32 kinds, int type, const char *name, const char *desc)
case RELOPT_TYPE_REAL:
size = sizeof(relopt_real);
break;
+ case RELOPT_TYPE_ENUM:
+ size = sizeof(relopt_enum);
+ break;
case RELOPT_TYPE_STRING:
size = sizeof(relopt_string);
break;
@@ -690,6 +710,24 @@ add_real_reloption(bits32 kinds, const char *name, const char *desc, double defa
}
/*
+ * add_enuum_reloption
+ * Add a new enum reloption
+ */
+void
+add_enum_reloption(bits32 kinds, const char *name, const char *desc,
+ const char **allowed_values, int default_val)
+{
+ relopt_enum *newoption;
+
+ newoption = (relopt_enum *) allocate_reloption(kinds, RELOPT_TYPE_ENUM,
+ name, desc);
+ newoption->allowed_values = allowed_values;
+ newoption->default_val = default_val;
+
+ add_reloption((relopt_gen *) newoption);
+}
+
+/*
* add_string_reloption
* Add a new string reloption
*
@@ -1192,6 +1230,78 @@ parse_one_reloption(relopt_value *option, char *text_str, int text_len,
optreal->min, optreal->max)));
}
break;
+ case RELOPT_TYPE_ENUM:
+ {
+relopt_enum *opt_enum = (relopt_enum *) option->gen;
+int i = 0;
+
+parsed = false;
+while (opt_enum->allowed_values[i])
+{
+ if (pg_
Re: [HACKERS] [PATCH] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind for custom AM
В письме от 3 сентября 2017 11:45:43 пользователь Alvaro Herrera написал: > I think we should split this in at least two commits, Added another part for commit: https://www.postgresql.org/message-id/43332102.S2V5pIjXRx@x200m This one adds an enum relation option type. -- Do code for fun. signature.asc Description: This is a digitally signed message part.
[PATCH] Minor fixes for reloptions tests
I have a small fixes for already committed reloption test patch (https://www.postgresql.org/message-id/2615372.orqtEn8VGB@x200m) First one is missing tab symbol where it should be. The second is a comment. "The OIDS option is not stored" is not quite correct. They are stored, but not as reloption. I've added this notion there... -- Do code for fun.diff --git a/src/test/regress/expected/reloptions.out b/src/test/regress/expected/reloptions.out index c4107d5..df3c99d 100644 --- a/src/test/regress/expected/reloptions.out +++ b/src/test/regress/expected/reloptions.out @@ -77,7 +77,7 @@ SELECT reloptions FROM pg_class WHERE oid = 'reloptions_test'::regclass; ALTER TABLE reloptions_test RESET (autovacuum_enabled, autovacuum_analyze_scale_factor); SELECT reloptions FROM pg_class WHERE oid = 'reloptions_test'::regclass AND -reloptions IS NULL; + reloptions IS NULL; reloptions @@ -86,7 +86,7 @@ reloptions IS NULL; -- RESET fails if a value is specified ALTER TABLE reloptions_test RESET (fillfactor); ERROR: RESET must not include values for parameters --- The OIDS option is not stored +-- The OIDS option is not stored as reloption DROP TABLE reloptions_test; CREATE TABLE reloptions_test(i INT) WITH (fillfactor , oids=true); SELECT reloptions, relhasoids FROM pg_class WHERE oid = 'reloptions_test'::regclass; diff --git a/src/test/regress/sql/reloptions.sql b/src/test/regress/sql/reloptions.sql index c9119fd..37fbf41 100644 --- a/src/test/regress/sql/reloptions.sql +++ b/src/test/regress/sql/reloptions.sql @@ -47,12 +47,12 @@ SELECT reloptions FROM pg_class WHERE oid = 'reloptions_test'::regclass; ALTER TABLE reloptions_test RESET (autovacuum_enabled, autovacuum_analyze_scale_factor); SELECT reloptions FROM pg_class WHERE oid = 'reloptions_test'::regclass AND -reloptions IS NULL; + reloptions IS NULL; -- RESET fails if a value is specified ALTER TABLE reloptions_test RESET (fillfactor); --- The OIDS option is not stored +-- The OIDS option is not stored as reloption DROP TABLE reloptions_test; CREATE TABLE reloptions_test(i INT) WITH (fillfactor , oids=true); SELECT reloptions, relhasoids FROM pg_class WHERE oid = 'reloptions_test'::regclass; signature.asc Description: This is a digitally signed message part.
Re: [HACKERS] [PATCH] Tests for reloptions
В письме от 29 октября 2017 17:08:29 пользователь Nikolay Shaplov написал: > While merging this commit to my branch, I found two issues that as I think > needs fixing. Hope this does not require creating new commit request... To draw more attention to this issue, I've filed it as a new thread https://www.postgresql.org/message-id/3951112.MkJ1MAZpIQ%40x200m with a entry for next commit fest. Hope this will help fixing missing tab. -- Do code for fun. signature.asc Description: This is a digitally signed message part.
Re: [PATCH][PROPOSAL] Refuse setting toast.* reloptions when TOAST table does not exist
В письме от 23 января 2018 12:03:50 пользователь Peter Eisentraut написал: > >>> This patch raises error if user tries o set or change toast.* option for > >>> a table that does not have a TOST relation. > > > > There might be a case for raising a warning in this situation, > > but I would disagree with making it a hard error in any case. > > All that's going to accomplish is to break people's scripts and > > get them mad at you. > > It might also be useful to set these reloptions before you add a new > column to a table. As Robert have just said, this will not work with current master. If we, as I've suggested in previous letter, will force TOAST creation if toast.* option is set, then your use case will also work. -- Do code for fun. signature.asc Description: This is a digitally signed message part.
Re: [PATCH][PROPOSAL] Refuse setting toast.* reloptions when TOAST table does not exist
В письме от 25 января 2018 11:29:34 пользователь Tom Lane написал: > Alvaro Herrera writes: > > Tom Lane wrote: > >> Well, maybe the right answer is to address that. It's clear to me > >> why that would happen if we store these things as reloptions on the > >> toast table, but can't they be stored on the parent table? > > > > Actually, Nikolay provided a possible solution: if you execute ALTER > > TABLE SET (toast.foobar = xyz), and a toast table doesn't exist, create > > one at that point. > > That adds a lot of overhead if you never actually need the toast table. I think this overhead case is not that terrible if it is properly warned ;-) > Still, maybe it's an appropriate amount of effort compared to the size > of the use-case for this. If you came to some final conclustion, please close the commiffest item with "Return with feedback" resolution, and I write another patch... -- Do code for fun. signature.asc Description: This is a digitally signed message part.
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от 9 февраля 2018 18:45:29 пользователь Alvaro Herrera написал: > If this patch gets in, I wonder if there are any external modules that > use actual strings. An hypothetical example would be something like a > SSL cipher list; it needs to be somewhat free-form that an enum would > not cut it. If there are such modules, then even if we remove all > existing in-core use cases we should keep the support code for strings. I did not remove string option support from the code. It might be needed later. > Maybe we need some in-core user to verify the string case still works. > A new module in src/test/modules perhaps? This sound as a good idea. I am too do not feel really comfortable with that this string options possibility exists, but is not tested. I'll have a look there, it it will not require a great job, I will add tests for string options there. > On the other hand, if we can > find no use for these string reloptions, maybe we should just remove the > support, since as I recall it's messy enough. No, the implementation of string options is quite good. And may be needed later. > > Possible flaws: > > > > 1. I've changed error message from 'Valid values are "XXX", "YYY" and > > "ZZZ".' to 'Valid values are "XXX", "YYY", "ZZZ".' to make a code a bit > > simpler. If it is not acceptable, please let me know, I will add "and" to > > the string. > I don't think we care about this, but is this still the case if you use > a stringinfo? May be not. I'll try to do it better. > > 2. Also about the string with the list of acceptable values: the code that > > creates this string is inside parse_one_reloption function now. > > I think you could save most of that mess by using appendStringInfo and > friends. Thanks. I will rewrite this part using these functions. That was really helpful. > I don't much like the way you've represented the list of possible values > for each enum. I think it'd be better to have a struct that represents > everything about each value (string name and C symbol. I actually do not like it this way too. I would prefer one structure, not two lists. But I did not find way how to do it in one struct. How to gave have string value and C symbol in one structure, without defining C symbols elsewhere. Otherwise it will be two lists again. I do not have a lot of hardcore C development experience, so I can miss something. Can you provide an example of the structure you are talking about? > Maybe the > numerical value too if that's needed, but is it? I suppose all code > should use the C symbol only, so why do we care about the numerical > value?). It is comfortable to have numerical values when debugging. I like to write something like elog(WARNING,"buffering_mode=%i",opt.buffering_mode); to check that everything works as expected. Such cases is the only reason to keep numerical value. -- Do code for fun. signature.asc Description: This is a digitally signed message part.
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от 9 февраля 2018 18:45:29 пользователь Alvaro Herrera написал:
> > 1. I've changed error message from 'Valid values are "XXX", "YYY" and
> > "ZZZ".' to 'Valid values are "XXX", "YYY", "ZZZ".' to make a code a bit
> > simpler. If it is not acceptable, please let me know, I will add "and" to
> > the string.
> I don't think we care about this, but is this still the case if you use
> a stringinfo?
>
> > 2. Also about the string with the list of acceptable values: the code that
> > creates this string is inside parse_one_reloption function now.
>
> I think you could save most of that mess by using appendStringInfo and
> friends.
Here is the second verion of the patch where I use appendStringInfo to prepare
error message. The code is much more simple here now, and it now create value
list with "and" at the end: '"xxx", "yyy" and "zzz"'
--
Do code for fun.diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 46276ce..82a0492 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -404,7 +404,9 @@ static relopt_real realRelOpts[] {{NULL}}
};
-static relopt_string stringRelOpts[] +static const char *gist_buffering_enum_names[] = GIST_OPTION_BUFFERING_VALUE_NAMES;
+static const char *view_check_option_names[] = VIEW_OPTION_CHECK_OPTION_VALUE_NAMES;
+static relopt_enum enumRelOpts[] {
{
{
@@ -413,10 +415,8 @@ static relopt_string stringRelOpts[] RELOPT_KIND_GIST,
AccessExclusiveLock
},
- 4,
- false,
- gistValidateBufferingOption,
- "auto"
+ gist_buffering_enum_names,
+ GIST_OPTION_BUFFERING_AUTO
},
{
{
@@ -425,11 +425,14 @@ static relopt_string stringRelOpts[] RELOPT_KIND_VIEW,
AccessExclusiveLock
},
- 0,
- true,
- validateWithCheckOption,
- NULL
+ view_check_option_names,
+ VIEW_OPTION_CHECK_OPTION_NOT_SET
},
+ {{NULL}}
+};
+
+static relopt_string stringRelOpts[] +{
/* list terminator */
{{NULL}}
};
@@ -476,6 +479,12 @@ initialize_reloptions(void)
realRelOpts[i].gen.lockmode));
j++;
}
+ for (i = 0; enumRelOpts[i].gen.name; i++)
+ {
+ Assert(DoLockModesConflict(enumRelOpts[i].gen.lockmode,
+ enumRelOpts[i].gen.lockmode));
+ j++;
+ }
for (i = 0; stringRelOpts[i].gen.name; i++)
{
Assert(DoLockModesConflict(stringRelOpts[i].gen.lockmode,
@@ -514,6 +523,14 @@ initialize_reloptions(void)
j++;
}
+ for (i = 0; enumRelOpts[i].gen.name; i++)
+ {
+ relOpts[j] = &enumRelOpts[i].gen;
+ relOpts[j]->type = RELOPT_TYPE_ENUM;
+ relOpts[j]->namelen = strlen(relOpts[j]->name);
+ j++;
+ }
+
for (i = 0; stringRelOpts[i].gen.name; i++)
{
relOpts[j] = &stringRelOpts[i].gen;
@@ -611,6 +628,9 @@ allocate_reloption(bits32 kinds, int type, const char *name, const char *desc)
case RELOPT_TYPE_REAL:
size = sizeof(relopt_real);
break;
+ case RELOPT_TYPE_ENUM:
+ size = sizeof(relopt_enum);
+ break;
case RELOPT_TYPE_STRING:
size = sizeof(relopt_string);
break;
@@ -690,6 +710,24 @@ add_real_reloption(bits32 kinds, const char *name, const char *desc, double defa
}
/*
+ * add_enuum_reloption
+ * Add a new enum reloption
+ */
+void
+add_enum_reloption(bits32 kinds, const char *name, const char *desc,
+ const char **allowed_values, int default_val)
+{
+ relopt_enum *newoption;
+
+ newoption = (relopt_enum *) allocate_reloption(kinds, RELOPT_TYPE_ENUM,
+ name, desc);
+ newoption->allowed_values = allowed_values;
+ newoption->default_val = default_val;
+
+ add_reloption((relopt_gen *) newoption);
+}
+
+/*
* add_string_reloption
* Add a new string reloption
*
@@ -1190,6 +1228,50 @@ parse_one_reloption(relopt_value *option, char *text_str, int text_len,
optreal->min, optreal->max)));
}
break;
+ case RELOPT_TYPE_ENUM:
+ {
+relopt_enum *opt_enum = (relopt_enum *) option->gen;
+int i = 0;
+
+parsed = false;
+while (opt_enum->allowed_values[i])
+{
+ if (pg_strcasecmp(value, opt_enum->allowed_values[i]) == 0)
+ {
+ option->values.enum_val = i;
+ parsed = true;
+ break;
+ }
+ i++;
+}
+if (!parsed)
+{
+ StringInfoData buf;
+
+ initStringInfo(&buf);
+ i = 0;
+ while (opt_enum->allowed_values[i])
+ {
+ appendStringInfo(&buf,"\"%s\"",
+ opt_enum->allowed_values[i]);
+ if (opt_enum->allowed_values[i + 1])
+ {
+ if (opt_enum->allowed_values[i + 2])
+appendStringInfo(&buf,", ");
+ else
+appendStringInfo(&buf," and ");
+ }
+ i++;
+ }
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid value for \"%s\" option",
+ option->gen->name),
+ errdetail("Valid values are %s.", /*str*/ buf.data)));
+ pfree(buf.data);
+}
+ }
+ break;
case RELOPT_TYPE_STRING:
{
relopt_string *optstring = (relopt_string *) option->gen;
@@ -1283,6 +1365,11 @@ fillRelOptions(void *
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от 9 февраля 2018 18:45:29 пользователь Alvaro Herrera написал: > Maybe we need some in-core user to verify the string case still works. > A new module in src/test/modules perhaps? I've looked attentively in src/test/modules... To properly test all reloptions hooks for modules wee need to create an extension with some dummy index in it. This way we can properly test everything. Creating dummy index would be fun, but it a quite big deal to create it, so it will require a separate patch to deal with. So I suppose string reloptions is better to leave untested for now, with a notion, that it should be done sooner or later > I don't much like the way you've represented the list of possible values > for each enum. I think it'd be better to have a struct that represents > everything about each value (string name and C symbol. Maybe the > numerical value too if that's needed, but is it? I suppose all code > should use the C symbol only, so why do we care about the numerical > value?). One more reason to keep numeric value, that came to my mind, is that it seems to be logical to keep enum value in postgres internals represented as C-enum. And C-enum is actually an int value (can be easily casted both ways). It is not strictly necessary, but it seems to be a bit logical... -- Do code for fun. signature.asc Description: This is a digitally signed message part.
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от 15 февраля 2018 12:53:27 пользователь Alvaro Herrera написал: > > > Maybe we need some in-core user to verify the string case still works. > > > A new module in src/test/modules perhaps? > > > > I've looked attentively in src/test/modules... To properly test all > > reloptions hooks for modules wee need to create an extension with some > > dummy index in it. This way we can properly test everything. Creating > > dummy index would be fun, but it a quite big deal to create it, so it > > will require a separate patch to deal with. So I suppose string > > reloptions is better to leave untested for now, with a notion, that it > > should be done sooner or later > > Do we really need a dummy index? I was thinking in something that just > calls a few C functions to create and parse a string reloption should be > more than enough. Technically we can add_reloption_kind(); then add some string options to it using add_string_reloption, and then call fillRelOptions with some dummy data and validate argument on and see what will happen. But I do not think it will test a lot. I think it is better to test it altogether, not just some part of it. Moreover when my whole patch is commited these all should be rewritten, as I changed some options internals for some good reasons. So I would prefer to keep it untested while we are done with reloptions, and then test it in a good way, with creating dummy index and so on. I think it will be needed for more tests and educational purposes... But if you will insist on it as a reviewer, I will do as you say. > > > I don't much like the way you've represented the list of possible values > > > for each enum. I think it'd be better to have a struct that represents > > > everything about each value (string name and C symbol. Maybe the > > > numerical value too if that's needed, but is it? I suppose all code > > > should use the C symbol only, so why do we care about the numerical > > > value?). > > > > One more reason to keep numeric value, that came to my mind, is that it > > seems to be logical to keep enum value in postgres internals represented > > as C-enum. And C-enum is actually an int value (can be easily casted both > > ways). It is not strictly necessary, but it seems to be a bit logical... > > Oh, I didn't mean to steer you away from a C enum. I just meant that we > don't need to define the numerical values ourselves -- it should be fine > to use whatever the C compiler chooses for each C symbol (enum member > name). In the code we don't refer to the values by numerical value, > only by the C symbol. Ah that is what you are talking about :-) I needed this numbers for debug purposes, nothing more. If it is not good to keep them, they can be removed now... (I would prefer to keep them for further debugging, but if it is not right, I can easily remove them, I do not need them right now) -- Do code for fun. signature.asc Description: This is a digitally signed message part.
Re: [PATCH] Add some useful asserts into View Options macroses
В письме от пятница, 1 ноября 2019 г. 13:29:58 MSK пользователь Peter Eisentraut написал: > Committed. > > I simplified the parentheses by one level from your patch. Thank you! -- Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)
Re: [PATCH] use separate PartitionedRelOptions structure to store partitioned table options
В письме от среда, 23 октября 2019 г. 11:59:45 MSK пользователь Amit Langote
написал:
> Sorry for the late reply.
Same apologies from my side. Get decent time-slot for postgres dev only now.
> I looked atthe v2 patch and noticed a typo:
>
> + * Binary representation of relation options for rtitioned tables.
>
> s/rtitioned/partitioned/g
>
> Other than that, looks good.
Here goes v3 patch with the typo fixed
--
Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da
Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index d8790ad..d30f3db 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -1099,9 +1099,11 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
case RELKIND_RELATION:
case RELKIND_TOASTVALUE:
case RELKIND_MATVIEW:
- case RELKIND_PARTITIONED_TABLE:
options = heap_reloptions(classForm->relkind, datum, false);
break;
+ case RELKIND_PARTITIONED_TABLE:
+ options = partitioned_table_reloptions(datum, false);
+ break;
case RELKIND_VIEW:
options = view_reloptions(datum, false);
break;
@@ -1572,6 +1574,25 @@ build_reloptions(Datum reloptions, bool validate,
}
/*
+ * Option parser for partitioned tables
+ */
+bytea *
+partitioned_table_reloptions(Datum reloptions, bool validate)
+{
+ int numoptions;
+
+ /*
+ * Since there are no options for patitioned tables for now, we just do
+ * validation to report incorrect option error and leave.
+ */
+
+ if (validate)
+ parseRelOptions(reloptions, validate, RELOPT_KIND_PARTITIONED,
+ &numoptions);
+ return NULL;
+}
+
+/*
* Option parser for views
*/
bytea *
@@ -1614,9 +1635,6 @@ heap_reloptions(char relkind, Datum reloptions, bool validate)
case RELKIND_RELATION:
case RELKIND_MATVIEW:
return default_reloptions(reloptions, validate, RELOPT_KIND_HEAP);
- case RELKIND_PARTITIONED_TABLE:
- return default_reloptions(reloptions, validate,
- RELOPT_KIND_PARTITIONED);
default:
/* other relkinds are not supported */
return NULL;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 5597be6..424654d 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -721,10 +721,17 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
reloptions = transformRelOptions((Datum) 0, stmt->options, NULL, validnsps,
true, false);
- if (relkind == RELKIND_VIEW)
- (void) view_reloptions(reloptions, true);
- else
- (void) heap_reloptions(relkind, reloptions, true);
+ switch (relkind)
+ {
+ case RELKIND_VIEW:
+ (void) view_reloptions(reloptions, true);
+ break;
+ case RELKIND_PARTITIONED_TABLE:
+ (void) partitioned_table_reloptions(reloptions, true);
+ break;
+ default:
+ (void) heap_reloptions(relkind, reloptions, true);
+ }
if (stmt->ofTypename)
{
@@ -12189,9 +12196,11 @@ ATExecSetRelOptions(Relation rel, List *defList, AlterTableType operation,
case RELKIND_RELATION:
case RELKIND_TOASTVALUE:
case RELKIND_MATVIEW:
- case RELKIND_PARTITIONED_TABLE:
(void) heap_reloptions(rel->rd_rel->relkind, newOptions, true);
break;
+ case RELKIND_PARTITIONED_TABLE:
+ (void) partitioned_table_reloptions(newOptions, true);
+ break;
case RELKIND_VIEW:
(void) view_reloptions(newOptions, true);
break;
diff --git a/src/include/access/reloptions.h b/src/include/access/reloptions.h
index db42aa3..ee7248a 100644
--- a/src/include/access/reloptions.h
+++ b/src/include/access/reloptions.h
@@ -306,6 +306,7 @@ extern bytea *default_reloptions(Datum reloptions, bool validate,
relopt_kind kind);
extern bytea *heap_reloptions(char relkind, Datum reloptions, bool validate);
extern bytea *view_reloptions(Datum reloptions, bool validate);
+extern bytea *partitioned_table_reloptions(Datum reloptions, bool validate);
extern bytea *index_reloptions(amoptions_function amoptions, Datum reloptions,
bool validate);
extern bytea *attribute_reloptions(Datum reloptions, bool validate);
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index 8b8b237..1eb323a 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -278,6 +278,16 @@ typedef struct StdRdOptions
#define HEAP_DEFAULT_FILLFACTOR 100
/*
+ * PartitionedRelOptions
+ * Binary representation of relation options for partitioned tables.
+ */
+typedef struct PartitionedRelOptions
+{
+ int32 vl_len_;/* varlena header (do not touch directly!) */
+ /* No options defined yet */
+} PartitionedRelOptions;
+
+/*
* RelationGetToastTupleTarget
* Returns the relation's toast_tuple_target. Note multiple eval of argument!
*/
Re: [PATCH] Do not use StdRdOptions in Access Methods
В письме от среда, 13 ноября 2019 г. 16:05:20 MSK пользователь Michael Paquier
написал:
Guys! Sorry for being away for so long. I did not have much opportunities to
pay my attention to postgres recently.
Thank you for introducing build_reloptions function. It is approximately the
direction I wanted to move afterwards myself.
But nevertheless, I am steady on my way, and I want to get rid of StdRdOptions
before doing anything else myself. This structure is long outdated and is not
suitable for access method's options at all.
I've changed the patch to use build_reloptions function and again propose it
to the commitfest.
--
Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da
Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index d8790ad..f40d68c 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -23,7 +23,7 @@
#include "access/htup_details.h"
#include "access/nbtree.h"
#include "access/reloptions.h"
-#include "access/spgist.h"
+#include "access/spgist_private.h"
#include "catalog/pg_type.h"
#include "commands/defrem.h"
#include "commands/tablespace.h"
@@ -1510,8 +1510,6 @@ default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
offsetof(StdRdOptions, user_catalog_table)},
{"parallel_workers", RELOPT_TYPE_INT,
offsetof(StdRdOptions, parallel_workers)},
- {"vacuum_cleanup_index_scale_factor", RELOPT_TYPE_REAL,
- offsetof(StdRdOptions, vacuum_cleanup_index_scale_factor)},
{"vacuum_index_cleanup", RELOPT_TYPE_BOOL,
offsetof(StdRdOptions, vacuum_index_cleanup)},
{"vacuum_truncate", RELOPT_TYPE_BOOL,
diff --git a/src/backend/access/hash/hashpage.c b/src/backend/access/hash/hashpage.c
index 63697bf..0cc4cb6 100644
--- a/src/backend/access/hash/hashpage.c
+++ b/src/backend/access/hash/hashpage.c
@@ -358,7 +358,7 @@ _hash_init(Relation rel, double num_tuples, ForkNumber forkNum)
data_width = sizeof(uint32);
item_width = MAXALIGN(sizeof(IndexTupleData)) + MAXALIGN(data_width) +
sizeof(ItemIdData); /* include the line pointer */
- ffactor = RelationGetTargetPageUsage(rel, HASH_DEFAULT_FILLFACTOR) / item_width;
+ ffactor = (BLCKSZ * HashGetFillFactor(rel) / 100) / item_width;
/* keep to a sane range */
if (ffactor < 10)
ffactor = 10;
diff --git a/src/backend/access/hash/hashutil.c b/src/backend/access/hash/hashutil.c
index c5005f4..669dccc 100644
--- a/src/backend/access/hash/hashutil.c
+++ b/src/backend/access/hash/hashutil.c
@@ -289,7 +289,14 @@ _hash_checkpage(Relation rel, Buffer buf, int flags)
bytea *
hashoptions(Datum reloptions, bool validate)
{
- return default_reloptions(reloptions, validate, RELOPT_KIND_HASH);
+ static const relopt_parse_elt tab[] = {
+ {"fillfactor", RELOPT_TYPE_INT, offsetof(HashOptions, fillfactor)},
+ };
+
+ return (bytea *) build_reloptions(reloptions, validate,
+ RELOPT_KIND_HASH,
+ sizeof(HashOptions),
+ tab, lengthof(tab));
}
/*
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 4cfd528..8352882 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -816,7 +816,7 @@ _bt_vacuum_needs_cleanup(IndexVacuumInfo *info)
}
else
{
- StdRdOptions *relopts;
+ BTOptions *relopts;
float8 cleanup_scale_factor;
float8 prev_num_heap_tuples;
@@ -827,7 +827,7 @@ _bt_vacuum_needs_cleanup(IndexVacuumInfo *info)
* tuples exceeds vacuum_cleanup_index_scale_factor fraction of
* original tuples count.
*/
- relopts = (StdRdOptions *) info->index->rd_options;
+ relopts = (BTOptions *) info->index->rd_options;
cleanup_scale_factor = (relopts &&
relopts->vacuum_cleanup_index_scale_factor >= 0)
? relopts->vacuum_cleanup_index_scale_factor
diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c
index c11a3fb..9906c80 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -716,8 +716,9 @@ _bt_pagestate(BTWriteState *wstate, uint32 level)
if (level > 0)
state->btps_full = (BLCKSZ * (100 - BTREE_NONLEAF_FILLFACTOR) / 100);
else
- state->btps_full = RelationGetTargetPageFreeSpace(wstate->index,
- BTREE_DEFAULT_FILLFACTOR);
+ state->btps_full = (BLCKSZ * (100 - BTGetFillFactor(wstate->index))
+ / 100);
+
/* no parent level, yet */
state->btps_next = NULL;
diff --git a/src/backend/access/nbtree/nbtsplitloc.c b/src/backend/access/nbtree/nbtsplitloc.c
index a04d4e2..29167f1 100644
--- a/src/backend/access/nbtree/nbtsplitloc.c
+++ b/src/backend/access/nbtree/nbtsplitloc.c
@@ -167,7 +167,7 @@ _bt_findsplitloc(Relation rel,
/* Count up total space in data items before actually scanning 'em */
olddataitemstotal = rightspace - (int) PageGetExactFreeSpace(page);
- leaffillfactor = RelationGetFillFactor(rel,
Re: [PATCH] use separate PartitionedRelOptions structure to store partitioned table options
В письме от среда, 13 ноября 2019 г. 16:30:29 MSK пользователь Michael Paquier написал: > On Tue, Nov 12, 2019 at 01:50:03PM +0900, Michael Paquier wrote: > > We have been through great length to have build_reloptions, so > > wouldn't it be better to also have this code path do so? Sure you > > need to pass NULL for the parsing table.. But there is a point to > > reduce the code paths using directly parseRelOptions() and the > > follow-up, expected calls to allocate and to fill in the set of > > reloptions. > > So I have been looking at this one, and finished with the attached. > It looks much better to use build_reloptions() IMO, taking advantage > of the same sanity checks present for the other relkinds. Thanks! I did not read that thread yet, when I sent v3 patch. build_reloptions is a good stuff and we should use it for sure. I've looked at yours v4 version of the patch, it is exactly what we need here. Can we commit it as it is? -- Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)
Re: [PATCH] Do not use StdRdOptions in Access Methods
В письме от четверг, 14 ноября 2019 г. 16:50:18 MSK пользователь Michael Paquier написал: > > I've changed the patch to use build_reloptions function and again propose > > it to the commitfest. > > Thanks for the new patch. I have not reviewed the patch in details, > but I have a small comment. > > > +#define SpGistGetFillFactor(relation) \ > > + ((relation)->rd_options ? \ > > + ((SpGistOptions *) (relation)->rd_options)->fillfactor : \ > > + SPGIST_DEFAULT_FILLFACTOR) > > + > > Wouldn't it make sense to add assertions here to make sure that the > relkind is an index? You basically did that in commit 3967737. For me there is no mush sense in it, as it does not prevent us from wrong type casting. Indexes can use all kinds of structures for reloptions, and checking that this is index, will not do much. Do you know way how to distinguish one index from another? If we can check in assertion this is index, and this index is spgist, then assertion will make sense for 100%. I just have no idea how to do it. As far as I can see it is not possible now. Another issue here is, that we should to it to all indexes, not only that have been using StdRdOptions, but to all indexes we have. (Damn code inconsistency). So I guess this should go as another patch to keep it step by step improvements. -- Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)
Re: [PATCH] Do not use StdRdOptions in Access Methods
В письме от среда, 20 ноября 2019 г. 16:44:18 MSK пользователь Michael Paquier написал: > > It seems to me that if the plan is to have one option structure for > > each index AM, which has actually the advantage to reduce the bloat of > > each relcache entry currently relying on StdRdOptions, then we could > > have those extra assertion checks in the same patch, because the new > > macros are introduced. > I have looked at this patch, and did not like much having the > calculations of the page free space around, so I have moved that into > each AM's dedicated header. Sounds as a good idea. I try not touch such things following the rule "is not broken do not fix" but this way it is definitely better. Thanks! > > There is rd_rel->relam. You can for example refer to pgstatindex.c > > which has AM-related checks to make sure that the correct index AM is > > being used. > > We can do something similar for GIN and BRIN on top of the rest, so > updated the patch with that. That is what I've been trying to tell speaking about code consistency. But ok, this way is also good. > Nikolay, I would be fine to commit this patch as-is. Yeah. I've read the patch. I like it, actually I started doing same thing myself but you were faster. I have opportunity to pay attention to postgres once a week these days... I like the patch, and also agree that it should be commited as is. Though I have a notion to think about. BRIN_AM_OID and friends are defined in catalog/pg_am_d.h so for core indexes we can do relation->rd_rel->relam == BRIN_AM_OID check. But for contrib indexes we can't do such a thing. Bloom index does not need such check as it uses options only when index is created. At that point you can not choose wrong relation. But if in future we will have some contrib index that uses options when it some data is inserted (as it is done with fillfactor in core indexes) then index author will not be able to do such relam check. I would not call it a big problem, but it is something to think about, for sure... > Thanks for your patience on this stuff. Thaks for joining this work, and sorry for late replies. Now I quite rarely have time for postgres :-( -- Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)
[PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions
In the thread
https://www.postgresql.org/message-id/2620882.s52SJui4ql@x200m
I've suggested to split one big StdRdOption that is used for options storage
into into Options structures individual for each relkind and each relam
That patch have been split into smaller parts, most of them were already
commit:
https://commitfest.postgresql.org/25/2294/ - Remove StdRdOptions from AM
https://commitfest.postgresql.org/25/2297/ - Do not use StdRdOption for
partitioned tables
https://commitfest.postgresql.org/25/2295/ - Some useful Asserts for view-
related macroses.
And here goes the last part of StrRdOptions removal patch, where StdRdOptions
is replaced with HeapOptions and ToastOptions.
What did I do here.
- Added HeapOptions and ToastOptions structues
- Moved options building tab for autovacuum into AUTOVACUUM_RELOPTIONS macro,
so it can be used in relopt_parse_elt tab both for heap and toast
- Changed everywhere in the code, where old heap_reloptions is used, to use
new heap_reloptions or toast_reloptions
- Changed heap & toast option fetching macros to use HeapOptions and
ToastOptions
- Added Asserts to heap and toast options macros. Now we finally can do it.
What I did not do
- I've split fillfactor related macros into heap and toast like
RelationGetFillFactor will become HeapGetFillFactor and ToastGetFillFactor. I
have to do it, because now they handle different structure.
but there are heap only option macros like RelationGetParallelWorkers that
should be better called HeapGetParallelWorkers, as it is heap related. But I
did not change the name, as somebody from core team (I think it was Alvaro, it
was a while ago) asked me not to change macros names unless in is inavoidable.
So I kept the names, though I still think that naming them with Heap prefix
will make code more clear.
- vacuum_index_cleanup and vacuum_truncate options were added recently. They
were added into StdRdOptions. I think their place is inside AutoVacOpts not in
StdRdOptions, but did not dare to change it. If you see it the same way as I
see, please let me know, I will move it to a proper place.
--
Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da
Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 48377ac..8b18381 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -46,9 +46,8 @@
* upper and lower bounds (if applicable); for strings, consider a validation
* routine.
* (ii) add a record below (or use add__reloption).
- * (iii) add it to the appropriate options struct (perhaps StdRdOptions)
- * (iv) add it to the appropriate handling routine (perhaps
- * default_reloptions)
+ * (iii) add it to the appropriate options struct
+ * (iv) add it to the appropriate handling routine
* (v) make sure the lock level is set correctly for that operation
* (vi) don't forget to document the option
*
@@ -1106,9 +1105,11 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
switch (classForm->relkind)
{
case RELKIND_RELATION:
- case RELKIND_TOASTVALUE:
case RELKIND_MATVIEW:
- options = heap_reloptions(classForm->relkind, datum, false);
+ options = heap_reloptions(datum, false);
+ break;
+ case RELKIND_TOASTVALUE:
+ options = toast_reloptions(datum, false);
break;
case RELKIND_PARTITIONED_TABLE:
options = partitioned_table_reloptions(datum, false);
@@ -1480,55 +1481,62 @@ fillRelOptions(void *rdopts, Size basesize,
/*
- * Option parser for anything that uses StdRdOptions.
+ * Option parsing definition for autovacuum. Used in toast and heap options.
+ */
+
+#define AUTOVACUUM_RELOPTIONS(OFFSET)\
+ {"autovacuum_enabled", RELOPT_TYPE_BOOL, \
+ OFFSET + offsetof(AutoVacOpts, enabled)},\
+ {"autovacuum_vacuum_threshold", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_threshold)}, \
+ {"autovacuum_analyze_threshold", RELOPT_TYPE_INT,\
+ OFFSET + offsetof(AutoVacOpts, analyze_threshold)}, \
+ {"autovacuum_vacuum_cost_delay", RELOPT_TYPE_REAL, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_cost_delay)}, \
+ {"autovacuum_vacuum_cost_limit", RELOPT_TYPE_INT,\
+ OFFSET + offsetof(AutoVacOpts, vacuum_cost_limit)}, \
+ {"autovacuum_freeze_min_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_min_age)}, \
+ {"autovacuum_freeze_max_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_max_age)}, \
+ {"autovacuum_freeze_table_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_table_age)}, \
+ {"autovacuum_multixact_freeze_min_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, multixact_freeze_min_age)}, \
+ {
Re: [PATCH] src/test/modules/dummy_index -- way to test reloptions from inside of access method
shows a subset of values? I am afraid that we will get a mess that will work well, but it would be difficult for a human to find any logic in the output. And sooner or later we will need it, when something will go wrong and somebody will try to find out why. So it is better to test one option at a time, and that's why mute test output for other options. > Please note that these > are visible directly via pg_class.reloptions. So we could shave quite > some code. Values from pg_class are well tested in regression test. My point here is to check that they reach index internal as expected. And there is a long way between pg_class.reloptions and index internals. > Please note that the compilation of the module fails. > nodes/relation.h maybe is access/relation.h? You may want to review > all that. Hm... I do not quite understand how it get there and why in worked for me before. But I changed it to nodes/pathnodes.h It was here because it is needed for PlannerInfo symbol. PS. Sorry for long delays. I do not always have much time to do postgres... diff --git a/.gitignore b/.gitignore index 794e35b..37331c2 100644 diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile index dfd0956..742078e 100644 --- a/src/test/modules/Makefile +++ b/src/test/modules/Makefile @@ -7,6 +7,7 @@ include $(top_builddir)/src/Makefile.global SUBDIRS = \ brin \ commit_ts \ + dummy_index_am \ dummy_seclabel \ snapshot_too_old \ test_bloomfilter \ diff --git a/src/test/modules/dummy_index_am/.gitignore b/src/test/modules/dummy_index_am/.gitignore new file mode 100644 index 000..5dcb3ff --- /dev/null +++ b/src/test/modules/dummy_index_am/.gitignore @@ -0,0 +1,4 @@ +# Generated subdirectories +/log/ +/results/ +/tmp_check/ diff --git a/src/test/modules/dummy_index_am/Makefile b/src/test/modules/dummy_index_am/Makefile new file mode 100644 index 000..695941b --- /dev/null +++ b/src/test/modules/dummy_index_am/Makefile @@ -0,0 +1,21 @@ +# contrib/bloom/Makefile + +MODULE_big = dummy_index_am +OBJS = dummy_index.o direloptions.o $(WIN32RES) + +EXTENSION = dummy_index_am +DATA = dummy_index_am--1.0.sql +PGFILEDESC = "dummy index access method - needed for all kinds of tests" + +REGRESS = reloptions + +ifdef USE_PGXS +PG_CONFIG = pg_config +PGXS := $(shell $(PG_CONFIG) --pgxs) +include $(PGXS) +else +subdir = src/test/modules/dummy_index_am +top_builddir = ../../../.. +include $(top_builddir)/src/Makefile.global +include $(top_srcdir)/contrib/contrib-global.mk +endif diff --git a/src/test/modules/dummy_index_am/README b/src/test/modules/dummy_index_am/README new file mode 100644 index 000..61f80d3 --- /dev/null +++ b/src/test/modules/dummy_index_am/README @@ -0,0 +1,45 @@ +Dummy index AM +== + +Dummy index, an index module for testing am-related internal calls that can't be +properly tested inside production indexes, but better to be tested inside actual +access method module. + +Guidelines: +1. Keep this index as simple as as possible. It should not do any real indexing +(unless you need it for your tests), if you need it make it as simple as +possible; +2. Keep all code related to feature you test inside id[name_of_a_feature].c +file. dummy_index.c file should contain code that is needed for everyone, and +make calls to feature-related code; +3. Code related to your feature tests should be enabled and disabled by GUC +variables. Create as many boolean GUC variables as you need and set them on so +your test will know when it is time to do test output. Thus output from +different features tests are not interfered; +4. Add section to this README file that describes the general idea of what your +tests do. + +Reloptions tests + + +Some relotions testing are done in regression tests. There we can check if new +option value properly reaches pg_class, minimum/maximum checks, etc. But we +can't check if option value successfully reached index internal code when we are +dealing with production indexes. But we can do it here in Dummy index. +Here we check that values of reloptions properly reaches am-internals, and that +all reloption-related code works as expected. Several GUC variables are defined +to control test's output. do_test_reloptions turns on and off any +reloptions-related output. You will also get some test output in the case when +no relation options is set at all. do_test_reloption_int, +do_test_reloption_real, do_test_reloption_bool and do_test_reloption_string +allows to enable test output for int, real, boolean and string options. +do_test_reloption_string2 enables test output for another string option. We need +second string option, because a) string reloption implementation is a bit +tricky, and it is better to check that second value works as well; b) better to +check both cases: string option with validation function and without one, so we +need two string
Re: Ltree syntax improvement
В письме от воскресенье, 24 февраля 2019 г. 14:31:55 MSK пользователь Dmitry
Belyavsky написал:
Hi! Am back here again.
I've been thinking about this patch a while... Come to some conclusions and
wrote some examples...
First I came to idea that the best way to simplify the code is change the
state machine from if to switch/case. Because in your code a lot of repetition
is done just because you can't say "Thats all, let's go to next symbol" in any
place in the code. Now it should follow all the branches of if-else tree that
is inside the state-machine "node" to get to the next symbol.
To show how simpler the things would be I changed the state machine processing
in lquery_in form if to switch/case, and changed the code for LQPRS_WAITLEVEL
state processing, removing duplicate code, using "break" wherever it is
possible
(The indention in this example is unproper to make diff more clear)
so from that much of code
=
if (state == LQPRS_WAITLEVEL)
{
if (t_isspace(ptr))
{
ptr += charlen;
pos++;
continue;
}
escaped_count = 0;
real_levels++;
if (charlen == 1)
{
if (t_iseq(ptr, '!'))
{
GETVAR(curqlevel) = lptr = (nodeitem *)
palloc0(sizeof(nodeitem) * numOR);
lptr->start = ptr + 1;
state = LQPRS_WAITDELIM;
curqlevel->numvar = 1;
curqlevel->flag |= LQL_NOT;
hasnot = true;
}
else if (t_iseq(ptr, '*'))
state = LQPRS_WAITOPEN;
else if (t_iseq(ptr, '\\'))
{
GETVAR(curqlevel) = lptr = (nodeitem *)
palloc0(sizeof(nodeitem) * numOR);
lptr->start = ptr;
curqlevel->numvar = 1;
state = LQPRS_WAITESCAPED;
}
else if (strchr(".|@%{}", *ptr))
{
UNCHAR;
}
else
{
GETVAR(curqlevel) = lptr = (nodeitem *)
palloc0(sizeof(nodeitem) * numOR);
lptr->start = ptr;
state = LQPRS_WAITDELIM;
curqlevel->numvar = 1;
if (t_iseq(ptr, '"'))
{
lptr->flag |= LVAR_QUOTEDPART;
}
}
}
else
{
GETVAR(curqlevel) = lptr = (nodeitem *)
palloc0(sizeof(nodeitem) *
numOR);
lptr->start = ptr;
state = LQPRS_WAITDELIM;
curqlevel->numvar = 1;
}
}
=
I came to this
=
case LQPRS_WAITLEVEL:
if (t_isspace(ptr))
break; /* Just go to next symbol */
escaped_count = 0;
real_levels++;
if (charlen == 1)
{
if (strchr(".|@%{}", *ptr))
UNCHAR;
if (t_iseq(ptr, '*'))
{
state = LQPRS_WAITOPEN;
break;
}
}
GETVAR(curqlevel) = lptr = (nodeitem *)
palloc0(sizeof(nodeitem) *
numOR);
lptr->start = ptr;
curqlevel->numvar = 1;
state = LQPRS_WAITDELIM;
if (charlen == 1)
{
if (t_iseq(ptr, '\\'))
{
state = LQPRS_WAITESCAPED;
break;
}
Re: pageinspect: add tuple_data_record()
В письме от 16 октября 2018 19:17:20 пользователь Andres Freund написал: > Hi, > > On 2018-10-16 22:05:28 -0400, James Coleman wrote: > > > I don't think it's entirely safe to do so for invisible rows. The toast > > > references could be reused, constraints be violated, etc. So while I > > > could have used this several times before, it's also very likely a good > > > way to cause trouble. It'd probably be ok to just fetch the binary > > > value of the columns, but detoasting sure ain't ok. > > Wouldn’t that be equally true for the already existing toast option to > > tuple_data_split()? > Yes. Whoever added that didn't think far enough. Teodor, Nikolay? I did compleatly got the question... The question is it safe to split tuple record into array of raw bytea? It is quite safe from my point of view. We use only data that is inside the tuple, and info from pg_catalog that describes the tuple structure. So we are not affected if for example toast table were cleaned by vacuum. If you try to deTOAST data when TOAST table were already overwritten by new data, you can get some trouble... > > Is “unsafe” here something that would lead to a crash? Or just returning > > invalid data? I think that you should try and see. Find a way to make postgres vacuum toast table, and overwrite it with some new data, but keep tuple in heap page untouched. Then try to deTOAST old data and see what happened. (Hint: it is good to turn off compression for your data to be TOASTed, so you can just look into toast-file in binary mode and get an idea what in it now) == >> Summary: >> The new function tuple_data_record() parallels the existing >> tuple_data_split() function, but instead of returning a bytea array of raw >> attribute heap values, it returns a row type of the relation being >> examined. I find this approach a bit wrong. pageinspect is about viewing data in raw mode, not about looking at the tuples that were deleted. It can be used for this task, but it has other main purpose. I would suggest instead of automatically detoast and uncompress actual data, I would add functions that: 1. Checks if TOAST id is valid, and data can be fetched 2. Fetches that data and print it in raw mode 3. Says if binary data is compressed varlen data 4. Uncompress binary data and return it as another binary data 5. Convert binary data into string. or something like that. This would be more according to the pageinspect spirit, as I see it. Than using these functions you can write pure sql wrapper that do what you really want. -- Do code for fun. signature.asc Description: This is a digitally signed message part.
Re: pageinspect: add tuple_data_record()
В письме от 17 октября 2018 12:36:54 пользователь James Coleman написал: > > I did compleatly got the question... The question is it safe to split > > tuple > > record into array of raw bytea? It is quite safe from my point of view. > > We > > use only data that is inside the tuple, and info from pg_catalog that > > describes the tuple structure. So we are not affected if for example toast > > table were cleaned by vacuum. If you try to deTOAST data when TOAST table > > were > > already overwritten by new data, you can get some trouble... > The existing tuple_data_split() method already explicitly allows deTOASTing > data, > so if this is a problem, the problem already exists in pageinspect. Oh. that's true... I do not remember writing it, but it seems I did it. After Andreas notion, I am sure this should be fixed by removing de_toast option for tuple_data_split... O_o -- Do code for fun. signature.asc Description: This is a digitally signed message part.
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от 12 сентября 2018 21:40:49 пользователь Nikolay Shaplov написал: > > As you mentioned in previous mail, you prefer to keep enum and > > relopt_enum_element_definition array in the same .h file. I'm not sure, > > but I think it is done to keep everything related to enum in one place > > to avoid inconsistency in case of changes in some part (e.g. change of > > enum without change of array). On the other hand, array content created > > without array creation itself in .h file. Can we move actual array > > creation into same .h file? What is the point to separate array content > > definition and array definition? > > Hm... This would be good. We really can do it? ;-) I am not C-expert, some > aspects of C-development is still mysterious for me. So if it is really ok > to create array in .h file, I would be happy to move it there (This patch > does not include this change, I still want to be sure we can do it) I've discussed this issue with a colleague, who IS C-expert, and his advice was not to include this static const into .h file. Because copy of this const would be created in all objective files where this .h were included. And it is not the best way... -- Do code for fun. signature.asc Description: This is a digitally signed message part.
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от 1 ноября 2018 11:10:14 пользователь Tom Lane написал: > >>> I see you lost the Oxford comma: > >>> > >>> -DETAIL: Valid values are "on", "off", and "auto". > >>> +DETAIL: Valid values are "auto", "on" and "off". > >>> > >>> Please put these back. > >> > >> Actually that's me who have lost it. The code with oxford comma would be > >> a > >> bit more complicated. We should put such coma when we have 3+ items and > >> do not put it when we have 2. > >> > >> Does it worth it? > > > > In my opinion, it is worth it. > > Uh ... I've not been paying attention to this thread, but this exchange > seems to be about somebody constructing a message like that piece-by-piece > in code. This has got to be a complete failure from the translatability > standpoint. See > > https://www.postgresql.org/docs/devel/static/nls-programmer.html#NLS-GUIDELI > NES It's a very good reason... In this case the only solution I can see is DETAIL: Valid values are: "value1", "value2", "value3". Where list '"value1", "value2", "value3"' is built in runtime but have no any bindnings to any specific language. And the rest of the message is 'DETAIL: Valid values are: %s' which can be properly translated. -- Do code for fun. signature.asc Description: This is a digitally signed message part.
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от 1 ноября 2018 18:26:20 пользователь Nikolay Shaplov написал:
> In this case the only solution I can see is
>
> DETAIL: Valid values are: "value1", "value2", "value3".
>
> Where list '"value1", "value2", "value3"' is built in runtime but have no
> any bindnings to any specific language. And the rest of the message is
> 'DETAIL: Valid values are: %s' which can be properly translated.
I've fiex the patch. Now it does not add "and" at all.
--
Do code for fun.diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index db84da0..b266c86 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -422,7 +422,11 @@ static relopt_real realRelOpts[] {{NULL}}
};
-static relopt_string stringRelOpts[] +static relopt_enum_element_definition gist_buffering_enum_def[] + GIST_OPTION_BUFFERING_ENUM_DEF;
+static relopt_enum_element_definition view_check_option_enum_def[] + VIEW_OPTION_CHECK_OPTION_ENUM_DEF;
+static relopt_enum enumRelOpts[] {
{
{
@@ -431,10 +435,8 @@ static relopt_string stringRelOpts[] RELOPT_KIND_GIST,
AccessExclusiveLock
},
- 4,
- false,
- gistValidateBufferingOption,
- "auto"
+ gist_buffering_enum_def,
+ GIST_OPTION_BUFFERING_AUTO
},
{
{
@@ -443,11 +445,14 @@ static relopt_string stringRelOpts[] RELOPT_KIND_VIEW,
AccessExclusiveLock
},
- 0,
- true,
- validateWithCheckOption,
- NULL
+ view_check_option_enum_def,
+ VIEW_OPTION_CHECK_OPTION_NOT_SET
},
+ {{NULL}}
+};
+
+static relopt_string stringRelOpts[] +{
/* list terminator */
{{NULL}}
};
@@ -494,6 +499,12 @@ initialize_reloptions(void)
realRelOpts[i].gen.lockmode));
j++;
}
+ for (i = 0; enumRelOpts[i].gen.name; i++)
+ {
+ Assert(DoLockModesConflict(enumRelOpts[i].gen.lockmode,
+ enumRelOpts[i].gen.lockmode));
+ j++;
+ }
for (i = 0; stringRelOpts[i].gen.name; i++)
{
Assert(DoLockModesConflict(stringRelOpts[i].gen.lockmode,
@@ -532,6 +543,14 @@ initialize_reloptions(void)
j++;
}
+ for (i = 0; enumRelOpts[i].gen.name; i++)
+ {
+ relOpts[j] = &enumRelOpts[i].gen;
+ relOpts[j]->type = RELOPT_TYPE_ENUM;
+ relOpts[j]->namelen = strlen(relOpts[j]->name);
+ j++;
+ }
+
for (i = 0; stringRelOpts[i].gen.name; i++)
{
relOpts[j] = &stringRelOpts[i].gen;
@@ -629,6 +648,9 @@ allocate_reloption(bits32 kinds, int type, const char *name, const char *desc)
case RELOPT_TYPE_REAL:
size = sizeof(relopt_real);
break;
+ case RELOPT_TYPE_ENUM:
+ size = sizeof(relopt_enum);
+ break;
case RELOPT_TYPE_STRING:
size = sizeof(relopt_string);
break;
@@ -708,6 +730,24 @@ add_real_reloption(bits32 kinds, const char *name, const char *desc, double defa
}
/*
+ * add_enuum_reloption
+ * Add a new enum reloption
+ */
+void
+add_enum_reloption(bits32 kinds, const char *name, const char *desc,
+ relopt_enum_element_definition *enum_def, int default_val)
+{
+ relopt_enum *newoption;
+
+ newoption = (relopt_enum *) allocate_reloption(kinds, RELOPT_TYPE_ENUM,
+ name, desc);
+ newoption->enum_def = enum_def;
+ newoption->default_val = default_val;
+
+ add_reloption((relopt_gen *) newoption);
+}
+
+/*
* add_string_reloption
* Add a new string reloption
*
@@ -1208,6 +1248,51 @@ parse_one_reloption(relopt_value *option, char *text_str, int text_len,
optreal->min, optreal->max)));
}
break;
+ case RELOPT_TYPE_ENUM:
+ {
+relopt_enum *optenum = (relopt_enum *) option->gen;
+relopt_enum_element_definition *el_def;
+
+parsed = false;
+for(el_def = optenum->enum_def; el_def->text_value; el_def++)
+{
+ if (pg_strcasecmp(value, el_def->text_value) == 0)
+ {
+ option->values.enum_val = el_def->numeric_value;
+ parsed = true;
+ break;
+ }
+}
+if (!parsed)
+{
+ /*
+ * If value is not among allowed enum text values, but we
+ * are not up to validating, just use default numeric value,
+ * otherwise we raise an error
+ */
+ if (!validate)
+ option->values.enum_val = optenum->default_val;
+ else
+ {
+ StringInfoData buf;
+ initStringInfo(&buf);
+ for(el_def = optenum->enum_def; el_def->text_value;
+ el_def++)
+ {
+ appendStringInfo(&buf,"\"%s\"",el_def->text_value);
+ if (el_def[1].text_value)
+appendStringInfo(&buf,", ");
+ }
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid value for \"%s\" option",
+ option->gen->name),
+ errdetail("Valid values are: %s.", buf.data)));
+ pfree(buf.data);
+ }
+}
+ }
+ break;
Re: [PATCH] Opclass parameters
В письме от 15 ноября 2018 18:26:43 пользователь Nikita Glukhov написал: > >> But since it is now "Rejected with feedback", let's wait till autumn. > > > > We don't want to wait that long. But now we only need to сome to an > > agreement > > about CREATE INDEX syntax and where to store the opclass parameters. > > Attached 2nd version of the patches. Nothing has changed since March, > this is just a rebased version. > > CREATE INDEX syntax and parameters storage method still need discussion. Hi! I'd like to review your patch if you do not mind. Since I am familiar with the subject. I'll add it to the TOP 3 of my IT TODO list :-) But even without carefully reading the code I have several important questions: 1. I am sure that this patch should contain code for opclass praram generic implementation, and _one_ example of how it is used for certain opclass. From my point of view, we should not add all hardcoded constant as opclass params just because we can. It should be done only when it is really needed. And each case needs special consideration. This is not only about code complexity, this worries me less, but also about documentation complexity. Once option is added, it should be documented. This will make documentation more hard to understand (and completely unexperienced users reads documentation too). If this option is needed, this is price we all pay. But if nobody really use it, I see no reason to make things more complex for everyone. 2. The second question, is why you create new column in one of the table of pg_catalog, when we have attopitions in attribute description. And each indexed column in index is technically a relation attribute. Options of index column should be stored there, from my point of view (yes I know it is a hassle to pass it there, but I did in in my preview, it can be done) 2.1. I did not read the code, I'll do it some time late, but the main question I have is, how you will manage case, when you set different values for same options of different columns. like: CREATE INDEX ON arrays USING gist ( arr1 gist__intbig_ops (siglen = 32), arr2 gist__intbig_ops (siglen = 64) ); It is easitly solved when you store them in attoptions. But my question how do you manage it... (I'll get the answer from the code soon, I hope) -- Do code for fun. signature.asc Description: This is a digitally signed message part.
Re: [PATCH] get rid of StdRdOptions, use individual binary reloptions representation for each relation kind instead
В письме от 2 октября 2018 13:46:13 пользователь Michael Paquier написал:
> On Fri, Sep 14, 2018 at 09:30:25PM +0300, Nikolay Shaplov wrote:
> > BTW this commit shows why do this patch is important: 857f9c36 adds new
> > option for b-tree indexes. But thanks to the StdRdOptions this option
> > will exist for no practical use in all heaps that has just any option set
> > to non-default value, and in indexes that use StdRdOptions (and also has
> > any option set) And there will be more. StdRdOptions is long outdated
> > solution and it needs to be replaced.
>
> This patch does not apply anymore, so this is moved to next CF, waiting
> for author.
Oup...It seems to me that I've fogot to rebase it when it is needed...
Did it now
--
Do code for fun.diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index db84da0..e7e2392 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -22,7 +22,7 @@
#include "access/htup_details.h"
#include "access/nbtree.h"
#include "access/reloptions.h"
-#include "access/spgist.h"
+#include "access/spgist_private.h"
#include "access/tuptoaster.h"
#include "catalog/pg_type.h"
#include "commands/defrem.h"
@@ -46,9 +46,8 @@
* upper and lower bounds (if applicable); for strings, consider a validation
* routine.
* (ii) add a record below (or use add__reloption).
- * (iii) add it to the appropriate options struct (perhaps StdRdOptions)
- * (iv) add it to the appropriate handling routine (perhaps
- * default_reloptions)
+ * (iii) add it to the appropriate options struct
+ * (iv) add it to the appropriate handling routine
* (v) make sure the lock level is set correctly for that operation
* (vi) don't forget to document the option
*
@@ -1004,7 +1003,7 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
case RELKIND_TOASTVALUE:
case RELKIND_MATVIEW:
case RELKIND_PARTITIONED_TABLE:
- options = heap_reloptions(classForm->relkind, datum, false);
+ options = relation_reloptions(classForm->relkind, datum, false);
break;
case RELKIND_VIEW:
options = view_reloptions(datum, false);
@@ -1337,63 +1336,133 @@ fillRelOptions(void *rdopts, Size basesize,
/*
- * Option parser for anything that uses StdRdOptions.
+ * Option parsing definition for autovacuum. Used in toast and heap options.
+ */
+
+#define AUTOVACUUM_RELOPTIONS(OFFSET)\
+ {"autovacuum_enabled", RELOPT_TYPE_BOOL, \
+ OFFSET + offsetof(AutoVacOpts, enabled)},\
+ {"autovacuum_vacuum_threshold", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_threshold)}, \
+ {"autovacuum_analyze_threshold", RELOPT_TYPE_INT,\
+ OFFSET + offsetof(AutoVacOpts, analyze_threshold)}, \
+ {"autovacuum_vacuum_cost_delay", RELOPT_TYPE_INT,\
+ OFFSET + offsetof(AutoVacOpts, vacuum_cost_delay)}, \
+ {"autovacuum_vacuum_cost_limit", RELOPT_TYPE_INT,\
+ OFFSET + offsetof(AutoVacOpts, vacuum_cost_limit)}, \
+ {"autovacuum_freeze_min_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_min_age)}, \
+ {"autovacuum_freeze_max_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_max_age)}, \
+ {"autovacuum_freeze_table_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_table_age)}, \
+ {"autovacuum_multixact_freeze_min_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, multixact_freeze_min_age)}, \
+ {"autovacuum_multixact_freeze_max_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, multixact_freeze_max_age)}, \
+ {"autovacuum_multixact_freeze_table_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, multixact_freeze_table_age)}, \
+ {"log_autovacuum_min_duration", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, log_min_duration)}, \
+ {"autovacuum_vacuum_scale_factor", RELOPT_TYPE_REAL, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_scale_factor)},\
+ {"autovacuum_analyze_scale_factor", RELOPT_TYPE_REAL,\
+ OFFSET + offsetof(AutoVacOpts, analyze_scale_factor)}
+
+/*
+ * Option parser for heap
*/
bytea *
-default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
+heap_reloptions(Datum reloptions, bool validate)
{
relopt_value *options;
- StdRdOptions *rdopts;
+ HeapRelOptions *rdopts;
int numoptions;
static const relopt_parse_elt tab[] = {
- {"fillfactor", RELOPT_TYPE_INT, offsetof(StdRdOptions, fillfactor)},
- {"autovacuum_enabled"
Re: [PATCH] Opclass parameters
В письме от 15 ноября 2018 18:26:43 пользователь Nikita Glukhov написал:
> Attached 2nd version of the patches. Nothing has changed since March,
> this is just a rebased version.
>
> CREATE INDEX syntax and parameters storage method still need discussion.
I've played around a bit with you patch and come to some conclusions, I'd like
to share. They are almost same as those before, but now there are more
details.
Again some issues about storing opclass options in pg_inedx:
1. Having both indoption and indoptions column in pg_index will make someone's
brain explode for sure. If not, it will bring troubles when people start
confusing them.
2. Now I found out how do you store option values for each opclass: text[] of
indoptions in pg_index is not the same as text[] in
reloptions in pg_catalog (and it brings more confusion). In reloption each
member of the array is a single option.
reloptions | {fillfactor=90,autovacuum_enabled=false}
In indoptions, is a whole string of options for one of the indexed attributes,
each array item has all options for one indexed attribute. And this string
needs furthermore parsing, that differs from reloption parsing.
indoptions | {"{numranges=150}","{numranges=160}"}
This brings us to the following issues:
2a. pg_index stores properties of index in general. Properties of each indexed
attributes is stored in pg_attribute table. If we follow this philosophy
it is wrong to add any kind of per-attribute array values into pg_index. These
values should be added to pg_attribute one per each pg_attribute entry.
2b. Since you've chosen method of storage that differs from one that is used
in reloptions, that will lead to two verstions of code that processes the
attributes. And from now on, if we accept this, we should support both of them
and keep them in sync. (I see that you tried to reuse as much code as
possible, but still you added some more that duplicate current reloptions
functionality.)
I know that relotions code is not really suitable for reusing. This was the
reason why I started solving oplcass option task with rewriting reloptions
code,to make it 100% reusable for any kind of options. So I would offer you
again to join me as a reviewer of that code. This will make opclass code more
simple and more sensible, if my option code is used...
3. Speaking of sensible code
Datum
g_int_options(PG_FUNCTION_ARGS)
{
Datum raw_options = PG_GETARG_DATUM(0);
boolvalidate = PG_GETARG_BOOL(1);
relopt_int siglen =
{ {"numranges", "number of ranges for compression", 0, 0, 9,
RELOPT_TYPE_INT },
G_INT_NUMRANGES_DEFAULT, 1, G_INT_NUMRANGES_MAX };
relopt_gen *optgen[] = { &siglen.gen };
int offsets[] = { offsetof(IntArrayOptions, num_ranges) };
IntArrayOptions *options =
parseAndFillLocalRelOptions(raw_options, optgen, offsets, 1,
sizeof(IntArrayOptions), validate);
PG_RETURN_POINTER(options);
}
It seems to be not a very nice hack.
What would you do if you would like to have both int, real and boolean options
for one opclass? I do not know how to implement it using this code.
We have only int opclass options for now, but more will come and we should be
ready for it.
4. Now getting back to not adding opclass options wherever we can, just
because we can:
4a. For inrarray there were no opclass options tests added. I am sure there
should be one, at least just to make sure it still does not segfault when you
try to set one. And in some cases more tests can be needed. To add and review
them one should be familiar with this opclass internals. So it is good when
different people do it for different opclasses
4b. When you add opclass options instead of hardcoded values, it comes to
setting minimum and maximum value. Why do you choose 1000 as maximum
for num_ranges in gist_int_ops in intarray? Why 1 as minimum? All these
decisions needs careful considerations and can't be done for bunch of
opclasses just in one review.
4c. Patch usually take a long path from prototype to final commit. Do you
really want to update all these opclasses code each time when some changes
in the main opclass option code is made? ;-)
So I would suggest to work only with intarray and add other opclasses later.
5. You've been asking about SQL grammar
> CREATE INDEX idx ON tab USING am (
>{expr {opclass | DEFAULT} ({name=value} [,...])} [,...]
> );
As for me I do not really care about it. For me all the solutions is
acceptable. But looking at is i came to one notion:
I've never seen before DEFAULT keyword to be used in this way. There is logic
in such usage, but I did not remember any practical usage case.
If there are such usages (I can easily missed it) or if it is somehow
recommended in SQL standard -- let it be. But if none above, I would suggest
to use WITH keyword instead. As it is already used for reloptions. As far as I
remember in my prototype
Re: Add extension options to control TAP and isolation tests
В письме от 10 ноября 2018 09:14:19 пользователь Michael Paquier написал: > > Nice cleanup. Also, I like the ability to enable/control more types of > > tests easily from the Makefile. What are the next steps for this > > patch? > > Thanks. It seems to me that a complete review is still in order, > particularly regarding the new makefile option names. And then, if > everybody caring about the topic is happy with the way the patch is > shaped, it can be carried over to being committed, which would be most > likely something I'll do. Is it ok, if I join the reviewing? I like test, especially TAP one, you know ;-) Since you are much more experienced in postgres then me, I'd try to understand how does the patch work, try to use if for writing more TAP test, and will report problems and thoughts I came across while doing that. So far while first reading the patch I came to following two 1. --- a/src/test/modules/brin/.gitignore +++ b/src/test/modules/brin/.gitignore @@ -1,3 +1,3 @@ # Generated subdirectories -/isolation_output/ +/output_iso/ /tmp_check/ For me name "output_iso" means nothing. iso is something about CD/DVD or about standards. I would not guess that iso stands for isolation if I did not know it already. isolation_output is more sensible: I have heard that there are some isolation tests, this must be something about it. May be it would be better to change it to isolation_output everywhere instead of changing to output_iso 2. --- a/doc/src/sgml/extend.sgml +++ b/doc/src/sgml/extend.sgml @@ -1293,6 +1293,34 @@ include $(PGXS) . + + ISOLATION + + +list of isolation test cases + + + + + + ISOLATION_OPTS + + +additional switches to pass to +pg_isolation_regress + + + + + + TAP_TESTS + + +switch defining if TAP tests need to be run + + + + NO_INSTALLCHECK I tried to find definition in documentation what does "isolation test" exactly means, but did not find. There is some general words about TAP tests in main postgres documentation https://www.postgresql.org/docs/11/regress-tap.html, but I would not understand anything from it if I did not already know how it works. In current extend-pgxs documentation there is some explanation about regression test, it sensible enough. Since TAP and isolation tests are introduced now, there should be same short explanation for both of them. And also it would be good to add links from extend-pgxs to regress-tap and regress saying that for more info about these tests one can look at postgres doc, because they work in a similar way. That's all so far. I'll look more into it later... -- Do code for fun. signature.asc Description: This is a digitally signed message part.
Re: Add extension options to control TAP and isolation tests
В письме от 21 ноября 2018 09:39:53 пользователь Michael Paquier написал: > > For me name "output_iso" means nothing. iso is something about CD/DVD or > > about standards. I would not guess that iso stands for isolation if I did > > not know it already. isolation_output is more sensible: I have heard that > > there are some isolation tests, this must be something about it. May be > > it would be better to change it to isolation_output everywhere instead of > > changing to output_iso > That's just a default configuration used by the isolation tester. > That's not much bothering with in my opinion for this patch, as the > patch is here to make sure that the default configuration gets used > where it had better be used. Changing this default value would be of > course doable technically, but that's around for years to changing it > does not seem like good idea. Ok. If it is long time tradition, let it be :-) > > I tried to find definition in documentation what does "isolation test" > > exactly means, but did not find. There is some general words about TAP > > tests in main postgres documentation > > https://www.postgresql.org/docs/11/regress-tap.html, > > but I would not understand anything from it if I did not already know how > > it works. > > Those are mentioned here as part of the additional test suites: > https://www.postgresql.org/docs/11/regress-run.html#id-1.6.20.5.5 Oh thanks (I feel urge to start editing documentation right now. I will surpress it, and do it, I hope, later.) May I also ask a question, I came across. It is not part of the patch, but nevertheless... If I look in the code, regression test are sql files with expected output that are in src/test/regress. If I look in the documentation, all the tests are regression tests including TAP tests. https://www.postgresql.org/docs/11/regress.html what is the right way to look at it? > > In current extend-pgxs documentation there is some explanation about > > regression test, it sensible enough. Since TAP and isolation tests are > > introduced now, there should be same short explanation for both of > > them. > > I see your point here, and it is true that documentation ought to be > better. So I have written out a new set of paragraphs which explain the > whereabouts of the new variables a bit more in depth in the section of > extend-pgxs. Oh thanks! Now it is much more clear. So, I continued exploring your patch. First I carefully read changes in pgxs.mk. The only part of it I did not understand is .PHONY: submake -submake: ifndef PGXS +submake: +ifdef REGRESS $(MAKE) -C $(top_builddir)/src/test/regress pg_regress$(X) endif +ifdef ISOLATION + $(MAKE) -C $(top_builddir)/src/test/isolation all +endif +endif # PGXS I suppose it is because the purpose of PGXS is never explained, not in the documentation, not in the code comments, and PGXS := $(shell $(PG_CONFIG) -- pgxs) sounds like some strange magic spell, that explains nothing. In what cases it is defined? In what cases it is not defined? What exactly does it store? Can we make things more easy to understand here? 2. As far as you said that TAP tests for bloom were never executed, I guess I should just ignore - -wal-check: temp-install - (prove_check) as a part of wrong way to execute TAP tests. (I fond no traces of "wal-check" string in the code, so I guess this build target is an invention of bloom authors) 3. The rest are trivial, except changes for contrib/test_decoding/ and src/test/modules/brin I will explore them in my next postgres-dev time slot and then my review work will be done :-) -- Do code for fun. signature.asc Description: This is a digitally signed message part.
Re: Add extension options to control TAP and isolation tests
В письме от 23 ноября 2018 09:43:45 пользователь Michael Paquier написал: > > So, I continued exploring your patch. First I carefully read changes in > > pgxs.mk. The only part of it I did not understand is > > > > .PHONY: submake > > > > -submake: > > ifndef PGXS > > > > +submake: > > +ifdef REGRESS > > > > $(MAKE) -C $(top_builddir)/src/test/regress pg_regress$(X) > > > > endif > > > > +ifdef ISOLATION > > + $(MAKE) -C $(top_builddir)/src/test/isolation all > > +endif > > +endif # PGXS > > This is to make sure that the necessary tools are compiled before > running the related tests. "submake:" needs to be moved out actually. > Thinking about it a bit more, we should also remove the "ifdef REGRESS" > and "ifdef ISOLATION" because there are some dependencies here. For > example TAP tests call pg_regress to initialize connection policy. TAP > tests may also use isolation_tester or such. Can you add some comments in this part of pgxs.mk that explains this thing about pre-building needed tools? This will make understanding it more easy... > > > I suppose it is because the purpose of PGXS is never explained, not in the > > documentation, not in the code comments, and PGXS := $(shell $(PG_CONFIG) > > -- pgxs) sounds like some strange magic spell, that explains nothing. In > > what cases it is defined? In what cases it is not defined? What exactly > > does it store? Can we make things more easy to understand here? > > That's part of a larger scope which is here: > https://www.postgresql.org/docs/11/extend-pgxs.html I've carefully read this documentation. And did not get clear explanation of what is the true purpose of PGXS environment variable. Only "The last three lines should always be the same. Earlier in the file, you assign variables or add custom make rules." May be it should bot be discussed in the doc, but it should be mentioned in pgxs.mk . I guess both PGXS nad NO_PGXS should be explicitly described, in order to make the rest of the code more readable. (As for me I now have some intuitive understanding of it's nature, but it would be better to have strict explanation) So about test_decoding contrib/test_decoding/Makefile: EXTRA_INSTALL=contrib/test_decoding This sounds a bit strange to me. Why in make file for we should explicitly specify that this is needed for tests. It is obviously needed! Man, we are testing it!! ;-) I would suspect that is should be added somewhere in pgxs.mk code, when it is needed. Or this is not as obvious and trivial as I see it? I guess it came from -submake-test_decoding: -$(MAKE) -C $(top_builddir)/contrib/test_decoding but still there is no logic in it. +REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/test_decoding/logical.conf +ISOLATION_OPTS = --temp-config $(top_srcdir)/contrib/test_decoding/logical.conf When I read these lines an idea about ISOLATION_CONF and REGRESS_CONF variables came into my mind. That are transformed into proper _OPTS in pgxs.mk ? Since there is only one use case, may be it do not worth it. So I just speak this thought aloud without any intention to make it reality. -$(MAKE) -C $(top_builddir)/src/test/regress all is replaced with $(MAKE) -C $(top_builddir)/src/test/regress pg_regress$(X) in pgxs.mk. I hope changing "all" to "pg_regress$(X)" is ok, because I do not understand it. I've also greped for "pg_isolation_regress_check" and found that it is also used in src/test/modules/snapshot_too_old that also uses PGXS (at least as an option) should not we include it in your patch too? So I think that is it. Since Artur said, he successfully used your TAP patch in other extensions, I will not do it, considering it is already checked. If you think it is good to recheck, I can do it too :-) signature.asc Description: This is a digitally signed message part.
Re: Add extension options to control TAP and isolation tests
В письме от 26 ноября 2018 08:53:20 Вы написали: > > I've carefully read this documentation. And did not get clear explanation > > of what is the true purpose of PGXS environment variable. Only > > > > "The last three lines should always be the same. Earlier in the file, you > > assign variables or add custom make rules." > > The definition of PGXS is here: > https://www.postgresql.org/docs/11/extend-pgxs.html Can you please provide the quotation? I see there PGXS mentioned several times as "a build infrastructure for extensions" and as an environment variable it is mentioned only in code sample PGXS := $(shell $(PG_CONFIG) --pgxs) include $(PGXS) So for me there is no explanation. Or it is hard to find (that is also bad) If we are done with your patch, can we still finish this line of discussion in order to create another small patch concerning PGXS-env-var comment? signature.asc Description: This is a digitally signed message part.
[PATCH] get rid of StdRdOptions, use individual binary reloptions representation for each relation kind instead
This is part or my bigger patch
https://www.postgresql.org/message-id/flat/2146419.veIEZdk4E4@x200m#2146419.veIEZdk4E4@x200m
we've decided to
commit by smaller parts.
Now in postgres an StdRdOptions structure is used as binary represenations of
reloptions for heap, toast, and some indexes. It has a number of fields, and
only heap relation uses them all. Toast relations uses only autovacuum
options, and indexes uses only fillfactor option.
So for example if you set custom fillfactor value for some index, then it will
lead to allocating 84 bytes of memory (sizeof StdRdOptions on i386) and only 8
bytes will be actually used (varlena header and fillfactor value). 74 bytes is
not much, but allocating them for each index for no particular reason is bad
idea, as I think.
Moreover when I wrote my big reloption refactoring patch, I came to "one
reloption kind - one binary representation" philosophy. It allows to write
code with more logic in it.
This patch replaces StdRdOptions with HeapRelOptions, ToastRelOptions,
BTRelOptions, HashRelOptions, SpGistRelOptions and PartitionedRelOptions
one for each relation kind that were using StdRdOptions before.
The second thing I've done, I've renamed Relation* macroses from
src/include/utils/rel.h, that were working with reloptions. I've renamed them
into Heap*, Toast* and View* (depend on what relation options they were
actually using)
I did it because there names were misleading. For example
RelationHasCheckOption can be called only for View relation, and will give
wrong result for other relation types. It just takes binary representation of
reloptions, cast is to (ViewOptions *) and then returns some value from it.
Naming it as ViewHasCheckOption would better reflect what it actually do, and
strictly specify that it is applicable only to View relations.
Possible flaws:
I replaced
saveFreeSpace = RelationGetTargetPageFreeSpace(state->rs_new_rel,
HEAP_DEFAULT_FILLFACTOR);
with
if (IsToastRelation(state->rs_new_rel))
saveFreeSpace = ToastGetTargetPageFreeSpace();
else
saveFreeSpace = HeapGetTargetPageFreeSpace(state->rs_new_rel);
wherever I met it (and other cases like that), but I am not sure if in some
cases that part of code is used for heap only or not. So may be this "ifs" is
not needed and should be removed, and only Heap-case should be left. But I am
not that much familiar with postgres internals to see it for sure... I need
advice of more experienced developers here.
--
Do code for fun.diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 46276ce..bf7441e 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -22,7 +22,7 @@
#include "access/htup_details.h"
#include "access/nbtree.h"
#include "access/reloptions.h"
-#include "access/spgist.h"
+#include "access/spgist_private.h"
#include "access/tuptoaster.h"
#include "catalog/pg_type.h"
#include "commands/defrem.h"
@@ -46,9 +46,8 @@
* upper and lower bounds (if applicable); for strings, consider a validation
* routine.
* (ii) add a record below (or use add__reloption).
- * (iii) add it to the appropriate options struct (perhaps StdRdOptions)
- * (iv) add it to the appropriate handling routine (perhaps
- * default_reloptions)
+ * (iii) add it to the appropriate options struct
+ * (iv) add it to the appropriate handling routine
* (v) make sure the lock level is set correctly for that operation
* (vi) don't forget to document the option
*
@@ -986,7 +985,7 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
case RELKIND_TOASTVALUE:
case RELKIND_MATVIEW:
case RELKIND_PARTITIONED_TABLE:
- options = heap_reloptions(classForm->relkind, datum, false);
+ options = relation_reloptions(classForm->relkind, datum, false);
break;
case RELKIND_VIEW:
options = view_reloptions(datum, false);
@@ -1319,61 +1318,133 @@ fillRelOptions(void *rdopts, Size basesize,
/*
- * Option parser for anything that uses StdRdOptions.
+ * Option parsing definition for autovacuum. Used in toast and heap options.
+ */
+
+#define AUTOVACUUM_RELOPTIONS(OFFSET)\
+ {"autovacuum_enabled", RELOPT_TYPE_BOOL, \
+ OFFSET + offsetof(AutoVacOpts, enabled)},\
+ {"autovacuum_vacuum_threshold", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_threshold)}, \
+ {"autovacuum_analyze_threshold", RELOPT_TYPE_INT,\
+ OFFSET + offsetof(AutoVacOpts, analyze_threshold)}, \
+ {"autovacuum_vacuum_cost_delay", RELOPT_TYPE_INT,\
+ OFFSET + offsetof(AutoVacOpts, vacuum_cost_delay)}, \
+ {"autovacuum_vacuum_cost_limit", RELOPT_TYPE_INT,\
+ OFFSET + offsetof(AutoVacOpts, vacuum_cost_limit)}, \
+ {"autovacuum_freeze_min_age", RELOPT_TYPE_INT, \
+ OFFSET + offse
Re: [HACKERS] [PATCH] Move all am-related reloption code into src/backend/access/[am-name] and get rid of relopt_kind for custom AM
В письме от 3 сентября 2017 11:45:43 пользователь Alvaro Herrera написал: > I think we should split this in at least two commits, I've added a third part of this patch to commitfest: https://www.postgresql.org/message-id/flat/2083183.Rn7qOxG4Ov@x200m#2083183.Rn7qOxG4Ov@x200m To finally commit the rest of the path (the main part of it at least) I need this, and enum-options patches to be commited. Hope it will be done in march and I will offer the last part in next commitfest. -- Do code for fun. signature.asc Description: This is a digitally signed message part.
Re: [PATCH] Opclass parameters
В письме от 28 февраля 2018 00:46:36 пользователь Nikita Glukhov написал:
> I would like to present patch set implementing opclass parameters.
>
> This feature was recently presented at pgconf.ru:
> http://www.sai.msu.su/~megera/postgres/talks/opclass_pgconf.ru-2018.pdf
>
> A analogous work was already done by Nikolay Shaplov two years ago:
> https://www.postgresql.org/message-id/5213596.TqFRiqmCTe%40nataraj-amd64
> But this patches are not based on it, although they are very similar.
Hi!
You know, I am still working on this issue.
When I started to work on it, I found out that option code is not flexible at
all, and you can' reuse it for options that are not relOptions.
I gave your patch just a short glance for now, but as far as I can you start
deviding options into global and local one. I am afraid it will create grater
mess than it is now.
What I am doing right now, I am creating a new reloption internal API, that
will allow to create any option in any place using the very same code.
I think it should be done first, and then use it for opclass parameters and
any kind of options you like.
The big patch is here https://commitfest.postgresql.org/14/992/ (I am afraid
it will not apply to current master as it is, It is quite old. But you can get
the idea)
The smaller parts of the patch that in current commitfest are
https://commitfest.postgresql.org/17/1536/
https://commitfest.postgresql.org/17/1489/
You can help reviewing them. Then the whole thing will go faster. The second
patch is quite trivial. The first will also need attention of someone who is
really good in understanding postgres internals.
---
Concerning the patch that you've provided. I've just have a short look. But I
already have some question.
1. I've seen you've added a new attribute into pg_index. Why??!!
As far as I can get, if have index built on several columns (A1, A2, A3) you
can set, own opclass for each column. And set individual options for each
opclass if we are speaking about options. So I would expect to have these
options not in pg_index, but in pg_attribute. And we already have one there:
attoptions.I just do not get how you have come to per-index options. May be I
should read code more attentively...
2. Your patch does not provide any example of your new tool usage. In my
prototype patch I've shown the implementation of opclass options for intarray.
May be you should do the same. (Use my example it will be more easy then do it
from scratch). It will give more understanding of how this improvement can be
used.
3.
--- a/src/test/regress/expected/btree_index.out
+++ b/src/test/regress/expected/btree_index.out
@@ -150,3 +150,8 @@ vacuum btree_tall_tbl;
-- need to insert some rows to cause the fast root page to split.
insert into btree_tall_tbl (id, t)
select g, repeat('x', 100) from generate_series(1, 500) g;
+-- Test unsupported btree opclass parameters
+create index on btree_tall_tbl (id int4_ops(foo=1));
+ERROR: access method "btree" does not support opclass options
+create index on btree_tall_tbl (id default(foo=1));
+ERROR: access method "btree" does not support opclass options
Are you sure we really need these in postgres???
-
So my proposal is the following:
let's commit
https://commitfest.postgresql.org/17/1536/
https://commitfest.postgresql.org/17/1489/
I will provide a final patch for option engine refactoring in commit fest, and
you will write you implementation of opclass options on top of it. We can to
it simultaneously. Or I will write opclass options myself... I do not care who
will do oplcass options as long it is done using refactored options engine. If
you do it, I can review it.
>
>
> Opclass parameters can give user ability to:
> * Define the values of the constants that are hardcoded now in the
> opclasses depending on the indexed data.
> * Specify what to index for non-atomic data types (arrays, json[b],
> tsvector). Partial index can only filter whole rows.
> * Specify what indexing algorithm to use depending on the indexed data.
>
>
> Description of patches:
>
> 1. Infrastructure for opclass parameters.
>
> SQL grammar is changed only for CREATE INDEX statement: parenthesized
> parameters in reloptions format are added after column's opclass name.
> Default opclass can be specified with DEFAULT keyword:
>
> CREATE INDEX idx ON tab USING am (
> {expr {opclass | DEFAULT} ({name=value} [,...])} [,...]
> );
>
> Example for contrib/intarray:
>
> CREATE INDEX ON arrays USING gist (
>arr gist__intbig_ops (siglen = 32),
>arr DEFAULT (numranges = 100)
> );
>
> \d arrays
> Table "public.arrays"
> Column | Type| Collation | Nullable | Default
> +---
Re: [PATCH] Opclass parameters
В письме от 1 марта 2018 23:02:20 пользователь Oleg Bartunov написал: > > 2. Your patch does not provide any example of your new tool usage. In my > > prototype patch I've shown the implementation of opclass options for > > intarray. May be you should do the same. (Use my example it will be more > > easy then do it from scratch). It will give more understanding of how > > this improvement can be used. > > Hey, look on patches, there are many examples ! Oups...Sorry. I've looked at the patch from commitfest https://commitfest.postgresql.org/17/1559/ and it have shown only the first file. And When I read the letter I did not pay attention to attachments at all. So I was sure there is only one there. Yes. Now I see seven examples. But I think seven it is too many. For each case a reviewer should make consideration if this parameter worth moving to opclass options, or fixed definition in the C code is quite ok. Doing it for whole bunch, may make it messy. I think, it would be good to commit an implementation of opclass options, with a good example of usage. And then commit patches for all cases where these options can be used. But since it is now "Rejected with feedback", let's wait till autumn. -- Do code for fun. signature.asc Description: This is a digitally signed message part.
Re: [PATCH] get rid of StdRdOptions, use individual binary reloptions representation for each relation kind instead
В письме от 1 марта 2018 16:15:32 пользователь Andres Freund написал: > > This is part or my bigger patch > > https://www.postgresql.org/message-id/flat/2146419.veIEZdk4E4@x200m#21464 > > 19.veIEZdk4E4@x200m we've decided to commit by smaller parts. > > I've not read that thread. Is this supposed to be a first step towards > something larger? I've started from the idea of opclass options. Same as here https://commitfest.postgresql.org/17/1559/ And found out that current reloptions code is not flexible at all, and could not be easily reused for other kind of options (that are not reloptions). So I tried to change reloptions code to make universal, and then use it for any option purposes. In the patch https://commitfest.postgresql.org/14/992/ I created a new file options.c, that works with options as with an abstraction. It has some option definition structure, that has all information about how this options should be parsed and transformed into binary representation, I called this structure OptionsCatalog, but name can be changed. And it also have all kind of functions that do all needed tranformation with options. Then reloptions.c uses functions from options options.c for all options transformations, and takes care about relation specificity of reloptions. While doing all of these, I first of all had to adjust code that was written around reloptions, so that there code would stay in tune with new reloptions code. And second, I met places where current reloption related code were imperfect, and I decided to change it too... Since I get a really big patch as a result, it was decided to commit it in parts. This patch is the adjusting of reloption related code. It does not change a great deal, but when you first commit it, the main reloptions refactoring patch will no longer look like a big mess, it will became much more logical. Enum options patch is more about making coder a little bit more perfect. It is not really needed for the main patch, but it is more easy to introduce it before, then after. https://commitfest.postgresql.org/17/1489/ There is one more patch, that deals with toast.* options for tables with no TOAST, but we can set it aside, it is independent enough, to deal with it later... https://commitfest.postgresql.org/17/1486/ The patch with reloptions tests I've written while working on this patch, were already commited https://commitfest.postgresql.org/15/1314/ > > So for example if you set custom fillfactor value for some index, then it > > will lead to allocating 84 bytes of memory (sizeof StdRdOptions on i386) > > and only 8 bytes will be actually used (varlena header and fillfactor > > value). 74 bytes is not much, but allocating them for each index for no > > particular reason is bad idea, as I think. > I'm not sure this is a particularly strong motivator though? Does the > patch have a goal besides saving a few bytes? My real motivation for this patch is to make code more uniform. So the patch over it will be much clearer. And to make result code more clear and uniform too. I guess long time ago, the StdRdOptions were the way to make code uniform. There were only one binary representation of options, and all relation were using it. Quite uniform. But now, some relation kinds uses it's own options binary representations. Some still uses StdRdOptions, but only some elements from it. It is not uniform enough for me. If start using individual binary representation for each relation kind, then code will be uniform and homogeneous again, and my next patch over it will be more easy to read and understand. > > Moreover when I wrote my big reloption refactoring patch, I came to "one > > reloption kind - one binary representation" philosophy. It allows to write > > code with more logic in it. > I have no idea what this means? I hope I managed to explain it above... May be I am not good enough with explaining, or with English. Or with explaining in English. If it is not clear enough, please tell me, what points are not clear, I'll try to explain more... > Are you aiming this for v11 or v12? I hope to commit StdRdOptions and Enum_Options patches before v11. Hope it does not go against any rules, since there patches does not change any big functionality, just do some refactoring and optimization, of the code that already exists. As for main reloptions refactoring patch, if there is a chance to get commited before v11 release, it would be great. If not, I hope to post it to commitfest, right after v11 were released, so it would be possible to commit opclass options upon it before v12 release. -- Do code for fun. signature.asc Description: This is a digitally signed message part.
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от 1 марта 2018 19:11:05 пользователь Nikita Glukhov написал:
> Hi.
>
> I have refactored patch by introducing new struct relop_enum_element to make
> it possible to use existing C-enum values in option's definition. So,
> additional enum GIST_OPTION_BUFFERING_XXX was removed.
Hi! I've imported yours relopt_enum_element solution. Since Alvaro liked it,
and I like it to. But I called it relopt_enum_element_definition, as it is not
an element itself, but a description of possibilities.
But I do not want to import the rest of it.
> #define RELOPT_ENUM_DEFAULT ((const char *) -1) /* pseudo-name for
> default
> value */
I've discussed this solution with my C-experienced friends, and each of them
said, that it will work, but it is better not to use ((const char *) -1) as it
will stop working without any warning, because it is not standard C solution
and newer C-compiler can stop accepting such thing without further notice.
I would avoid using of such thing if possible.
> Also default option value should be placed now in the first element of
> allowed_values[]. This helps not to expose default values definitions (like
> GIST_BUFFERING_AUTO defined in gistbuild.c).
For all other reloption types, default value is a part of relopt_* structure.
I see no reason to do otherwise for enum.
As for exposing GIST_BUFFERING_AUTO. We do the same for default fillfactor
value. I see no reason why we should do otherwise here...
And if we keep default value on relopt_enum, we will not need
RELOPT_ENUM_DEFAULT that, as I said above, I found dubious.
> for (elem = opt_enum->allowed_values; elem->name; elem++)
It is better then I did. I imported that too.
> if (validate && !parsed)
Oh, yes... There was my mistake. I did not consider validate == false case.
I should do it. Thank you for pointing this out.
But I think that we should return default value if the data in pg_class is
brocken.
May be I later should write an additional patch, that throw WARNING if
reloptions from pg_class can't be parsed. DB admin should know about it, I
think...
The rest I've kept as I do before. If you think that something else should be
changed, I'd like to see, not only the changes, but also some explanations. I
am not sure, for example that we should use same enum for reloptions and
metapage buffering mode representation for example. This seems to be logical,
but it may also confuse. I wan to hear all opinions before doing it, for
example.
May be
typedef enum gist_option_buffering_numeric_values
{
GIST_OPTION_BUFFERING_ON = GIST_BUFFERING_STATS,
GIST_OPTION_BUFFERING_OFF = GIST_BUFFERING_DISABLED,
GIST_OPTION_BUFFERING_AUTO = GIST_BUFFERING_AUTO,
} gist_option_buffering_value_numbers;
will do, and then we can assign one enum to another, keeping the traditional
variable naming?
I also would prefer to keep all enum definition in an .h file, not enum part
in .h file, and array part in .c.
--
Do code for fun.diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 46276ce..4b775ab 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -404,7 +404,11 @@ static relopt_real realRelOpts[] {{NULL}}
};
-static relopt_string stringRelOpts[] +static relopt_enum_element_definition gist_buffering_enum_def[] + GIST_OPTION_BUFFERING_ENUM_DEF;
+static relopt_enum_element_definition view_check_option_enum_def[] + VIEW_OPTION_CHECK_OPTION_ENUM_DEF;
+static relopt_enum enumRelOpts[] {
{
{
@@ -413,10 +417,8 @@ static relopt_string stringRelOpts[] RELOPT_KIND_GIST,
AccessExclusiveLock
},
- 4,
- false,
- gistValidateBufferingOption,
- "auto"
+ gist_buffering_enum_def,
+ GIST_OPTION_BUFFERING_AUTO
},
{
{
@@ -425,11 +427,14 @@ static relopt_string stringRelOpts[] RELOPT_KIND_VIEW,
AccessExclusiveLock
},
- 0,
- true,
- validateWithCheckOption,
- NULL
+ view_check_option_enum_def,
+ VIEW_OPTION_CHECK_OPTION_NOT_SET
},
+ {{NULL}}
+};
+
+static relopt_string stringRelOpts[] +{
/* list terminator */
{{NULL}}
};
@@ -476,6 +481,12 @@ initialize_reloptions(void)
realRelOpts[i].gen.lockmode));
j++;
}
+ for (i = 0; enumRelOpts[i].gen.name; i++)
+ {
+ Assert(DoLockModesConflict(enumRelOpts[i].gen.lockmode,
+ enumRelOpts[i].gen.lockmode));
+ j++;
+ }
for (i = 0; stringRelOpts[i].gen.name; i++)
{
Assert(DoLockModesConflict(stringRelOpts[i].gen.lockmode,
@@ -514,6 +525,14 @@ initialize_reloptions(void)
j++;
}
+ for (i = 0; enumRelOpts[i].gen.name; i++)
+ {
+ relOpts[j] = &enumRelOpts[i].gen;
+ relOpts[j]->type = RELOPT_TYPE_ENUM;
+ relOpts[j]->namelen = strlen(relOpts[j]->name);
+ j++;
+ }
+
for (i = 0; stringRelOpts[i].gen.name; i++)
{
relOpts[j] = &stringRelOpts[i].gen;
@@ -611,6 +630,9 @@ allocate_reloption(bits32 kinds, int type, const char *name, const char *desc)
case RELOPT_TYPE_REAL:
size = sizeof(relo
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от 1 марта 2018 14:47:35 пользователь Alvaro Herrera написал: > I see you lost the Oxford comma: > > -DETAIL: Valid values are "on", "off", and "auto". > +DETAIL: Valid values are "auto", "on" and "off". > > Please put these back. Actually that's me who have lost it. The code with oxford comma would be a bit more complicated. We should put such coma when we have 3+ items and do not put it when we have 2. Does it worth it? As I've read oxford using of comma is not mandatory and used to avoid ambiguity. "XXX, YYY and ZZZ" can be read as "XXX, YYY, ZZZ" or as "XXX, (YYY and ZZZ)". oxford comma is used to make sure that YYY and ZZZ are separate items of the list, not an expression inside one item. But here we hardly have such ambiguity. So I'll ask again, do you really think it worth it? -- Do code for fun. signature.asc Description: This is a digitally signed message part.
Re: [PATCH] get rid of StdRdOptions, use individual binary reloptions representation for each relation kind instead
В письме от 2 марта 2018 11:27:49 пользователь Andres Freund написал: > > Since I get a really big patch as a result, it was decided to commit it in > > parts. > > I get that, but I strongly suggest not creating 10 loosely related > threads, but keeping it as a patch series in one thread. It's really > hard to follow for people not continually paying otherwise. Oups. Sorry I thought I should do other way round and create a new thread for new patch. And tried to post a link to other threads for connectivity wherever I can. Will do it as you say from now on. But I am still confused what should I do if I am commiting two patch from the patch series in simultaneously... -- Do code for fun. signature.asc Description: This is a digitally signed message part.
Bug in jsonb_in function (14 & 15 version are affected)
Hi!
I found a bug in jsonb_in function (it converts json from sting representation
into jsonb internal representation).
To reproduce this bug (the way I found it) you should get 8bit instance of
postgres db:
1. add en_US locale (dpkg-reconfigure locales in debian)
2. initdb with latin1 encoding:
LANG=en_US ./initdb --encoding=LATIN1 -D my_pg_data
3. run database and execute the query:
SELECT
E'{\x0a"\x5cb\x5c"\x5c\x5c\x5c/\x5cb\x5cf\x5cn\x5cr\x5ct\x5c"\x5c\x5c\x5c\x5crZt\x5c"\x5c\x5c\x5c/\x5cb\x5c"\x5c\x5c\x5c/\x5cb\x5c"\x5cu000f0\x5cu000f000\x5cuDFFF"000\x5cu000\xb4\x5cuDBFF\x5cuDFFF0002000\x5cuDBFF'::jsonb;
In postgres 14 and 15, the backend will crash.
The packtrace produce with ASan is in the attached file.
This bug was found while fuzzing postgres input functions, using AFL++.
For now we are using lightweight wrapper around input functions that
create minimal environment for these functions to run conversion, and run the,
in fuzzer.
My colleagues (they will come here shortly) have narrowed down this query to
SELECT E'\n"\\u0"'::jsonb;
and says that is crashes even in utf8 locale.
They also have a preliminary version of patch to fix it. They will tell about
it soon, I hope.
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
=
==90012==ERROR: AddressSanitizer: negative-size-param: (size=-1)
#0 0x533f04 in __asan_memcpy
(/home/nataraj/dev/.install/default/bin/postgres+0x533f04)
#1 0x102145a in report_json_context
/home/nataraj/dev/postgrespro/src/backend/utils/adt/jsonfuncs.c:685:2
#2 0x1020a32 in json_ereport_error
/home/nataraj/dev/postgrespro/src/backend/utils/adt/jsonfuncs.c:631:3
#3 0x1020976 in pg_parse_json_or_ereport
/home/nataraj/dev/postgrespro/src/backend/utils/adt/jsonfuncs.c:511:3
#4 0x1009176 in jsonb_from_cstring
/home/nataraj/dev/postgrespro/src/backend/utils/adt/jsonb.c:265:2
#5 0x1008f3b in jsonb_in
/home/nataraj/dev/postgrespro/src/backend/utils/adt/jsonb.c:81:9
#6 0x121d547 in InputFunctionCall
/home/nataraj/dev/postgrespro/src/backend/utils/fmgr/fmgr.c:1533:11
#7 0x121dd10 in OidInputFunctionCall
/home/nataraj/dev/postgrespro/src/backend/utils/fmgr/fmgr.c:1636:9
#8 0x8cbd0e in stringTypeDatum
/home/nataraj/dev/postgrespro/src/backend/parser/parse_type.c:662:9
#9 0x87eb68 in coerce_type
/home/nataraj/dev/postgrespro/src/backend/parser/parse_coerce.c
#10 0x87dec1 in coerce_to_target_type
/home/nataraj/dev/postgrespro/src/backend/parser/parse_coerce.c:105:11
#11 0x891b40 in transformTypeCast
/home/nataraj/dev/postgrespro/src/backend/parser/parse_expr.c:2717:11
#12 0x88f0b8 in transformExprRecurse
/home/nataraj/dev/postgrespro/src/backend/parser/parse_expr.c:168:13
#13 0x88efb1 in transformExpr
/home/nataraj/dev/postgrespro/src/backend/parser/parse_expr.c:126:11
#14 0x8c358b in transformTargetEntry
/home/nataraj/dev/postgrespro/src/backend/parser/parse_target.c:96:11
#15 0x8c37c2 in transformTargetList
/home/nataraj/dev/postgrespro/src/backend/parser/parse_target.c:184:10
#16 0x83a583 in transformSelectStmt
/home/nataraj/dev/postgrespro/src/backend/parser/analyze.c:1317:20
#17 0x836ad4 in transformStmt
/home/nataraj/dev/postgrespro/src/backend/parser/analyze.c:366:15
#18 0x836d5e in transformOptionalSelectInto
/home/nataraj/dev/postgrespro/src/backend/parser/analyze.c:306:9
#19 0x8364d4 in transformTopLevelStmt
/home/nataraj/dev/postgrespro/src/backend/parser/analyze.c:256:11
#20 0x83640e in parse_analyze_fixedparams
/home/nataraj/dev/postgrespro/src/backend/parser/analyze.c:124:10
#21 0xef9c4b in pg_analyze_and_rewrite_fixedparams
/home/nataraj/dev/postgrespro/src/backend/tcop/postgres.c:659:10
#22 0xefe818 in exec_simple_query
/home/nataraj/dev/postgrespro/src/backend/tcop/postgres.c:1169:20
#23 0xefd92d in PostgresMain
/home/nataraj/dev/postgrespro/src/backend/tcop/postgres.c
#24 0xd837f0 in BackendRun
/home/nataraj/dev/postgrespro/src/backend/postmaster/postmaster.c:4538:2
#25 0xd82e6f in BackendStartup
/home/nataraj/dev/postgrespro/src/backend/postmaster/postmaster.c:4266:3
#26 0xd816b3 in ServerLoop
/home/nataraj/dev/postgrespro/src/backend/postmaster/postmaster.c:1833:7
#27 0xd7e8e0 in PostmasterMain
/home/nataraj/dev/postgrespro/src/backend/postmaster/postmaster.c:1505:11
#28 0xb7ddc2 in main
/home/nataraj/dev/postgrespro/src/backend/main/main.c:209:3
#29 0x77190d09 in __libc_start_main csu/../csu/libc-start.c:308:16
#30 0x4baa99 in _start
(/home/nataraj/dev/.install/default/bin/postgres+0
Build problem with square brackets in build path
I am not sure it is really a bug, but nevertheless If you do mkdir [source] git clone git://git.postgresql.org/git/postgresql.git [source] mkdir build; cd build ../\[source\]/configure make you will get make[1]: *** No rule to make target 'generated-headers'. Stop. If there are no "[]" in the path to the source, everything is OK. It would be OK for me, if it still does not work. But I would appreciate at least proper error message there (or at configure step), this would save time, for figuring out, why it suddenly stopped working. -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su signature.asc Description: This is a digitally signed message part.
Re: [PATCH] New [relation] option engine
В письме от пятница, 8 декабря 2023 г. 08:59:41 MSK пользователь Michael Paquier написал: > > I've rebased patch, so it could be add to commitfest again. > > This is a 270kB patch with quite a few changes, and a lot of code > > moved around: > > 47 files changed, 2592 insertions(+), 2326 deletions(-) > > Could it be possible to split that into more successive steps to ease > its review? Theoretically I can create patch with full options.c as it is in the patch now, and use that code only in index AM, and keep reloption.c mostly unchanged. This will be total mess with two different options mechanisms working in the same time, but this might be much more easy to review. When we are done with the first step, we can change the rest. If this will help to finally include patch into postgres, I can do it. Will that help you to review? -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su signature.asc Description: This is a digitally signed message part.
Re: [PATCH] New [relation] option engine
В письме от пятница, 8 декабря 2023 г. 15:59:09 MSK пользователь Alvaro Herrera написал: > > Theoretically I can create patch with full options.c as it is in the patch > > now, and use that code only in index AM, and keep reloption.c mostly > > unchanged. > > > > This will be total mess with two different options mechanisms working in > > the same time, but this might be much more easy to review. When we are > > done with the first step, we can change the rest. > > If this will help to finally include patch into postgres, I can do it. > > Will > > that help you to review? > > I don't think that's better, because we could create slight > inconsistencies between the code used for index AMs and the users of > reloptions. I've written quite good regression tests for it, there should be no inconsistency. > I'm not seeing any reasonable way to split this patch in smaller ones. Actually me neither. Not a good one anyway. But if somebody really need it to be split, it can be done that way. -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su signature.asc Description: This is a digitally signed message part.
Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions
A new version of the patch.
Autovacuum options were extended in b07642db
So I added that options to the current patch.diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index e136116..55bd1ec 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -46,9 +46,8 @@
* upper and lower bounds (if applicable); for strings, consider a validation
* routine.
* (ii) add a record below (or use add__reloption).
- * (iii) add it to the appropriate options struct (perhaps StdRdOptions)
- * (iv) add it to the appropriate handling routine (perhaps
- * default_reloptions)
+ * (iii) add it to the appropriate options struct
+ * (iv) add it to the appropriate handling routine
* (v) make sure the lock level is set correctly for that operation
* (vi) don't forget to document the option
*
@@ -1147,9 +1146,11 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
switch (classForm->relkind)
{
case RELKIND_RELATION:
- case RELKIND_TOASTVALUE:
case RELKIND_MATVIEW:
- options = heap_reloptions(classForm->relkind, datum, false);
+ options = heap_reloptions(datum, false);
+ break;
+ case RELKIND_TOASTVALUE:
+ options = toast_reloptions(datum, false);
break;
case RELKIND_PARTITIONED_TABLE:
options = partitioned_table_reloptions(datum, false);
@@ -1521,59 +1522,66 @@ fillRelOptions(void *rdopts, Size basesize,
/*
- * Option parser for anything that uses StdRdOptions.
+ * Option parsing definition for autovacuum. Used in toast and heap options.
+ */
+
+#define AUTOVACUUM_RELOPTIONS(OFFSET)\
+ {"autovacuum_enabled", RELOPT_TYPE_BOOL, \
+ OFFSET + offsetof(AutoVacOpts, enabled)},\
+ {"autovacuum_vacuum_threshold", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_threshold)}, \
+ {"autovacuum_vacuum_insert_threshold", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_ins_threshold)}, \
+ {"autovacuum_analyze_threshold", RELOPT_TYPE_INT,\
+ OFFSET + offsetof(AutoVacOpts, analyze_threshold)}, \
+ {"autovacuum_vacuum_cost_delay", RELOPT_TYPE_REAL, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_cost_delay)}, \
+ {"autovacuum_vacuum_cost_limit", RELOPT_TYPE_INT,\
+ OFFSET + offsetof(AutoVacOpts, vacuum_cost_limit)}, \
+ {"autovacuum_freeze_min_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_min_age)}, \
+ {"autovacuum_freeze_max_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_max_age)}, \
+ {"autovacuum_freeze_table_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_table_age)}, \
+ {"autovacuum_multixact_freeze_min_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, multixact_freeze_min_age)}, \
+ {"autovacuum_multixact_freeze_max_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, multixact_freeze_max_age)}, \
+ {"autovacuum_multixact_freeze_table_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, multixact_freeze_table_age)}, \
+ {"log_autovacuum_min_duration", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, log_min_duration)}, \
+ {"autovacuum_vacuum_scale_factor", RELOPT_TYPE_REAL, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_scale_factor)},\
+ {"autovacuum_vacuum_insert_scale_factor", RELOPT_TYPE_REAL, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_ins_scale_factor)}, \
+ {"autovacuum_analyze_scale_factor", RELOPT_TYPE_REAL,\
+ OFFSET + offsetof(AutoVacOpts, analyze_scale_factor)}
+/*
+ * Option parser for heap
*/
bytea *
-default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
+heap_reloptions(Datum reloptions, bool validate)
{
static const relopt_parse_elt tab[] = {
- {"fillfactor", RELOPT_TYPE_INT, offsetof(StdRdOptions, fillfactor)},
- {"autovacuum_enabled", RELOPT_TYPE_BOOL,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, enabled)},
- {"autovacuum_vacuum_threshold", RELOPT_TYPE_INT,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, vacuum_threshold)},
- {"autovacuum_vacuum_insert_threshold", RELOPT_TYPE_INT,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, vacuum_ins_threshold)},
- {"autovacuum_analyze_threshold", RELOPT_TYPE_INT,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, analyze_threshold)},
- {"autovacuum_vacuum_cost_limit", RELOPT_TYPE_INT,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, vacuum_cost_limit)},
- {"autovacuum_freeze_min_age", RELOPT_TYPE_INT,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, freeze_min_age)},
- {"autovacuum_freeze_max_age", RELOPT_TYPE_INT,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, freeze_max_age)},
- {"autovacuum_freeze_table_age"
[PATCH] minor reloption regression tests improvement
I'd like to suggest a patch for reloption regression tests.
This patch tests case, that can be rarely met in actual life: when reloptions
have some illegal option set (as a result of malfunction or extension
downgrade or something), and user tries to remove this option by using RESET.
Current postgres behaviour is to actually remove this option.
Like:
UPDATE pg_class SET reloptions = '{illegal_option=4}'
WHERE oid = 'reloptions_test'::regclass;
ALTER TABLE reloptions_test RESET (illegal_option);
Why this should be tested:
1. It is what postgres actually do now.
2. This behaviour is reasonable. DB User can fix problem without updating
pg_class, having rights to change his own table.
3. Better to get test alarm, if this behavior is accidentally changed.
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
diff --git a/src/test/regress/expected/reloptions.out b/src/test/regress/expected/reloptions.out
index b6aef6f654..9f460a7e60 100644
--- a/src/test/regress/expected/reloptions.out
+++ b/src/test/regress/expected/reloptions.out
@@ -183,6 +183,17 @@ SELECT reloptions FROM pg_class WHERE oid = (
{autovacuum_vacuum_cost_delay=23}
(1 row)
+-- Can reset option that is not allowed, but for some reason is already set
+UPDATE pg_class
+ SET reloptions = '{fillfactor=13,autovacuum_enabled=false,illegal_option=4}'
+ WHERE oid = 'reloptions_test'::regclass;
+ALTER TABLE reloptions_test RESET (illegal_option);
+SELECT reloptions FROM pg_class WHERE oid = 'reloptions_test'::regclass;
+reloptions
+--
+ {fillfactor=13,autovacuum_enabled=false}
+(1 row)
+
--
-- CREATE INDEX, ALTER INDEX for btrees
--
diff --git a/src/test/regress/sql/reloptions.sql b/src/test/regress/sql/reloptions.sql
index 4252b0202f..fadce3384d 100644
--- a/src/test/regress/sql/reloptions.sql
+++ b/src/test/regress/sql/reloptions.sql
@@ -105,6 +105,13 @@ SELECT reloptions FROM pg_class WHERE oid = 'reloptions_test'::regclass;
SELECT reloptions FROM pg_class WHERE oid = (
SELECT reltoastrelid FROM pg_class WHERE oid = 'reloptions_test'::regclass);
+-- Can reset option that is not allowed, but for some reason is already set
+UPDATE pg_class
+ SET reloptions = '{fillfactor=13,autovacuum_enabled=false,illegal_option=4}'
+ WHERE oid = 'reloptions_test'::regclass;
+ALTER TABLE reloptions_test RESET (illegal_option);
+SELECT reloptions FROM pg_class WHERE oid = 'reloptions_test'::regclass;
+
--
-- CREATE INDEX, ALTER INDEX for btrees
--
Re: TAP output format in pg_regress
Hi! Do this patch still need reviewer, or reviewer field in commit-fest have
been left empty by mistake?
I am fan of TAP-tests, so I am quite happy that pg_regress output changed to
TAP tests...
I've checked new output, if is conform TAP specification. Checked that prove
consumes new pg_regress output well.
Did not found quick way to include prove TAP harness right into Makefile, so I
check dumped output, but it is not really important for now, I guess.
As for the code, I gave it a quick readthrough... And main issue I've stumbled
was here:
--
indent = 36 - (parallel ? 8 : 0) - floor(log10(testnumber) + 1);
status("ok %-5i %s%-*s %8.0f ms",
testnumber, (parallel ? "" : ""), indent,
testname,
runtime);
--
This peace of code has non-obvious logic (I can solve it without taking time
for deep thinking) and this code (almost similar) is repeated three times.
This is hard to support, and error prone solution.
I would suggest to add one _print_test_status function, that also accepts
status string and do this complex calculations and formatting only once (
"# SKIP (ignored)" should also added as %s, so we have only one print with
complex format string).
Then you will have to read check and fix it only once.
And may be it would be good to make this calculations more human-readable...
If this patch really needs reviewer and my way of thinking is acceptable,
please let me know, I will set myself as a reviewer and will dig further into
the code.
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
signature.asc
Description: This is a digitally signed message part.
Re: TAP output format in pg_regress
You guys are really fast... I only think about problem, it is already
mentioned here... Most issues I've noticed are already fixed before I was able
to say something.
Nevertheless...
/*
* Bailing out is for unrecoverable errors which prevents further testing to
* occur and after which the test run should be aborted. By passing non_rec
* as true the process will exit with _exit(2) and skipping registered exit
* handlers.
*/
static void
bail_out(bool non_rec, const char *fmt,...)
{
In original code, where _exit were called, there were mention about recursion
(whatever it is) as a reason for using _exit() instead of exit(). Now this
mention is gone:
- _exit(2); /* not exit(), that could be recursive */
+ /* Not using the normal bail() as we want _exit */
+ _bail(_("could not stop postmaster: exit code was %d\n"), r);
The only remaining part of recursion is _rec suffix.
I guess we should keep mention of recursion in comments, and for me _rec
stands for "record", not "recursion", I will not guess original meaning by
_rec suffix. I would suggest to change it to _recurs or _recursive to keep
things clear
-
+#define _bail(...) bail_out(true, __VA_ARGS__)
+#define bail(...) bail_out(false, __VA_ARGS__)
I will never guess what the difference between bail, _bail and bail_out. We
need really good explanation here, or better to use self-explanatory names
here...
I would start bail_out with _ as it is "internal", not used in the code.
And for _bail I would try to find some name that shows it's nature. Like
bail_in_recursion or something.
-
+ if (ok || ignore)
{
- va_start(ap, fmt);
- vfprintf(logfile, fmt, ap);
- va_end(ap);
+ emit_tap_output(TEST_STATUS, "ok %-5i %s%-*s %8.0f ms%s\n",
+ testnumber,
+ (parallel ? PARALLEL_INDENT : ""),
+ padding, testname,
+ runtime,
+ (ignore ? " # SKIP (ignored)" : ""));
+ }
+ else
+ {
+ emit_tap_output(TEST_STATUS, "not ok %-5i %s%-*s %8.0f ms\n",
+ testnumber,
+ (parallel ? PARALLEL_INDENT : ""),
+ padding, testname,
+ runtime);
}
This magic spell "...%-5i %s%-*s %8.0f ms\n" is too dark to repeat it even two
times. I understand problems with spaces... But may be it would be better
somehow narrow it to one ugly print... Print "ok %-5i"|"not ok %-5i" to
buffer first, and then have one "%s%-*s %8.0f ms%s\n" print or something like
that...
I am not strongly insisting on that, but I feel strong urge to remove code
duplication here...
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
signature.asc
Description: This is a digitally signed message part.
Re: TAP output format in pg_regress
В письме от пятница, 25 ноября 2022 г. 00:20:01 MSK пользователь Daniel
Gustafsson написал:
+ /*
+ * The width of the testname field when printing to ensure vertical
alignment
+ * of test runtimes. Thius number is somewhat arbitrarily chosen to match
the
+ * older pre-TAP output format.
+ */
"Thius" seems to be a typo :-)
-
+ #define bail_noatexit(...) bail_out(true, __VA_ARGS__)
BTW what does "noat" stands for? I thought it is typo too :-) and originally
meant to be "not".
-
- snprintf(buf, sizeof(buf),
-_(" All %d tests passed. "),
-success_count);
- else if (fail_count == 0) /* fail_count=0, fail_ignore_count>0 */
- snprintf(buf, sizeof(buf),
-_(" %d of %d tests passed, %d failed test(s) ignored. "),
-success_count,
-success_count + fail_ignore_count,
-fail_ignore_count);
- else if (fail_ignore_count == 0)/* fail_count>0 && fail_ignore_count=0
*/
- snprintf(buf, sizeof(buf),
-_(" %d of %d tests failed. "),
-fail_count,
-success_count + fail_count);
+ note(_("All %d tests passed.\n"), success_count);
+ /* fail_count=0, fail_ignore_count>0 */
+ else if (fail_count == 0)
+ note(_("%d of %d tests passed, %d failed test(s) ignored.\n"),
+success_count,
+success_count + fail_ignore_count,
+fail_ignore_count);
+ /* fail_count>0 && fail_ignore_count=0 */
+ else if (fail_ignore_count == 0)
+ diag(_("%d of %d tests failed.\n"),
+fail_count,
+success_count + fail_count);
+ /* fail_count>0 && fail_ignore_count>0 */
Just out of overaccuracy: Logic here have not changed. Can we keep ifs, elses
and may be indent offsets of lines that did not change as they were to have
nicer diff? Would make understanding this changeset more easy... Or this is
work of pg_indent that spoils it?
While looking at the my output I am getting wrong offset for
sanity_check:
ok 84 hash_func 121 ms
ok 85 errors 68 ms
ok 86 infinite_recurse 233 ms
ok 87sanity_check 144 ms
# parallel group (20 tests): select_into delete random select_having
select_distinct_on namespace select_implicit case prepared_xacts subselect
transactions portals select_distinct union arrays update hash_index join
aggregates btree_index
ok 88 select_into134 ms
ok 89 select_distinct812 ms
(also for select_parallel write_parallel vacuum_parallel and fast_default)
I guess the intention was to align them too...
As for the rest: I see no other problems in the code, and consider it should
be passed to commiter (or may be more experienced reviewer)
> > On 24 Nov 2022, at 20:32, Andres Freund wrote:
> >
> > On November 24, 2022 11:07:43 AM PST, Daniel Gustafsson
wrote:
> >>> On 24 Nov 2022, at 18:07, Nikolay Shaplov wrote:
> >> One option could be to redefine bail() to take the exit function as a
> >> parameter and have the caller pass the preferred exit handler.
> >>
> >> -bail_out(bool non_rec, const char *fmt,...)
> >> +bail(void (*exit_func)(int), const char *fmt,...)
> >>
> >> The callsites would then look like the below, which puts a reference to
> >> the
> >> actual exit handler used in the code where it is called.
> >
> > I'd just rename _bail to bail_noatexit().
>
> That's probably the best option, done in the attached along with the comment
> fixup to mention the recursion issue.
>
> >>> This magic spell "...%-5i %s%-*s %8.0f ms\n" is too dark to repeat it
> >>> even two times. I understand problems with spaces... But may be it
> >>> would be better somehow narrow it to one ugly print... Print "ok %-5i
> >>> "|"not ok %-5i" to buffer first, and then have one "%s%-*s %8.0f
> >>> ms%s\n" print or something like that...
> >>
> >> I'm not convinced that this printf format is that hard to read (which may
> >> well be attributed to Stockholm Syndrome), and I do think that breaking
> >> it up and adding more code to print the line will make it less readable
> >> instead.>
> > I don't think it's terrible either. I do think it'd also be ok to switch
> >
Re: TAP output format in pg_regress
91 ms ok 11 types.oid 47 ms ok 12 types.float4 88 ms ok 13 types.float8 139 ms ok 14 types.bit165 ms ok 15 types.numeric 1065 ms but this does not seems very realistic in the current code base and the scope of this path. Theoretically TAP 14 has subtests and this parallel tests looks like subtests... but TAP 14 is not supported by modern harnesses.. So I have no idea... May be leaving it as it is for now, is good enough... -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su signature.asc Description: This is a digitally signed message part.
Re: TAP output format in pg_regress
В письме от понедельник, 28 ноября 2022 г. 21:28:48 MSK пользователь Andres Freund написал: > On 2022-11-28 14:13:16 +0100, Daniel Gustafsson wrote: > > > On 27 Nov 2022, at 11:22, Nikolay Shaplov wrote: > > > В письме от суббота, 26 ноября 2022 г. 23:35:45 MSK пользователь Daniel > > > Gustafsson написал: > > > > > > I wold suggest to use word immediate instead of noatexit. This will do > > > the > > > code much more sensible for me. > > > > I think noatexit is clearer since the codepath is specifically to avoid > > any > > registered atexit functions. The point of this function is to be able to > > call bail while in a function registered with atexit() so I think the > > current name is better. > > +1 Ok, then I would not insist on it. > > So this extra offset indicates that test is being included into parallel > > group? Guess it not really obvious... > > Grouping parallel tests via an initial list of test and then indenting each > test with whitespace was committed 22 years ago. While there might be > better > ways to do this, the lack of complaints so far at least seems to indicate > that it isn't all too terrible. Ok, it was there for 22 years, it will do no harm if it is left this way for some time :-) ---- From my reviewer's point of view patch is ready for commit. Thank you for your patience :-) PS Should I change commitfest status? -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su signature.asc Description: This is a digitally signed message part.
Re: [PATCH] New [relation] option engine
В письме от среда, 1 февраля 2023 г. 12:04:26 MSK пользователь Alvaro Herrera написал: > On 2023-Jan-31, vignesh C wrote: > > On Tue, 3 Jan 2023 at 18:38, vignesh C wrote: > > > The patch does not apply on top of HEAD as in [1], please post a rebased > > > patch: === Applying patches on top of PostgreSQL commit ID > > > 92957ed98c5c565362ce665266132a7f08f6b0c0 === > > > === applying patch ./new_options_take_two_v03f.patch > > > patching file src/include/access/reloptions.h > > > Hunk #1 FAILED at 1. > > > 1 out of 4 hunks FAILED -- saving rejects to file > > > src/include/access/reloptions.h.rej > > > > There has been no updates on this thread for some time, so this has > > been switched as Returned with Feedback. Feel free to open it in the > > next commitfest if you plan to continue on this. > > Well, no feedback has been given, so I'm not sure this is a great > outcome. In the interest of keeping it alive, I've rebased it. It > turns out that the only conflict is with the 2022 -> 2023 copyright line > update. > > I have not reviewed it. Guys sorry, I am totally unable to do just anything this month... Do not have spare resources to switch my attention to anything even this simple... Hope, next month will be better... And I will try to do some more reviewing of other patches... -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su signature.asc Description: This is a digitally signed message part.
Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions
В письме от пятница, 4 июня 2021 г. 3:19:09 MSK пользователь Jeff Davis написал: > > Moving reloptions to AM code is the goal I am slowly moving to. I've > > started > > some time ago with big patch > > https://commitfest.postgresql.org/14/992/ and > > have been told to split it into smaller parts. So I did, and this > > patch is > > last step that cleans options related things up, and then actual > > moving can be > > done. > > Thank you for working on this. Welcome! Sorry for slow reply, I am on summer vacations now, no chance for fast replies now :-) > Can you outline the plan for moving these options to the table AM to > make sure this patch is a step in the right direction? Yes, I can. First you can see the whole patch, the way it was several years ago: https://commitfest.postgresql.org/15/992/, reloptions8a.diff The things I would say is accurate for postgres ~11, it may have been changed since I last payed attention to them. So, there are three general places where options can be stored: 1. Global boolRelOpts, intRelOpts, realRelOpts, stringRelOpts in src/backend/ access/common/reloptions.c for in-core access methods. 2. custom_options array of accessable via add_bool_reloption, add_int_reloption, add_real_reloption, add_string_reloption for access methods from contribs. (See reloptions.c too) 3. And also each access method has an array of relopt_parse_elt[] that is also about reloptions. 1 and 2 are technically arrays of relopt_get, and store information what kind of options do we have. 3 is array of relopt_parse_elt[] that store information how options should be stored into binary representation. My idea was to merge relopt_get and relopt_parse_elt into one structure (in my patch it is "option_definition_basic"), each access method, that needs options, should have a set of option_definition_basic for their options (called option_definition_basic in my patch, may be should be renamed) and this set of option_definitions is the only data that is needed to parse option into binary representation. So in access method instead of am_option function we will have amrelopt_catalog function that returns "options defenition set" for this am, and this definition set is used by option parser to parse options. So, it my explanation is not quite clear, please ask questions, I will try to answer them. > I was trying to work through this problem as well[1], and there are a > few complications. > > * Which options apply to any relation (of any table AM), and which > apply to only heaps? As far as I can tell, the only one that seems > heap-specific is "fillfactor". From my point of view, each relation kind has it's own set of options. The fact, that almost every kind has a fillfactor is just a coincidence. If we try to do some optimization here, we will be buried under the complexity of it soon. So they are _different_ options just having the same name. > * Toast tables can be any AM, as well, so if we accept new reloptions > for a custom AM, we also need to accept options for toast tables of > that AM. When I wrote this patch, AM was introduced only to index relations. I do not how it is implemented for heap now, but there should be some logic in it. If toast tables has it's own AM, then option definition set should be stored there, and we should find a way to work with it, somehow. > * Implementation-wise, the bytea representation of the options is not > very easy to extend. Should we have a new text field in the catalog to > hold the custom options? I am not really understanding this question. Normally all options can be well represented as binary structure stored at bytea. I see no problem here. If we need some unusual behaviour, we can use string option with custom validation function. This should cover almost all needs I can imagine. === So it you are interested in having better option implementation, and has no ideas of your own, I would suggest to revive my patch and try to commit it. What I need first of all is a reviewer. Testing and coauthoring will also be apriciated. My original big patch, I gave you link to, have been split into several parts. The last minor part, that should be commit in advance, and have not been commit yet is https://commitfest.postgresql.org/33/2370/ If you join as a reviewer this would be splendid! :-) -- Nikolay Shaplov dh...@nataraj.su (e-mail, jabber) @dhyan:nataraj.su (matrix)
Re: Zstandard support for toast compression
В письме от вторник, 17 мая 2022 г. 23:01:07 MSK пользователь Tom Lane написал: Hi! I came to this branch looking for a patch to review, but I guess I would join the discussion instead of reading the code. > >> Yeah - I think we had better reserve the fourth bit pattern for > >> something extensible e.g. another byte or several to specify the > >> actual method, so that we don't have a hard limit of 4 methods. But > >> even with such a system, the first 3 methods will always and forever > >> be privileged over all others, so we'd better not make the mistake of > >> adding something silly as our third algorithm. > > > > In such a situation, would they really end up being properly distinct > > when it comes to what our users see..? I wouldn't really think so. > > It should be transparent to users, sure, but the point is that the > first three methods will have a storage space advantage over others. > Plus we'd have to do some actual work to create that extension mechanism. Postgres is well known for extensiblility. One can write your own implementation of almost everything and make it an extension. Though one would hardly need more than one (or two) additional compression methods, but which method one will really need is hard to say. So I guess it would be much better to create and API for creating and registering own compression method and create build in Zstd compression method that can be used (or optionally not used) via that API. Moreover I guess this API (may be with some modification) can be used for seamless data encryption, for example. So I guess it would be better to make it extensible from the start and use this precious bit for compression method chosen by user, and may be later extend it with another byte of compression method bits, if it is needed. -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su signature.asc Description: This is a digitally signed message part.
Re: Zstandard support for toast compression
В письме от пятница, 20 мая 2022 г. 23:17:42 MSK пользователь Stephen Frost написал: > While I generally agree that we want to provide extensibility in this > area, given that we already have zstd as a compile-time option and it > exists in other parts of the system, I don't think it makes sense to > require users to install an extension to use it. I mean that there can be Compression Method Provider, either built in postgres core, or implemented in extension. And one will need to create Compression method using desired Compression Method Provider. Like (it is just pure imagination) CREATE COMPRESSION METHOD MY_COMPRESSION USING 'my_compression_provider'; This will assign certain bits combination to that method, and later one can use that method for TOAST compression... > > Moreover I guess this API (may be with some modification) can be used for > > seamless data encryption, for example. > > Perhaps.. but this kind of encryption wouldn't allow indexing Yes, this will require some more efforts. But for encrypting pure storage that API can be quite useful. > and certainly lots of other metadata would still be unencrypted (the entire > system catalog being a good example..). In many it is enough to encrypt only sensible information itself, not whole metadata. My point was not to discuss DB encryption, it is quite complex issue, my point was to point out that API that allows custom compression methods may became useful for other solutions. Encryption was just first example that came in my mind. Robert Haas has another example for compression method optimized for certain data type. So it is good if you can have method of your own. -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su signature.asc Description: This is a digitally signed message part.
Re: [PATCH] minor reloption regression tests improvement
В письме от четверг, 30 июня 2022 г. 00:38:48 MSK пользователь Jacob Champion написал: > This patch is hanging open in the March commitfest. There was a bit of > back-and-forth on whether it should be rejected, but no clear statement on > the issue, so I'm going to mark it Returned with Feedback. If you still > feel strongly about this patch, please feel free to re-register it in the > July commitfest. Hi! I am surely feel this patch is important. I have bigger patch https://commitfest.postgresql.org/38/3536/ and this test makes sense as a part of big work of options refactoring, I am also was strongly advised to commit things chopped into smaller parts, when possible. This test can be commit separately so I am doing it. -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su signature.asc Description: This is a digitally signed message part.
Re: [PATCH] minor reloption regression tests improvement
В письме от четверг, 30 июня 2022 г. 06:47:48 MSK пользователь Nikolay Shaplov написал: > Hi! I am surely feel this patch is important. I have bigger patch > https://commitfest.postgresql.org/38/3536/ and this test makes sense as a > part of big work of options refactoring, > > I am also was strongly advised to commit things chopped into smaller parts, > when possible. This test can be commit separately so I am doing it. Let me again explain why this test is importaint, so potential reviewers can easily find this information. Tests are for developers. You change the code and see that something does not work anymore, as it worked before. When you change the code, you should keep both documented and undocumented behaviour. Because user's code can intentionally or accidentally use it. So known undocumented behavior is better to be tested as well as documented one. If you as developer want to change undocumented behavior, it is better to do it consciously. This test insures that. I, personally hit this problem while working with reloption code. Older version of my new patch changed this behaviour, and I even did not notice that. So I decided to write this test to ensure, that nobody ever meet same problem. And as I said before, I was strongly advised to commit in small parts, when possible. This test can be commit separately. -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su signature.asc Description: This is a digitally signed message part.
Re: [PATCH] New [relation] option engine
В письме от понедельник, 11 июля 2022 г. 23:03:55 MSK пользователь Jeff Davis написал: > > For index access methods "amoptions" member function that preformed > > option > > processing, were replaced with "amreloptspecset" member function that > > provided > > an SpecSet for reloptions for this AM, so caller can trigger option > > processing > > himself. > > What about table access methods? There have been a couple attempts to > allow custom reloptions for table AMs. Does this patch help that use > case? This patch does not add custom reloptions for table AM. It is already huge enough, with no extra functionality. But new option engine will make adding custom options for table AM more easy task, as main goal of this patch is to simplify adding options everywhere they needed. And yes, adding custom table AM options is one of my next goals, as soon as this patch is commit. -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su signature.asc Description: This is a digitally signed message part.
Re: Bug: Reading from single byte character column type may cause out of bounds memory reads.
В письме от среда, 13 июля 2022 г. 16:14:39 MSK пользователь Aleksander
Alekseev написал:
Hi! Let me join the review process. Postgres data types is field of expertise I
am interested in.
0. Though it looks like a steady bug, I can't reproduce it. Not using
valgrind, not using ASan (address sanitizer should catch reading out of bounds
too). I am running Debian Bullseye, and tried to build both postgresl 14.4 and
current master.
Never the less I would dig into this issue. And start with the parts that is
not covered by the patch, but seems to be important for me.
1. typename "char" (with quotes) is very-very-very confusing. it is described
in documentation, but you need to be postgres expert or careful documentation
reader, to notice important difference between "char" and char.
What is the history if "char" type? Is it specified by some standard? May be it
is good point to create more understandable alias, like byte_char, ascii_char
or something for usage in practice, and keep "char" for backward compatibility
only.
2. I would totally agree with Tom Lane and Isaac Morland, that problem should
be also fixed on the side of type conversion. There is whole big thread about
it. Guess we should come to some conclusion there
3.Fixing out of bound reading for broken unicode is also important. Though
for now I am not quite sure it is possible.
> - p += pg_mblen(p);
> + {
> + int t = pg_mblen(p);
> + p += t;
> + max_copy_bytes -= t;
> + }
Minor issue: Here I would change variable name from "t" to "char_len" or
something, to make code more easy to understand.
Major issue: is pg_mblen function safe to call with broken encoding at the end
of buffer? What if last byte of the buffer is 0xF0 and you call pg_mblen for it?
>+ copy_bytes = p - s;
>+ if(copy_bytes > max_copy_bytes)
>+ copy_bytes = max_copy_bytes;
Here I would suggest to add comment about broken utf encoding case. That would
explain why we might come to situation when we can try to copy more than we
have.
I would also suggest to issue a warning here. I guess person that uses
postgres would prefer to know that he managed to stuff into postgres a string
with broken utf encoding, before it comes to some terrible consequences.
> Hi Spyridon,
>
> > The column "single_byte_col" is supposed to store only 1 byte.
> > Nevertheless, the INSERT command implicitly casts the '🀆' text into
> > "char". This means that only the first byte of '🀆' ends up stored in the
> > column. gdb reports that "pg_mblen(p) = 4" (line 1046), which is expected
> > since the pg_mblen('🀆') is indeed 4. Later at line 1050, the memcpy will
> > copy 4 bytes instead of 1, hence an out of bounds memory read happens for
> > pointer 's', which effectively copies random bytes.
> Many thanks for reporting this!
>
> > - OS: Ubuntu 20.04
> > - PSQL version 14.4
>
> I can confirm the bug exists in the `master` branch as well and
> doesn't depend on the platform.
>
> Although the bug is easy to fix for this particular case (see the
> patch) I'm not sure if this solution is general enough. E.g. is there
> something that generally prevents pg_mblen() from doing out of bound
> reading in cases similar to this one? Should we prevent such an INSERT
> from happening instead?
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
signature.asc
Description: This is a digitally signed message part.
Re: The "char" type versus non-ASCII characters
В письме от пятница, 3 декабря 2021 г. 22:12:10 MSK пользователь Tom Lane написал: > which > is that the "char" type is not very encoding-safe. charout() for > example just regurgitates the single byte as-is. I think we deemed > that okay the last time anyone thought about it, but that was when > single-byte encodings were the mainstream usage for non-ASCII data. > If you're using UTF8 or another multi-byte server encoding, it's > quite easy to get an invalidly-encoded string this way, which at > minimum is going to break dump/restore scenarios. As I've mentioned in another thread I've been very surprised when I first saw "char" type name. And I was also very confused. This leads me to an idea that may be as we fix "char" behaviour, we should also change it's name to something more speaking for itself. Like ascii_char or something like that. Or better to add ascii_char with behaviour we need, update system tables with it, and keep "char" with old behaviour in "deprecated" status in the case somebody still using it. To give them time to change it to something more decent: ascii_char or char(1). I've also talked to a guy who knows postgres history very well, he told me that "char" existed at least from portgres version 3.1, it also had "char16", and in v.4 "char2", "char4", "char8" were added. But later on they was all removed, and we have only "char". Aslo "char" has nothing in common with SQL standard. Actually it looks very unnaturally. May be it is time to get rid of it too, if we are changing this part of code... > I can think of at least three ways we might address this: > > * Forbid all non-ASCII values for type "char". This results in > simple and portable semantics, but it might break usages that > work okay today. > > * Allow such values only in single-byte server encodings. This > is a bit messy, but it wouldn't break any cases that are not > problematic already. > > * Continue to allow non-ASCII values, but change charin/charout, > char_text, etc so that the external representation is encoding-safe > (perhaps make it an octal or decimal number). This will give us steady #1 for ascii_char, and deprecation and removal of "char" later on. -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su signature.asc Description: This is a digitally signed message part.
Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions
В письме от среда, 14 июля 2021 г. 15:09:12 MSK пользователь vignesh C
написал:
> The patch does not apply on Head anymore, could you rebase and post a
> patch. I'm changing the status to "Waiting for Author".
Thank you for notification.
I've tried to rebase it and found out that some options have been added to
partitioned table.
Handling this needs careful approach, and I will fix it two weeks later, when I
am back from vacations.
Meanwhile I would strongly suggest to change
{"vacuum_index_cleanup", RELOPT_TYPE_BOOL,
to
{"vacuum_index_cleanup", RELOPT_TYPE_ENUM,
in src/backend/access/common/reloptions.c
This change should be done in 3499df0d
But current implementation of reloptions is very error prone , and it is very
easy to miss this part.
--
Nikolay Shaplov
dh...@nataraj.su (e-mail, jabber)
@dhyan:nataraj.su (matrix)
Re: POC, WIP: OR-clause support for indexes
Hi! Let me join the review process. I am no expert in execution plans, so there would not be much help in doing even better optimization. But I can read the code, as a person who is not familiar with this area and help making it clear even to a person like me. So, I am reading v25-0001-Transform-OR-clauses-to-ANY-expression.patch that have been posted some time ago, and especially transform_or_to_any function. > @@ -38,7 +45,6 @@ > int from_collapse_limit; > int join_collapse_limit; > > - > /* > * deconstruct_jointree requires multiple passes over the join tree, > because we > * need to finish computing JoinDomains before we start distributing quals. Do not think that removing empty line should be part of the patch > + /* > + * If the const node's (right side of operator > expression) type > + * don't have “true†array type, then we cannnot do > the > + * transformation. We simply concatenate the expression > node. > + */ Guess using unicode double quotes is not the best idea here... Now to the first part of `transform_or_to_any` signature.asc Description: This is a digitally signed message part.
Re: POC, WIP: OR-clause support for indexes
Hi! Let me join the review process. I am no expert in execution plans, so there would not be much help in doing even better optimization. But I can read the code, as a person who is not familiar with this area and help making it clear even to a person like me. So, I am reading v25-0001-Transform-OR-clauses-to-ANY-expression.patch that have been posted some time ago, and especially transform_or_to_any function. > @@ -38,7 +45,6 @@ > int from_collapse_limit; > int join_collapse_limit; > > - > /* > * deconstruct_jointree requires multiple passes over the join tree, because we > * need to finish computing JoinDomains before we start distributing quals. Do not think that removing empty line should be part of the patch > + /* > + * If the const node's (right side of operator expression) type > + * don't have “true†array type, then we cannnot do the > + * transformation. We simply concatenate the expression node. > + */ Guess using unicode double quotes is not the best idea here... Now to the first part of `transform_or_to_any`: > + List *entries = NIL; I guess the idea of entries should be explained from the start. What kind of entries are accomulated there... I see they are added there all around the code, but what is the purpose of that is not quite clear when you read it. At the first part of `transform_or_to_any` function, you costanly repeat two lines, like a mantra: > + entries = lappend(entries, rinfo); > + continue; "If something is wrong -- do that mantra" From my perspective, if you have to repeat same code again and again, then most probably you have some issues with architecture of the code. If you repeat some code again and again, you need to try to rewrite the code, the way, that part is repeated only once. In that case I would try to move the most of the first loop of `transform_or_to_any` to a separate function (let's say its name is prepare_single_or), that will do all the checks, if this or is good for us; return NULL if it does not suits our purposes (and in this case we do "entries = lappend(entries, rinfo); continue" in the main code, but only once) or return pointer to some useful data if this or clause is good for our purposes. This, I guess will make that part more clear and easy to read, without repeating same "lappend mantra" again and again. Will continue digging into the code tomorrow. P.S. Sorry for sending partly finished email. Pressed Ctrl+Enter accidentally... With no way to make it back :-((( signature.asc Description: This is a digitally signed message part.
Re: POC, WIP: OR-clause support for indexes
В письме от понедельник, 24 июня 2024 г. 23:51:56 MSK пользователь Nikolay Shaplov написал: So, I continue reading the patch. I see there is `entries` variable in the code, that is the list of `RestrictInfo` objects and `entry` that is `OrClauseGroup` object. This naming is quite misguiding (at least for me). `entries` variable name can be used, as we deal only with RestrictInfo entries here. It is kind of "generic" type. Though naming it `restric_info_entry` might make te code more readable. But when we come to an `entry` variable, it is very specific entry, it should be `OrClauseGroup` entry, not just any entry. So I would suggest to name this variable `or_clause_group_entry`, or even `or_clause_group` , so when we meet this variable in the middle of the code, we can get the idea what we are dealing with, without scrolling code up. Variable naming is very important thing. It can drastically improve (or ruin) code readability. Also I see some changes in the tests int this patch. There are should be tests that check that this new feature works well. And there are test whose behavior have been just accidentally affected. I whould suggest to split these tests into two patches, as they should be reviewed in different ways. Functionality tests should be thoroughly checked that all stuff we added is properly tested, and affected tests should be checked that nothing important is not broken. It would be more easy to check if these are two different patches. I would also suggest to add to the commit message of affected tests changes some explanation why this changes does not really breaks anything. This will do the checking more simple. To be continued. -- Nikolay Shaplov aka Nataraj Fuzzing Engineer at Postgres Professional Matrix IM: @dhyan:nataraj.su signature.asc Description: This is a digitally signed message part.
Re: POC, WIP: OR-clause support for indexes
В письме от среда, 17 июля 2024 г. 22:36:19 MSK пользователь Alexander
Korotkov написал:
Hi All!
I am continue reading the patch, now it's newer version
First main question:
As far a I can get, the entry point for OR->ANY convertation have been moved
to match_clause_to_indexcol funtion, that checks if some restriction can use
index for performance.
The thing I do not understand what match_clause_to_indexcol actually received
as arguments. Should this be set of expressions with OR in between grouped by
one of the expression argument?
If not I do not understand how this ever should work.
The rest is about code readability
> + if (bms_is_member(index->rel->relid, rinfo->right_relids))
> + return NULL;
This check it totally not obvious for person who is not deep into postgres
code. There should go comment explaining what are we checking for, and why it
does not suit our purposes
> + foreach(lc, orclause->args)
> + {
Being no great expert in postgres code, I am confused what are we iterating on
here? Two arguments of OR statement? (a>1) OR (b>2) those in brackets? Or
what? Comment explaining that would be a great help here.
> +if (sub_rinfo->is_pushed_down != rinfo->is_pushed_down ||
> + sub_rinfo->is_clone != rinfo->is_clone ||
> + sub_rinfo->security_level != rinfo->security_level ||
> + !bms_equal(sub_rinfo->required_relids, rinfo->required_relids) ||
> + !bms_equal(sub_rinfo->incompatible_relids, rinfo-
incompatible_relids) ||
> + !bms_equal(sub_rinfo->outer_relids, rinfo->outer_relids))
> + {
This check it totally mind-blowing... What in the name of existence is going
on here?
I would suggest to split these checks into parts (compiler optimizer should
take care about overhead) and give each part a sane explanation.
--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions
В письме от четверг, 2 июля 2020 г. 17:15:13 MSK пользователь Daniel
Gustafsson написал:
> > A new version of the patch.
> > Autovacuum options were extended in b07642db
> >
> > So I added that options to the current patch.
>
> The heapam.c hunk in this version fails to apply to HEAD, can you please
> submit a rebased version?
Thanks for reminding about it.
Here goes a rebased version.
--
Software Developer: https://www.upwork.com/freelancers/~014a87e140ff02c0da
Body-oriented Therapist: https://vk.com/nataraj_rebalancing (Russian)diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 8ccc228..6118b55 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -46,9 +46,8 @@
* upper and lower bounds (if applicable); for strings, consider a validation
* routine.
* (ii) add a record below (or use add__reloption).
- * (iii) add it to the appropriate options struct (perhaps StdRdOptions)
- * (iv) add it to the appropriate handling routine (perhaps
- * default_reloptions)
+ * (iii) add it to the appropriate options struct
+ * (iv) add it to the appropriate handling routine
* (v) make sure the lock level is set correctly for that operation
* (vi) don't forget to document the option
*
@@ -1375,9 +1374,11 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
switch (classForm->relkind)
{
case RELKIND_RELATION:
- case RELKIND_TOASTVALUE:
case RELKIND_MATVIEW:
- options = heap_reloptions(classForm->relkind, datum, false);
+ options = heap_reloptions(datum, false);
+ break;
+ case RELKIND_TOASTVALUE:
+ options = toast_reloptions(datum, false);
break;
case RELKIND_PARTITIONED_TABLE:
options = partitioned_table_reloptions(datum, false);
@@ -1811,59 +1812,66 @@ fillRelOptions(void *rdopts, Size basesize,
/*
- * Option parser for anything that uses StdRdOptions.
+ * Option parsing definition for autovacuum. Used in toast and heap options.
+ */
+
+#define AUTOVACUUM_RELOPTIONS(OFFSET)\
+ {"autovacuum_enabled", RELOPT_TYPE_BOOL, \
+ OFFSET + offsetof(AutoVacOpts, enabled)},\
+ {"autovacuum_vacuum_threshold", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_threshold)}, \
+ {"autovacuum_vacuum_insert_threshold", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_ins_threshold)}, \
+ {"autovacuum_analyze_threshold", RELOPT_TYPE_INT,\
+ OFFSET + offsetof(AutoVacOpts, analyze_threshold)}, \
+ {"autovacuum_vacuum_cost_delay", RELOPT_TYPE_REAL, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_cost_delay)}, \
+ {"autovacuum_vacuum_cost_limit", RELOPT_TYPE_INT,\
+ OFFSET + offsetof(AutoVacOpts, vacuum_cost_limit)}, \
+ {"autovacuum_freeze_min_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_min_age)}, \
+ {"autovacuum_freeze_max_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_max_age)}, \
+ {"autovacuum_freeze_table_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_table_age)}, \
+ {"autovacuum_multixact_freeze_min_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, multixact_freeze_min_age)}, \
+ {"autovacuum_multixact_freeze_max_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, multixact_freeze_max_age)}, \
+ {"autovacuum_multixact_freeze_table_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, multixact_freeze_table_age)}, \
+ {"log_autovacuum_min_duration", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, log_min_duration)}, \
+ {"autovacuum_vacuum_scale_factor", RELOPT_TYPE_REAL, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_scale_factor)},\
+ {"autovacuum_vacuum_insert_scale_factor", RELOPT_TYPE_REAL, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_ins_scale_factor)}, \
+ {"autovacuum_analyze_scale_factor", RELOPT_TYPE_REAL,\
+ OFFSET + offsetof(AutoVacOpts, analyze_scale_factor)}
+/*
+ * Option parser for heap
*/
bytea *
-default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
+heap_reloptions(Datum reloptions, bool validate)
{
static const relopt_parse_elt tab[] = {
- {"fillfactor", RELOPT_TYPE_INT, offsetof(StdRdOptions, fillfactor)},
- {"autovacuum_enabled", RELOPT_TYPE_BOOL,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, enabled)},
- {"autovacuum_vacuum_threshold", RELOPT_TYPE_INT,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, vacuum_threshold)},
- {"autovacuum_vacuum_insert_threshold", RELOPT_TYPE_INT,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, vacuum_ins_threshold)},
- {"autovacuum_analyze_threshold", RELOPT_TYPE_INT,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, analyze_threshold)},
-
Re: [PATCH] get rid of StdRdOptions, use individual binary reloptions representation for each relation kind instead
В письме от четверг, 17 января 2019 г. 20:33:06 MSK пользователь Alvaro
Herrera написал:
> You introduced new macros IsHeapRelation and IsViewRelation, but I don't
> want to introduce such API. Such things have been heavily contested and
> I don't to have one more thing to worry about for this patch, so please
> just put the relkind directly in the code.
Sorry.
I've been trying to avoid repeating
(AssertMacro(relation->rd_rel->relkind == RELKIND_RELATION || \
relation->rd_rel->relkind == RELKIND_MATVIEW), \
all the time.
Fixed.
Also I make more relaxed parallel_workers behavior in get_relation_info as we
discussed in another thread.diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index cdf1f4a..b1fd67c 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -22,7 +22,7 @@
#include "access/htup_details.h"
#include "access/nbtree.h"
#include "access/reloptions.h"
-#include "access/spgist.h"
+#include "access/spgist_private.h"
#include "access/tuptoaster.h"
#include "catalog/pg_type.h"
#include "commands/defrem.h"
@@ -46,9 +46,8 @@
* upper and lower bounds (if applicable); for strings, consider a validation
* routine.
* (ii) add a record below (or use add__reloption).
- * (iii) add it to the appropriate options struct (perhaps StdRdOptions)
- * (iv) add it to the appropriate handling routine (perhaps
- * default_reloptions)
+ * (iii) add it to the appropriate options struct
+ * (iv) add it to the appropriate handling routine
* (v) make sure the lock level is set correctly for that operation
* (vi) don't forget to document the option
*
@@ -1010,7 +1009,7 @@ extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
case RELKIND_TOASTVALUE:
case RELKIND_MATVIEW:
case RELKIND_PARTITIONED_TABLE:
- options = heap_reloptions(classForm->relkind, datum, false);
+ options = relation_reloptions(classForm->relkind, datum, false);
break;
case RELKIND_VIEW:
options = view_reloptions(datum, false);
@@ -1343,63 +1342,69 @@ fillRelOptions(void *rdopts, Size basesize,
/*
- * Option parser for anything that uses StdRdOptions.
+ * Option parsing definition for autovacuum. Used in toast and heap options.
+ */
+
+#define AUTOVACUUM_RELOPTIONS(OFFSET)\
+ {"autovacuum_enabled", RELOPT_TYPE_BOOL, \
+ OFFSET + offsetof(AutoVacOpts, enabled)},\
+ {"autovacuum_vacuum_threshold", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_threshold)}, \
+ {"autovacuum_analyze_threshold", RELOPT_TYPE_INT,\
+ OFFSET + offsetof(AutoVacOpts, analyze_threshold)}, \
+ {"autovacuum_vacuum_cost_delay", RELOPT_TYPE_INT,\
+ OFFSET + offsetof(AutoVacOpts, vacuum_cost_delay)}, \
+ {"autovacuum_vacuum_cost_limit", RELOPT_TYPE_INT,\
+ OFFSET + offsetof(AutoVacOpts, vacuum_cost_limit)}, \
+ {"autovacuum_freeze_min_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_min_age)}, \
+ {"autovacuum_freeze_max_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_max_age)}, \
+ {"autovacuum_freeze_table_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, freeze_table_age)}, \
+ {"autovacuum_multixact_freeze_min_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, multixact_freeze_min_age)}, \
+ {"autovacuum_multixact_freeze_max_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, multixact_freeze_max_age)}, \
+ {"autovacuum_multixact_freeze_table_age", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, multixact_freeze_table_age)}, \
+ {"log_autovacuum_min_duration", RELOPT_TYPE_INT, \
+ OFFSET + offsetof(AutoVacOpts, log_min_duration)}, \
+ {"autovacuum_vacuum_scale_factor", RELOPT_TYPE_REAL, \
+ OFFSET + offsetof(AutoVacOpts, vacuum_scale_factor)},\
+ {"autovacuum_analyze_scale_factor", RELOPT_TYPE_REAL,\
+ OFFSET + offsetof(AutoVacOpts, analyze_scale_factor)}
+
+/*
+ * Option parser for heap
*/
bytea *
-default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
+heap_reloptions(Datum reloptions, bool validate)
{
relopt_value *options;
- StdRdOptions *rdopts;
+ HeapRelOptions *rdopts;
int numoptions;
static const relopt_parse_elt tab[] = {
- {"fillfactor", RELOPT_TYPE_INT, offsetof(StdRdOptions, fillfactor)},
- {"autovacuum_enabled", RELOPT_TYPE_BOOL,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, enabled)},
- {"autovacuum_vacuum_threshold", RELOPT_TYPE_INT,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, vacuum_threshold)},
- {"autovacuum_analyze_threshold", RELOPT_TYPE_INT,
- offsetof(StdRdOptions, autovacuum) + offsetof(AutoVacOpts, analyze_threshold)},
- {"autovacuum_
Re: Ltree syntax improvement
В письме от вторник, 29 января 2019 г. 20:43:07 MSK пользователь Dmitry
Belyavsky написал:
> Please find attached the patch extending the sets of symbols allowed in
> ltree labels. The patch introduces 2 variants of escaping symbols, via
> backslashing separate symbols and via quoting the labels in whole for
> ltree, lquery and ltxtquery datatypes.
Hi!
Let's me join the review process...
I've just give a superficial look to the patch, read it through, and try here
and there, so first I'll give feedback about exterior features of the patch.
So, the first question: why do we need all this? The answer seems to be
obvious, but it would be good say it explicitly. What problems would it solve?
Do these problems really exists?
The second question is about coding style. As far as I can see you've passed
your code through pg_indent, but it does not solve all problems.
As far as I know, postgres commiters are quite strict about code width, the
code should not be wider that 80 col, except places were string constants are
introduced (There you can legally exceed 80 col). So I would advice to use vim
with tabstop=4 and colorcolumn=80 and make sure that new code text does not
cross red vertical line.
Comments. There is no fixed coding style for comments in postgres. Usually this
means that it is better to follow coding in the code around. But ltree does
not have much comments, so I would suggest to use the best style I've seen in
the code
/*
* [function name]
* < tab > [Short description, what does it do]
*
* [long explanation how it works, several subparagraph if needed
*/
(Seen this in src/include/utils/rel.h)
or something like that.
At least it would be good to have explicit separation between descriptions and
explanation of how it works.
And about comment
/*
* Function calculating bytes to escape
* to_escape is an array of "special" 1-byte symbols
* Behvaiour:
* If there is no "special" symbols, return 0
* If there are any special symbol, we need initial and final quote, so return
2
* If there are any quotes, we need to escape all of them and also initial and
final quote, so
* return 2 + number of quotes
*/
It has some formatting. But it should not. As far as I understand this
comment should be treated as single paragraph by reformatter, and refomatted.
I do not understand why pg_indent have not done it. Documentation (https://
www.postgresql.org/docs/11/source-format.html) claims that if you want to
avoid reformatting, you should add "-" at the beginning and at the
end of the comment. So it would be good either remove this formatting, or add
"" if you are sure you want to keep it.
Third part is about error messages.
First I've seen errors like elog(ERROR, "internal error during splitting
levels");. I've never seen error like that in postgres. May be if this error
is in the part of the code, that should never be reached in real live, may be
it would be good to change it to Assert? Or if this code can be normally
reached, add some more explanation of what had happened...
About other error messages.
There are messages like
errmsg("syntax error"),
errdetail("Unexpected end of line.")));
or
errmsg("escaping syntax error")));
In postgres there is error message style guide
https://www.postgresql.org/docs/11/error-style-guide.html
Here I would expect messages like that
Primary:Error parsing ltree path
Detail: Curly quote is opened and not closed
Or something like that, I am not expert in English, but this way it would be
better for sure. Key points are: Primary message should point to general area
where error occurred (there can be a large SQL with a lot of things in it, so
it is good to point that problem is with ltree staff). And is is better to
word from the point of view of a user. What for you (and me) is unexpected end
of line, for him it is unclosed curly quote.
Also I am not sure, if it is easy, but if it is possible, it would be good to
move "^" symbol that points to the place of the error, to the place inside ' '
where unclosed curly quote is located. As a user I would appreciate that.
So that's what I've seen while giving it superficial look...
Re: [PATCH][PROPOSAL] Add enum releation option type
В письме от среда, 20 февраля 2019 г. 15:08:32 MSK пользователь Michael Paquier написал: > On Sun, Jan 06, 2019 at 06:28:11PM +0300, Nikolay Shaplov wrote: > > Actually I am not satisfied with it too... That is why I started that > > bigger reloptions refactoring work. So for now I would offer to keep > > initialization as it was for other types: in initialize_reloptions using > > [type]RelOpts[] arrays. And then I will offer patch that changes it for > > all types and remove difference between biult-in reloptions and > > reloptions in extensions. > Should this be switched as waiting on author or just marked as > returned with feedback then? Actually I would prefer "Commited" ;-) And speaking seriously... Anyway, this test module in src/test/modules should be a separate patch, as it would test that all types of options are properly reaching index extension, not only enum. From my point of view, it can be committed without any dependency with enum reloption. Either we first commit this patch, and then that test module will test that enum support, or first we commit that test moudule without testing enum support, and add tests into this enum patch. I would prefer to commit enum first, and I would promise that I will add thus module to the top of my dev TODO list. If it is not ok for you, then please tell me about it and set this patch as "Waiting for author" and I will do the test patch first. > > 2.5 May be this src/test/modules dummy index is subject to another patch. > > So I will start working on it right now, but we will do this work not > > dependent to any other patches. And just add there what we need when it > > is ready. I would actually add there some code that checks all types of > > options, because we actually never check that option value from reloptons > > successfully reaches extension code. I did this checks manually, when I > > wrote that bigger reloption patch, but there is no facilities to do > > regularly check is via test suit. Creating dummy index will create such > > facilities. > > bloom is a user-facing extension, while src/test/modules are normally > not installed (there is an exception for MSVC anyway..), so stressing > add_enum_reloption in src/test/modules looks more appropriate to me, > except if you can define an option which is useful for the user and is > an enum. Thank you for pointing me right direction. 've been waiting for it. Now I can go on :)) So let's it be src/test/modules module...
Re: Ltree syntax improvement
В письме от вторник, 29 января 2019 г. 20:43:07 MSK пользователь Dmitry
Belyavsky написал:
> Dear all,
>
> Please find attached the patch extending the sets of symbols allowed in
> ltree labels. The patch introduces 2 variants of escaping symbols, via
> backslashing separate symbols and via quoting the labels in whole for
> ltree, lquery and ltxtquery datatypes.
Sorry I still did not do full revision, I've stumbled up on other things in
the code, which, as I think should be fixed before final checking through. (I
guess that code do what is expected, since it passes tests and so on, it needs
recheck, but I'd like to do this full recheck only once)
So my doubts about this code is that a lot parts of code is just a copy-paste
of the same code that was written right above. And it can be rewritten in a
way that the same lines (or parts of lines) is repeated only once...
This should make code more readable, and make code more similar to the code of
postgres core, I've seen.
Here I will speak about lquery_in function from ltree_io.c.
1. May be it is better here to switch from else if (state ==
LQPRS_[something]) to switch (stat) { case LQPRS_[something]: }
because because here the main case is to determine what is the current state
of your state machine, do something with it, and then go to the next step.
Now after you've done what should be done in this step, you should make sure
that no other code is executed writing more ifs... If you use switch/case you
will be able to say break wherever you like, it will save you some ifs.
Theoretical example
(cl is for character length fc is for fist char)
if (cl==1 && fc =='1') {do_case1();}
else if (cl==1 && fc =='2') {do_case2();}
else if (cl==1 && fc =='3') {do_case3();}
else {do_otherwise();}
If it is wrapped in switch/case it will be like
if (cl==1)
{
if (fc =='1') {do_case1(); break;}
if (fc =='2') {do_case2(); break;}
if (fc =='3') {do_case3(); break;}
}
/*if nothing is found */
do_otherwise();
This for example will allow you to get rid of multiply charlen == 1 checks in
else if ((lptr->flag & LVAR_QUOTEDPART) == 0)
{
if (charlen == 1 && t_iseq(ptr, '@'))
{
if (lptr->start == ptr)
UNCHAR;
lptr->flag |= LVAR_INCASE;
curqlevel->flag |= LVAR_INCASE;
}
else if (charlen == 1 && t_iseq(ptr, '*'))
{
if (lptr->start == ptr)
UNCHAR;
lptr->flag |= LVAR_ANYEND;
curqlevel->flag |= LVAR_ANYEND;
[PATCH] check for ctags utility in make_ctags
Hi! I'd like to propose a small patch for make_ctags script. It checks if ctags utility is intalled or not. If not it reports an error and advises to install ctags. This will make life of a person that uses ctags first time in his life a bit easier. I use command -v to detect if ctags command exists. It is POSIX standard and I hope it exist in all shells.diff --git a/src/tools/make_ctags b/src/tools/make_ctags index 1609c07..edbcd82 100755 --- a/src/tools/make_ctags +++ b/src/tools/make_ctags @@ -2,6 +2,13 @@ # src/tools/make_ctags +if [ ! $(command -v ctags) ] +then + echo "'ctags' utility is not found" &1>2 + echo "Please install 'ctags' to run make_ctags" &1>2 + exit 1 +fi + trap "rm -f /tmp/$$" 0 1 2 3 15 rm -f ./tags
