Re: [HACKERS] Extra check in 9.0 exclusion constraint unintended consequences
Excerpts from Jeff Davis's message of vie jul 08 00:58:20 -0400 2011: > On Thu, 2011-07-07 at 12:36 -0400, Robert Haas wrote: > > I think it's probably too late to go fiddling with the behavior of 9.0 > > at this point. If we change the text of error messages, there is a > > chance that it might break applications; it would also require those > > messages to be re-translated, and I don't think the issue is really > > important enough to justify a change. > > Good point on the error messages -- I didn't really think of that as a > big deal. > > > I am happy to see us document > > it better, though, since it's pretty clear that there is more > > likelihood of hitting that error than we might have suspected at the > > outset. > > Doc patch attached, but I'm not attached to the wording. Remember that > we only need to update the 9.0 docs, I don't think you want to apply > this to master (though I'm not sure how this kind of thing is normally > handled). Is this really a good idea? I think the note should still be there in 9.1 and beyond (with the version applicability note of course) -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Need help understanding pg_locks
Can someone help me understand pg_locks? There are three fields related to virtual and real xids: virtualtransaction | text | transactionid | xid | virtualxid | text | Our docs say 'virtualtransaction' is: Virtual ID of the transaction that is holding or awaiting this lock This field was clear to me. and 'transactionid' is documented as: ID of a transaction, or null if the object is not a transaction ID In my testing it was the (non-virtual) xid of the lock holder. Is that correct? Can it be a waiter? 'virtualxid' is documented as: Virtual ID of a transaction, or null if the object is not a virtual transaction ID In my testing this field is for locking your own vxid, meaning it owned by its own vxid. I looked at the C code in /pg/backend/utils/adt/lockfuncs.c and was confused. Clearly our documentation is lacking in this area and I would like to clarify it. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] per-column generic option
Shigeru Hanada escribió:
> (2011/06/26 18:34), Kohei KaiGai wrote:
> > I checked your patch.
>
> Thanks for the review! Please find attached a revised patch.
Err, \dec seems to have a line in describe.h but nowhere else; are you
going to introduce that command?
The new ALTER TABLE grammar seems a bit strange -- ADD, SET, DROP. Is
this defined by the SQL/MED standard? It seems at odds with our
handling of attoptions (and the pg_dump query seems rather bizarre in
comparison to the handling of attoptions there; why do we need
pg_options_to_table when attoptions do not?).
What's the reason for this:
@@ -3681,7 +3691,7 @@ AlterFdwStmt: ALTER FOREIGN DATA_P WRAPPER name
opt_fdw_options alter_generic_op
/* Options definition for CREATE FDW, SERVER and USER MAPPING */
create_generic_options:
OPTIONS '(' generic_option_list ')' { $$ = $3; }
- | /*EMPTY*/ { $$ = NIL; }
+ | /*EMPTY*/ { $$ = NIL }
;
I think this should be removed:
+ foreach (clist, column->fdwoptions)
+ {
+ DefElem*option = (DefElem *) lfirst(clist);
+ elog(DEBUG3, "%s=%s", option->defname, strVal(option->arg));
+ }
As for whether attfdwoptions needs to be separate from attoptions, I am
not sure that they really need to be; but if they are, I think we should
use a different name than attfdwoptions, because we might want to use
them for something else. Maybe attgenoptions (and note that the alter
table code already calls them "generic options" so I'm not sure why the
rest of the code is calling them FDW options) ... but then I really
start to question whether they need to be separate from attoptions.
Currently, attoptions are used to store n_distinct and
n_distinct_inherited. Do those options make sense for foreign tables?
If they do make sense for some types of foreign servers, maybe we should
decree that they need to be specifically declared as such by the FDW --
that is, the FDW needs to provide its own attribute_reloptions routine
(or equivalent therein) for validation that includes those core options.
There is no saying that, even if all existing core options such as
n_distinct apply to a FDW now, more core options that we might invent in
the future are going to apply as well.
In short: in my opinion, attoptions and attfdwoptions need to be one
thing and the same.
--
Álvaro Herrera
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
Excerpts from Robert Haas's message of vie jul 08 23:30:10 -0400 2011: > On Thu, Jul 7, 2011 at 5:34 PM, Alvaro Herrera > wrote: > > The attached patch introduces pg_constraint rows for NOT NULL > > column constraints. > > > > This patch is a heavily reworked version of Bernd Helmle's patch here: > > http://archives.postgresql.org/message-id/E57A252DFD60C1FCA91731BD@amenophis > It would probably be good to add this to the next CommitFest. Not > sure about anyone else, but I'm too busy looking at patches that were > submitted in April, May, and June to look at any new ones right now. Yeah, that's what I did. -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Enhanced psql in core?
On Sat, Jul 9, 2011 at 8:14 PM, Jaime Casanova wrote: > On Sat, Jul 9, 2011 at 5:29 AM, hubert depesz lubaczewski > wrote: >> hi, >> would it be possible to incorporate >> http://www.postgres.cz/index.php/Enhanced-psql in core PostgreSQL/psql? >> >> This patch adds lots of nice functionalities, which we could definitely >> use. >> > > big part of this seems to be (based on the examples on the page, > haven't read the patch) scripting functionality but now that we have > DO, is really a need for that? > i'm not really sure if we can do what the same as your example using > DO but i'm really dubious about the usefullness of that example. > > -- > Jaime Casanova www.2ndQuadrant.com > Professional PostgreSQL: Soporte 24x7 y capacitación > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > At least it would be useful to have conditional includes... \if ... \i something.sql \endif -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Enhanced psql in core?
On Sat, Jul 9, 2011 at 5:29 AM, hubert depesz lubaczewski wrote: > hi, > would it be possible to incorporate > http://www.postgres.cz/index.php/Enhanced-psql in core PostgreSQL/psql? > > This patch adds lots of nice functionalities, which we could definitely > use. > big part of this seems to be (based on the examples on the page, haven't read the patch) scripting functionality but now that we have DO, is really a need for that? i'm not really sure if we can do what the same as your example using DO but i'm really dubious about the usefullness of that example. -- Jaime Casanova www.2ndQuadrant.com Professional PostgreSQL: Soporte 24x7 y capacitación -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Enhanced psql in core?
2011/7/9 hubert depesz lubaczewski : > hi, > would it be possible to incorporate > http://www.postgres.cz/index.php/Enhanced-psql in core PostgreSQL/psql? > > This patch adds lots of nice functionalities, which we could definitely > use. Some features are very interesting but I I would suggest to split each in a separate patch proposal. The \lf is a must have, imo. -- Cédric Villemain 2ndQuadrant http://2ndQuadrant.fr/ PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] csvlog_fields review
It bit rotted a bit find a new version attached that includes the following fixes: - show_session_authorization() no longer exists, instead access the session_authorization_guc directly (like we do for show_role in commands/variable.c). I find it quite ugly tho... - it changed %u to %U and %U to be %u... flipped it back so %u remains unchanged - num_fields in write_csvlog was unused, removed it - new_csv_fields would leak in an error path of assign_csvlog_fields (which probably matters as we are in TopMemoryContext) All of Itagaki-san's comments still stand: > * csvlog_fields and csvlog_header won't work with non-default log_filename when it doesn't contain seconds in the format. They expect they can always open empty log files. I think at the very least we should document this? Or maybe only write out the header when its a zero length file? > * The long default value for csvlog_fields leads long text line in postgresql.conf, SHOW ALL, pg_settings view, but there were no better alternative solutions in the past discussion. I think it might be worth revisiting using the %X syntax log_line_prefix uses instead of inventing our own long form names. > * csvlog_fields is marked as PGC_POSTMASTER. It can protect mixed formats in a csv file on default log_filename, but other similar GUC variables are usually marked AS PGC_SIGHUP. I don't really see this as a problem... As it stands I think we still need to address the first two questions before its ready for a -commiter. csvlog_fields_ah.patch.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] Creating temp tables inside read only transactions
Jeff Davis wrote: On Fri, 2011-07-08 at 23:39 -0700, Darren Duncan wrote: What if you used the context of the calling code and resolve in favor of whatever match is closest to it? The problem is related to general-purpose programming languages. Basically start looking in the lexical context for an "x" and if you find one use that; otherwise, assuming we're talking about referencing code that lives in the database such as a function, look at the innermost schema containing the referencing code and see if it has a direct child named "x"; otherwise go up one level to a parent schema, and so on until you get to the top, and finding none by then say it doesn't exist. This is an example of where data languages and normal programming languages have a crucial difference. With a data language, you have this problem: 1. An application uses a query referencing 'y.z.foo' that resolves to internal object with fully-qualified name 'x.y.z'. 2. An administrator creates object 'y.z.foo'. Now, the application breaks all of a sudden. In a normal prgramming language, if the schema of the two "foo"s are different, the compiler could probably catch the error. SQL really has no hope of catching it though. PostgreSQL has this problem now in a couple ways, but it's much easier to grasp what you might be conflicting with. If you have multiple nested levels to traverse and different queries using different levels of qualification, it gets a little more messy and I think a mistake is more likely. Well, my search path suggestion was based on Tom Lane's comment that "the SQL spec requires us to be able to [support abbreviations]" and I expected it would be syntactically and semantically backwards compatible with how things work now. FYI, with Muldis D, being more green fields, there are no search paths in the general case, and every entity reference is unambiguous because it has to be fully-qualified. However, I also support relative references, and in fact require their use for references within the same database, which carries a number of benefits, at the cost of being a few characters more verbose than when using a search path. So introducing new things with the same names in different namespaces won't break anything there, even if they are "closer". Its essentially like navigating a Unix filesystem but with "." rather than "/". So for example, if you had 2 sibling schemas "s1" and "s2", each with 2 functions "f1","f2" and a table "t", then s1.f1 would reference s1.f2 and s1.t as sch.lib.f2 and sch.data.t respectively, while s1.f1 would refer to the entities in s2 as sch.par.s2.lib.f1 and sch.par.s2.data.t and such (a function can also refer to itself anonymously as "rtn" if it's recursive). The "sch" is like "." in Unix and the "par" is like ".." in Unix. The "data" is for data tables or views (and "cat" is for catalog tables/views) while "lib" is for user-defined types, routines, constraints, etc (and "sys" is for built-in types and routines, but "sys" may be omitted and search paths exist just for built-ins). Synonyms are also supported. I don't expect you would adopt relative (fully-qualified) references, because the syntax isn't in standard SQL (I think), but I did. Unless you like them and can come up with a syntax that will fit into how SQL does things. -- Darren Duncan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] cataloguing NOT NULL constraints
On tor, 2011-07-07 at 17:34 -0400, Alvaro Herrera wrote: > The attached patch introduces pg_constraint rows for NOT NULL > column constraints. The information schema views check_constraints and table_constraints currently make up some artificial constraint names for not-null constraints. I suppose this patch removes the underlying cause for that, so could you look into updating the information schema as well? You could probably just remove the separate union branches for not null and adjust the contype conditions. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parameterized aggregate subquery (was: Pull up aggregate subquery)
On 2011-07-09 16:23, Hitoshi Harada wrote: 2011/7/5 Hitoshi Harada: 2011/7/5 Yeb Havinga: Hello Hitosh, list, Attached is revised version. I failed to attached the patch. I'm trying again. I'm currently unable to test, since on holiday. I'm happy to continue testing once returned but it may not be within the bounds of the current commitfest, sorry. Thanks for replying. Regarding the time remained until the end of this commitfest, I'd think we should mark this item "Returned with Feedback" if no objection appears. I will be very happy if Yeb tests my latest patch after he comes back. I'll make up my mind around regression test. It seems that Yeb marked "Returned with Feedback". Thanks for the review again. Still, I'd appreciate if further review is done on my latest patch. Yes, I did so after you suggested to return it to feedback. I'll review your latest patch as soon as there is enough time to do a proper review, which is probably after next week. regards, Yeb Havinga -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Extra check in 9.0 exclusion constraint unintended consequences
On Fri, 2011-07-08 at 22:51 -0400, Robert Haas wrote: > I'm wondering if we might want to call this out with a or > similar... especially if we're only going to put it into the 9.0 > docs. Sure, sounds good. Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] Creating temp tables inside read only transactions
On Fri, 2011-07-08 at 23:39 -0700, Darren Duncan wrote: > What if you used the context of the calling code and resolve in favor of > whatever match is closest to it? The problem is related to general-purpose > programming languages. > > Basically start looking in the lexical context for an "x" and if you find one > use that; otherwise, assuming we're talking about referencing code that lives > in > the database such as a function, look at the innermost schema containing the > referencing code and see if it has a direct child named "x"; otherwise go up > one > level to a parent schema, and so on until you get to the top, and finding > none > by then say it doesn't exist. This is an example of where data languages and normal programming languages have a crucial difference. With a data language, you have this problem: 1. An application uses a query referencing 'y.z.foo' that resolves to internal object with fully-qualified name 'x.y.z'. 2. An administrator creates object 'y.z.foo'. Now, the application breaks all of a sudden. In a normal prgramming language, if the schema of the two "foo"s are different, the compiler could probably catch the error. SQL really has no hope of catching it though. PostgreSQL has this problem now in a couple ways, but it's much easier to grasp what you might be conflicting with. If you have multiple nested levels to traverse and different queries using different levels of qualification, it gets a little more messy and I think a mistake is more likely. Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch: add GiST support for BOX @> POINT queries
2011/6/19 Hitoshi Harada : > 2011/6/17 Andrew Tipton : >> >> At this point I'm a bit lost -- while pg_amop.h has plenty of examples >> of crosstype comparison operators for btree index methods, there are >> none for GiST. Is GiST somehow a special case in this regard? > > It was I that was lost. As Tom mentioned, GiST indexes have records in > pg_amop in their specialized way. I found gist_point_consistent has > some kind of hack for that and pg_amop for point_ops records have > multiple crosstype for that. So, if I understand correctly your first > approach modifying gist_box_consistent was the right way, although > trivial issues should be fixed. Also, you may want to follow point_ops > when you are worried if the counterpart operator of commutator should > be registered or not. > > Looking around those mechanisms, it occurred to me that you mentioned > only box @> point. Why don't you add circly @> point, poly @> point as > well as box? Is that hard? > It looks like the time to wrap up. I marked "Return with Feedback" on this patch, since response from author has not come for a while. You may think the fix was pretty easy and the patch be small, but more general approach was preferred, I guess. Looking forward to seeing it in better shape next time! Thanks, -- Hitoshi Harada -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parameterized aggregate subquery (was: Pull up aggregate subquery)
2011/7/5 Hitoshi Harada : > 2011/7/5 Yeb Havinga : >> Hello Hitosh, list, >> >>> > >>> > Attached is revised version. >>> >>> I failed to attached the patch. I'm trying again. >>> >> I'm currently unable to test, since on holiday. I'm happy to continue >> testing once returned but it may not be within the bounds of the current >> commitfest, sorry. > > Thanks for replying. Regarding the time remained until the end of this > commitfest, I'd think we should mark this item "Returned with > Feedback" if no objection appears. I will be very happy if Yeb tests > my latest patch after he comes back. I'll make up my mind around > regression test. It seems that Yeb marked "Returned with Feedback". Thanks for the review again. Still, I'd appreciate if further review is done on my latest patch. Thanks, -- Hitoshi Harada -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Enhanced psql in core?
hi, would it be possible to incorporate http://www.postgres.cz/index.php/Enhanced-psql in core PostgreSQL/psql? This patch adds lots of nice functionalities, which we could definitely use. Best regards, depesz -- The best thing about modern society is how easy it is to avoid contact with it. http://depesz.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
On Sat, Jul 09, 2011 at 10:52:33AM +0200, Kohei KaiGai wrote:
> 2011/7/9 Noah Misch :
> > On Sat, Jul 09, 2011 at 09:00:30AM +0200, Kohei KaiGai wrote:
> >> The attached patch is a revised version according to the approach that
> >> updates
> >> pg_class system catalog before AlterTableInternal().
> >> It invokes the new ResetViewOptions when rel->rd_options is not null, and
> >> it set
> >> null on the pg_class.reloptions of the view and increments command counter.
> >
> >> + /*
> >> + * ResetViewOptions
> >> + *
> >> + * It clears all the reloptions prior to replacing
> >> + */
> >> + static void
> >> + ResetViewOptions(Oid viewOid)
> >> + {
> >> + Relation pg_class;
> >> + HeapTuple oldtup;
> >> + HeapTuple newtup;
> >> + Datum values[Natts_pg_class];
> >> + bool nulls[Natts_pg_class];
> >> + bool replaces[Natts_pg_class];
> >> +
> >> + pg_class = heap_open(RelationRelationId, RowExclusiveLock);
> >> +
> >> + oldtup = SearchSysCache1(RELOID, DatumGetObjectId(viewOid));
> >
> > Use SearchSysCacheCopy1, since you're modifying the tuple.
> >
> The heap_modify_tuple() allocates a new tuple as a copy of old tuple.
> No need to worry about.
Ah, yes. Sorry for the noise.
> >> + if (!HeapTupleIsValid(oldtup))
> >> + elog(ERROR, "cache lookup failed for relation %u", viewOid);
> >> +
> >> + memset(values, 0, sizeof(values));
> >> + memset(nulls, false, sizeof(nulls));
> >> + memset(replaces, false, sizeof(replaces));
> >> +
> >> + replaces[Anum_pg_class_reloptions - 1] = true;
> >> + nulls[Anum_pg_class_reloptions - 1] = true;
> >> +
> >> + newtup = heap_modify_tuple(oldtup, RelationGetDescr(pg_class),
> >> + values, nulls,
> >> replaces);
> >> + simple_heap_update(pg_class, &newtup->t_self, newtup);
> >> +
> >> + CatalogUpdateIndexes(pg_class, newtup);
> >> +
> >> + ReleaseSysCache(oldtup);
> >> +
> >> + heap_close(pg_class, RowExclusiveLock);
> >> +
> >> + CommandCounterIncrement();
> >
> > Why is a CCI necessary?
> >
> ATExecSetRelOptions() reference the view to be updated using syscache,
> however, this update will not become visible without CCI.
> In the result, it will reference old tuple, then get an error because
> it tries to
> update already updated tuple.
Okay, thanks.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
2011/7/9 Noah Misch :
> On Sat, Jul 09, 2011 at 09:00:30AM +0200, Kohei KaiGai wrote:
>> The attached patch is a revised version according to the approach that
>> updates
>> pg_class system catalog before AlterTableInternal().
>> It invokes the new ResetViewOptions when rel->rd_options is not null, and it
>> set
>> null on the pg_class.reloptions of the view and increments command counter.
>
>> + /*
>> + * ResetViewOptions
>> + *
>> + * It clears all the reloptions prior to replacing
>> + */
>> + static void
>> + ResetViewOptions(Oid viewOid)
>> + {
>> + Relation pg_class;
>> + HeapTuple oldtup;
>> + HeapTuple newtup;
>> + Datum values[Natts_pg_class];
>> + bool nulls[Natts_pg_class];
>> + bool replaces[Natts_pg_class];
>> +
>> + pg_class = heap_open(RelationRelationId, RowExclusiveLock);
>> +
>> + oldtup = SearchSysCache1(RELOID, DatumGetObjectId(viewOid));
>
> Use SearchSysCacheCopy1, since you're modifying the tuple.
>
The heap_modify_tuple() allocates a new tuple as a copy of old tuple.
No need to worry about.
>> + if (!HeapTupleIsValid(oldtup))
>> + elog(ERROR, "cache lookup failed for relation %u", viewOid);
>> +
>> + memset(values, 0, sizeof(values));
>> + memset(nulls, false, sizeof(nulls));
>> + memset(replaces, false, sizeof(replaces));
>> +
>> + replaces[Anum_pg_class_reloptions - 1] = true;
>> + nulls[Anum_pg_class_reloptions - 1] = true;
>> +
>> + newtup = heap_modify_tuple(oldtup, RelationGetDescr(pg_class),
>> + values, nulls,
>> replaces);
>> + simple_heap_update(pg_class, &newtup->t_self, newtup);
>> +
>> + CatalogUpdateIndexes(pg_class, newtup);
>> +
>> + ReleaseSysCache(oldtup);
>> +
>> + heap_close(pg_class, RowExclusiveLock);
>> +
>> + CommandCounterIncrement();
>
> Why is a CCI necessary?
>
ATExecSetRelOptions() reference the view to be updated using syscache,
however, this update will not become visible without CCI.
In the result, it will reference old tuple, then get an error because
it tries to
update already updated tuple.
>> + }
>
> In any event, we seem to be converging on a version of parts 0 and 1 that are
> ready for committer. However, Robert contends that this will not be committed
> separately from part 2. Unless someone wishes to contest that, I suggest we
> mark this Returned with Feedback and let the CF entry for part 2 subsume its
> future development. Does that sound reasonable?
>
At least, it seems to me we don't need to tackle to this matter from
the beginning
on the next commit fest again.
Thanks,
--
KaiGai Kohei
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
On Sat, Jul 09, 2011 at 09:00:30AM +0200, Kohei KaiGai wrote:
> The attached patch is a revised version according to the approach that updates
> pg_class system catalog before AlterTableInternal().
> It invokes the new ResetViewOptions when rel->rd_options is not null, and it
> set
> null on the pg_class.reloptions of the view and increments command counter.
> + /*
> + * ResetViewOptions
> + *
> + * It clears all the reloptions prior to replacing
> + */
> + static void
> + ResetViewOptions(Oid viewOid)
> + {
> + Relationpg_class;
> + HeapTuple oldtup;
> + HeapTuple newtup;
> + Datum values[Natts_pg_class];
> + boolnulls[Natts_pg_class];
> + boolreplaces[Natts_pg_class];
> +
> + pg_class = heap_open(RelationRelationId, RowExclusiveLock);
> +
> + oldtup = SearchSysCache1(RELOID, DatumGetObjectId(viewOid));
Use SearchSysCacheCopy1, since you're modifying the tuple.
> + if (!HeapTupleIsValid(oldtup))
> + elog(ERROR, "cache lookup failed for relation %u", viewOid);
> +
> + memset(values, 0, sizeof(values));
> + memset(nulls, false, sizeof(nulls));
> + memset(replaces, false, sizeof(replaces));
> +
> + replaces[Anum_pg_class_reloptions - 1] = true;
> + nulls[Anum_pg_class_reloptions - 1] = true;
> +
> + newtup = heap_modify_tuple(oldtup, RelationGetDescr(pg_class),
> +values, nulls,
> replaces);
> + simple_heap_update(pg_class, &newtup->t_self, newtup);
> +
> + CatalogUpdateIndexes(pg_class, newtup);
> +
> + ReleaseSysCache(oldtup);
> +
> + heap_close(pg_class, RowExclusiveLock);
> +
> + CommandCounterIncrement();
Why is a CCI necessary?
> + }
In any event, we seem to be converging on a version of parts 0 and 1 that are
ready for committer. However, Robert contends that this will not be committed
separately from part 2. Unless someone wishes to contest that, I suggest we
mark this Returned with Feedback and let the CF entry for part 2 subsume its
future development. Does that sound reasonable?
Thanks,
nm
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 2
2011/7/9 Robert Haas : > On Fri, Jul 8, 2011 at 4:57 PM, Noah Misch wrote: >> Note that it does not matter whether we're actually doing an index scan -- a >> seq >> scan with a filter using only leakproof operators is equally acceptable. >> What I >> had in mind was to enumerate all operators in operator classes of indexes >> below >> each security view. Those become the leak-free operators for that security >> view. If the operator for an OpExpr is considered leak-free by all sources >> of >> its operands, then we may push it down. That's purely a high-level sketch: I >> haven't considered implementation concerns in any detail. The resulting >> behavior could be surprising: adding an index may change a plan without the >> new >> plan actually using the index. >> >> I lean toward favoring the pg_proc flag. Functions like "texteq" will be >> taken >> as leakproof even if no involved table has an index on a text column. It >> works >> for functions that will never take a place in an operator class, like >> length(text). When a user reports a qualifier not getting pushed down, the >> answer is much more satisfying: "Run 'CREATE OR REPLACE FUNCTION >> ... I_DONT_LEAK' as a superuser." Compare to "Define an operator class that >> includes the function, if needed, and create an otherwise-useless index." >> The >> main disadvantage I see is the loss of policy locality. Only a superuser (or >> maybe database owner?) can create or modify declared-leakproof functions, and >> that decision applies throughout the database. However, I think the other >> advantages clearly outweigh that loss. > > This strikes me as a fairly compelling refutation of Heikki's proposed > approach. > OK, I'll try to modify the patch according to the flag of pg_proc design. As long as the default of user-defined function is off, and we provide built-in functions with appropriate configurations, it seems to me the burden of DBA is quite limited. Thanks, -- KaiGai Kohei -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] Creating temp tables inside read only transactions
On Fri, 2011-07-08 at 21:04 -0700, Darren Duncan wrote: > > I think you should make more of an effort to understand how the system > > works now, and why, before proposing radical redesigns. > > Well yes, of course. But that will take time and I think I already > understand > enough about it to make some useful contributions in the meantime. How much > or > what I already know may not always come across well. If this bothers people > then I can make more of an effort to reduce my input until I have more solid > things to back them up. I don't think anyone expects you to understand all the internal APIs in postgres before you make a proposal. But we do expect you to look critically at your own proposals with the status quo (i.e. existing code, users, and standards) in mind. And that probably means poking at the code a little to see if you find stumbling blocks, and asking questions to try to trace out the shape of the project. I'm hoping that we can learn a lot from your work on Muldis D. In particular, the type system might be the most fertile ground -- you've clearly done some interesting things there, and I think we've felt some pressure to improve the type system from a number of different projects*. Regards, Jeff Davis * That being said, PostgreSQL's type system is actually very good. Consider the sophisticated type infrastructure (or at least plumbing around the type system) required to make KNN-GiST work, for instance. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] DROP Reworks Part.1 - Consolidate routines to handle DropStmt
2011/7/9 Robert Haas : > On Fri, Jul 8, 2011 at 9:06 AM, Kohei KaiGai wrote: >> I definitely agree with this idea. It will enables to eliminate ugly switch >> statements for error-messaging reasons. > > All right, so please submit a patch that introduces that concept > first, and then resubmit this patch rebased over those changes. > > In view of the time remaining in this CommitFest, I am going to mark > this Returned with Feedback for now. The next CommitFest starts > September 15th, but I'll try to review it sooner as time permits. > OK, Thanks for your reviewing. -- KaiGai Kohei -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.2] Fix leaky-view problem, part 1
2011/7/8 Noah Misch : > On Fri, Jul 08, 2011 at 09:20:46AM +0100, Kohei KaiGai wrote: >> 2011/7/7 Noah Misch : >> > On Thu, Jul 07, 2011 at 03:56:26PM +0100, Kohei KaiGai wrote: >> >> 2011/7/7 Noah Misch : >> >> > On Wed, Jul 06, 2011 at 10:25:12PM +0200, Kohei KaiGai wrote: > >> >> > That gets the job done for today, but DefineVirtualRelation() should >> >> > not need >> >> > to know all view options by name to simply replace the existing list >> >> > with a >> >> > new one. ?I don't think you can cleanly use the ALTER TABLE SET/RESET >> >> > code for >> >> > this. ?Instead, compute an option list similar to how DefineRelation() >> >> > does so >> >> > at tablecmds.c:491, then update pg_class. >> >> > >> >> My opinion is ALTER TABLE SET/RESET code should be enhanced to accept >> >> an operation to reset all the existing options, rather than tricky >> >> updates of pg_class. >> > >> > The pg_class update has ~20 lines of idiomatic code; see >> > tablecmds.c:7931-7951. >> > >> Even if idiomatic, another part of DefineVirtualRelation() uses >> AlterTableInternal(). >> I think a common way is more straightforward. > > The fact that we use ALTER TABLE ADD COLUMN in DefineVirtualRelation() is not > itself cause to use ALTER TABLE SET (...) nearby. We should do so only if it > brings some advantage, like simpler or more-robust code. I'm not seeing > either > advantage. Those can be points of style, so perhaps I have the poor taste > here. > The attached patch is a revised version according to the approach that updates pg_class system catalog before AlterTableInternal(). It invokes the new ResetViewOptions when rel->rd_options is not null, and it set null on the pg_class.reloptions of the view and increments command counter. Rest of stuffs are not changed from the v5. Thanks, -- KaiGai Kohei pgsql-v9.2-fix-leaky-view-part-0.v6.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
