Re: [HACKERS] WIP: Page space reservation (pgupgrade)
On Sat, Nov 8, 2008 at 8:08 PM, Tom Lane <[EMAIL PROTECTED]> wrote: > Zdenek Kotala <[EMAIL PROTECTED]> writes: >> Attached patch allows to setup storage parameter for space >> reservation. > > What is the point of this? That's my question. Why is this needed at all? -- Jonah H. Harris, Senior DBA myYearbook.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] Optimizing COPY
Heikki, I was assigned as a round-robin reviewer for this patch, but it looks to me like it is still WIP, so I'm not sure how much effort it's worth putting in at this point. Do you plan to finish this for 8.4, and if so, should I wait for the next version before reviewing further? Thanks, ...Robert On Thu, Oct 30, 2008 at 8:14 AM, Heikki Linnakangas <[EMAIL PROTECTED]> wrote: > Back in March, I played around with various hacks to improve COPY FROM > performance: > http://archives.postgresql.org/pgsql-patches/2008-03/msg00145.php > > I got busy with other stuff, but I now got around to try what I planned back > then. I don't know if I have the time to finish this for 8.4, but might as > well post what I've got. > > The basic idea is to replace the custom loop in CopyReadLineText with > memchr(), because memchr() is very fast. To make that possible, perform the > client-server encoding conversion on each raw block that we read in, before > splitting it into lines. That way CopyReadLineText only needs to deal with > server encodings, which is required for the memchr() to be safe. > > Attached is a quick patch for that. Think of it as a prototype; I haven't > tested it much, and I feel that it needs some further cleanup. Quick testing > suggests that it gives 0-20% speedup, depending on the data. Very narrow > tables don't benefit much, but the wider the table, the bigger the gain. I > haven't found a case where it performs worse. > > I'm only using memchr() with non-csv format at the moment. It could be used > for CSV as well, but it's more complicated because in CSV mode we need to > keep track of the escapes. > > Some collateral damage: > \. no longer works. If we only accept \. on a new line, like we already do > in CSV mode, it shouldn't be hard or expensive to make it work again. The > manual already suggests that we only accept it on a single line: "End of > data can be represented by a single line containing just backslash-period > (\.)." > > Escaping a linefeed or carriage return by prepending it with a backslash no > longer works. You have to use \n and \r. The manual already warns against > doing that, so I think we could easily drop support for it. If we wanted, it > wouldn't be very hard to make it work, though: after hitting a newline, scan > back and count how many backslashes there is before the newline. An odd > number means that it's an escaped newline, even number (including 0) means > it's a real newline. > > For the sake of simplifying the code, would anyone care if we removed > support for COPY with protocol version 2? > > -- > Heikki Linnakangas > EnterpriseDB http://www.enterprisedb.com > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > > -- 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] WIP: Page space reservation (pgupgrade)
Zdenek Kotala <[EMAIL PROTECTED]> writes: > Attached patch allows to setup storage parameter for space > reservation. What is the point of this? We don't need it for 8.3->8.4, we aren't going to back-patch such a thing into 8.2 or before (certainly not before, since reloptions didn't exist before 8.2), and I would hope we put in a more general solution than this for 8.4-to-later preparatory fixes. regards, tom lane -- 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 for SQL-Standard Interval output and decoupling DateStyle from IntervalStyle
Chuck McDevitt <[EMAIL PROTECTED]> writes: > Doesn't ANSI standard interval syntax have the minus sign before the quotes? > Select interval -'2008-10'; They allow it either there or inside the quotes. We can't support outside-the-quotes unless we make INTERVAL a fully reserved word (and even then there are syntactic problems :-(). Putting the minus sign before INTERVAL works fine btw ... regards, tom lane -- 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 for SQL-Standard Interval output and decoupling DateStyle from IntervalStyle
Doesn't ANSI standard interval syntax have the minus sign before the quotes? Select interval -'2008-10'; ??? > -Original Message- > From: [EMAIL PROTECTED] [mailto:pgsql-hackers- > [EMAIL PROTECTED] On Behalf Of Tom Lane > Sent: Saturday, November 08, 2008 11:39 AM > To: Ron Mayer > Cc: Brendan Jurd; Kevin Grittner; pgsql-hackers@postgresql.org > Subject: Re: [HACKERS] Patch for SQL-Standard Interval output and > decoupling DateStyle from IntervalStyle > > BTW, I just noticed that CVS HEAD has a bug in reading negative SQL- > spec > literals: > > regression=# select interval '-2008-10'; >interval > -- > -2008 years -10 mons > (1 row) > > regression=# select interval '--10'; > interval > -- > 10 mons > (1 row) > > Surely the latter must mean -10 months. This is orthogonal to the > current patch ... > > regards, tom lane > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers -- 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 for SQL-Standard Interval output and decoupling DateStyle from IntervalStyle
Ron Mayer <[EMAIL PROTECTED]> writes: > Brendan Jurd wrote: >> The changes to the documentation all look good. I did notice one >> final typo that I think was introduced in the latest version. >> doc/src/sgml/datatype.sgml:2270 has "Nonstandardrd" instead of >> "Nonstandard". > Just checked in a fix to that one; and updated my website at > http://0ape.com/postgres_interval_patches/ > and pushed it to my (hopefully fixed now) git server. Applied with some revisions: I changed the rule for negating input fields in SQL_STANDARD style as we discussed, made IntervalStyle into an "enum" GUC, and did some pretty heavy editorialization on the docs changes. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] WIP: Page space reservation (pgupgrade)
Attached patch allows to setup storage parameter for space reservation. I use
reloptions capability for it. You can use it:
CREATE TABLE test(id int) with (reservedspace=10);
The idea is to reduce freespace value about reservedspace on places where
PageGet(Heap)FreeSpace is called.
I need perform more tests on this patch, however I need feedback if it is
reasonable way. It seems to me that the patch could be backported without any
problem.
I have still following doubts/questions:
1) GiST - gist uses gistnospace() function to check correct amount of space.
unfortunately, it does not have information about Relation. The function is
called from:
gistContinueInsert(), gistplacetopage(), and gistVacuumUpdate().
It seems to me that better that modify this function should be modified these
callers (exclude gistContinueInsert see 2)
2) WAL - I do not modify freespace during WAL replay. I think that when
reservedspace is set, then WAL record cannot break a space reservation.
3) vacuum - PageGetFreeSpaceWithFillFactor
It look likes that vacuum uses fill factor to check possible free space on page,
but it does not try to free space on page to satisfy fill factor criteria. Is it
correct or I'm wrong?
Thanks for your comments.
Zdenek
--
Zdenek Kotala Sun Microsystems
Prague, Czech Republic http://sun.com/postgresql
diff -Nrc pgsql_spacereserve.9e46c188067f/src/backend/access/common/reloptions.c pgsql_spacereserve.c901d4bb8cca/src/backend/access/common/reloptions.c
*** pgsql_spacereserve.9e46c188067f/src/backend/access/common/reloptions.c 2008-11-08 23:19:47.795930570 +0100
--- pgsql_spacereserve.c901d4bb8cca/src/backend/access/common/reloptions.c 2008-11-08 23:19:47.910535376 +0100
***
*** 286,330
default_reloptions(Datum reloptions, bool validate,
int minFillfactor, int defaultFillfactor)
{
! static const char *const default_keywords[1] = {"fillfactor"};
! char *values[1];
! int fillfactor;
StdRdOptions *result;
! parseRelOptions(reloptions, 1, default_keywords, values, validate);
/*
* If no options, we can just return NULL rather than doing anything.
* (defaultFillfactor is thus not used, but we require callers to pass it
* anyway since we would need it if more options were added.)
*/
! if (values[0] == NULL)
return NULL;
! if (!parse_int(values[0], &fillfactor, 0, NULL))
{
! if (validate)
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
! errmsg("fillfactor must be an integer: \"%s\"",
! values[0])));
! return NULL;
}
! if (fillfactor < minFillfactor || fillfactor > 100)
{
! if (validate)
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
! errmsg("fillfactor=%d is out of range (should be between %d and 100)",
! fillfactor, minFillfactor)));
! return NULL;
}
result = (StdRdOptions *) palloc(sizeof(StdRdOptions));
SET_VARSIZE(result, sizeof(StdRdOptions));
result->fillfactor = fillfactor;
return (bytea *) result;
}
--- 286,360
default_reloptions(Datum reloptions, bool validate,
int minFillfactor, int defaultFillfactor)
{
! static const char *const default_keywords[2] = {"fillfactor","reservedspace"};
! char *values[2];
! int fillfactor=defaultFillfactor;
! int reservedspace=0;
StdRdOptions *result;
! parseRelOptions(reloptions, 2, default_keywords, values, validate);
/*
* If no options, we can just return NULL rather than doing anything.
* (defaultFillfactor is thus not used, but we require callers to pass it
* anyway since we would need it if more options were added.)
*/
! if ((values[0] == NULL) && (values[1] == NULL))
return NULL;
! /* fill factor */
! if (values[0] != NULL)
{
! if (!parse_int(values[0], &fillfactor, 0, NULL))
! {
! if (validate)
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
! errmsg("fillfactor must be an integer: \"%s\"",
! values[0])));
! return NULL;
! }
!
! if (fillfactor < minFillfactor || fillfactor > 100)
! {
! if (validate)
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
! errmsg("fillfactor=%d is out of range (should be between %d and 100)",
! fillfactor, minFillfactor)));
! return NULL;
! }
}
! /* reserved space */
! if (values[1] != NULL)
{
! if (!parse_int(values[1], &reservedspace, 0, NULL))
! {
! if (validate)
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
! errmsg("reservedspace must be an integer: \"%s\"",
! values[1])));
! return NULL;
! }
!
! if (reservedspace < 0 || reservedspace > BLCKSZ/4)
! {
! if (validate)
! ereport(ERROR,
! (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
! errmsg("reservedspace=%d is out of range (should be between 0 and %d)",
!
Re: [HACKERS] [PATCH] Recreate Missing WAL Directories (from TODO)
On Sat, Nov 8, 2008 at 4:08 PM, Tom Lane <[EMAIL PROTECTED]> wrote: > This has been suggested before but I'm unconvinced that it's a good > idea. It's reasonably common for pg_xlog to be a symlink. If you > neglect to re-establish the symlink then what would happen is that xlog > gets recreated on the data disk, and with no notice you are running in > a degraded mode. Agreed on the basis that people sometimes forget to symlink. That's the reason why I was echoing a message. Initially the message was WARNING, but I degraded it to LOG. > It might be reasonable to auto-recreate XLOGDIR/archive_status, though. Attached. BTW, I have seen people create both pg_xlog and archive_status as files, which is why I'm validating that in this function rather than waiting for it to error-out later in the code. -- Jonah H. Harris, Senior DBA myYearbook.com arcstatdir_v2.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
Re: [HACKERS] [PATCH] Recreate Missing WAL Directories (from TODO)
"Jonah H. Harris" <[EMAIL PROTECTED]> writes: > When performing a PITR copy of a data cluster, the pg_xlog directory > is generally omitted. As such, when starting the copy up for > replay/recovery, the WAL directories need to be recreated. This patch > checks to see whether XLOGDIR and XLOGDIR/archive_status exist on > XLOGStartup and if not, recreates them. This has been suggested before but I'm unconvinced that it's a good idea. It's reasonably common for pg_xlog to be a symlink. If you neglect to re-establish the symlink then what would happen is that xlog gets recreated on the data disk, and with no notice you are running in a degraded mode. It might be reasonable to auto-recreate XLOGDIR/archive_status, though. regards, tom lane -- 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 for SQL-Standard Interval output and decoupling DateStyle from IntervalStyle
I wrote: > ... Consider > -1 1:00:00 flips the sign > - 1 1:00:00 doesn't flip the sign > -1 day 1:00:00 doesn't flip the sign > -2008-10 1:00:00flips the sign > -2008-10 1 doesn't flip the sign > -2008 years 1:00:00 doesn't flip the sign > If the rule were that it never flipped the sign for non-SQL-spec input > then I think that'd be okay, but case 4 here puts the lie to that. > I'm also not entirely sure if case 2 is allowed by SQL spec or not, > but if it is then we've got a problem with that; and even if it isn't > it's awfully hard to explain why it's treated differently from case 1. Actually case 2 is a red herring --- I now see that ParseDateTime() collapses out the whitespace and makes it just like case 1. So never mind that. Case 4 is still bogus though. regards, tom lane -- 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 for SQL-Standard Interval output and decoupling DateStyle from IntervalStyle
Ron Mayer <[EMAIL PROTECTED]> writes: > Yes, at first glance I think that approach is better; but we'd need > to make sure not to apply the rule too enthusiastically on traditional > postgres intervals; Well, of course we'd only apply it in SQL_STANDARD mode. The idea here is that intervalstyle helps us resolve an ambiguity about what the signs are, more or less independently of syntactic details. If you consider that the issue is sql standard: leading sign applies to all fields postgres: each field is independently signed this applies perfectly well without any restrictions on syntax. > In some ways I wonder if we should have 2 totally separate parsing > one for the SQL standard ones, and one for the postgres. No, I think we want the style to be a hint for resolving ambiguous cases, not to cause complete failure if the input doesn't conform to the style. That's certainly how DateStyle has always worked. regards, tom lane -- 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 for SQL-Standard Interval output and decoupling DateStyle from IntervalStyle
Ron Mayer <[EMAIL PROTECTED]> writes: > Tom Lane wrote: >> BTW, I just noticed that CVS HEAD has a bug in reading negative SQL-spec >> literals: > Perhaps the below patch fixes that? Actually I think it should be if (*field[i] == '-') as in the comparable case for fractional seconds just below. Will apply. regards, tom lane -- 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 for SQL-Standard Interval output and decoupling DateStyle from IntervalStyle
Tom Lane wrote: Another thought here ... I'm looking at the sign hack + if (IntervalStyle == INTSTYLE_SQL_STANDARD && and not liking it very much. Yes, it does the intended thing for strict SQL-spec input, but it seems to produce a bunch of weird corner cases for non-spec input. Consider [... many examples ...] I'm inclined to think we need a more semantically-based instead of syntactically-based rule. For instance, if first field is negative and no other field has an explicit sign, then force all fields to be <= 0. This would probably have to be applied at the end of DecodeInterval instead of on-the-fly within the loop. Thoughts? I'll take a step back and think about that Yes, at first glance I think that approach is better; but we'd need to make sure not to apply the rule too enthusiastically on traditional postgres intervals; or worse, ones that mix sql standardish and postgres values For example dish=# select interval '-1 1:1 1 years'; interval -- 1 year -1 days +01:01:00 (1 row) that 8.3 accepts. (or do we not care about those)? In some ways I wonder if we should have 2 totally separate parsing one for the SQL standard ones, and one for the postgres. That would avoid some other confusing inputs like: select interval '-00-01 1 years'; select interval '1-1 hours'; select interval '1:1 years'; select interval '1 hours 1-1 1 years' that are currently accepted. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] Recreate Missing WAL Directories (from TODO)
When performing a PITR copy of a data cluster, the pg_xlog directory is generally omitted. As such, when starting the copy up for replay/recovery, the WAL directories need to be recreated. This patch checks to see whether XLOGDIR and XLOGDIR/archive_status exist on XLOGStartup and if not, recreates them. -- Jonah H. Harris, Senior DBA myYearbook.com arcstatdir.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
Re: [HACKERS] TABLE command
> Hmm. Given the current infrastructure for \h, the only way to do that > would be to make a separate ref page for WITH, which feels like the > wrong thing. And the objection I have to TABLE is not the code but the > apparent need to give it its own ref page (as we already did for VALUES, > and I found that pretty ugly too). Suck it up. :-) Presumably there could be more things in this category in the future, so we'd better figure out how to handle it. I just noticed that, at present, "\h WI" tab-completes to "\h WITH" and then to "\h WITH RECURSIVE", but hitting return then tells you that no help is actually available, which is pretty horrible. > Is there a way to make all of these point at the SELECT ref page? I think we could probably modify helpSQL() to support a list of aliases. I'm not sure how tricky that would be - the existing logic appears slightly byzantine. I'll take a crack at it if you would like. > Something cleaner than a special hack in psql would be nice, but > I guess I'd settle for that as still an improvement over considering > that TABLE is a command. The problem with documenting VALUES and > TABLE as commands is that this doesn't reflect their principal use > as elements of a SELECT; and it also becomes quite unclear why you can't > use, say, EXPLAIN or SHOW as elements of a SELECT. Well, I suppose we could make it so that they can be. *ducks* ...Robert -- 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 for SQL-Standard Interval output and decoupling DateStyle from IntervalStyle
Another thought here ... I'm looking at the sign hack
+ if (IntervalStyle == INTSTYLE_SQL_STANDARD &&
+ field[0][0] == '-' && i == 1 &&
+ field[i][0] != '-' && field[i][0] != '+')
+ {
+ /*--
+ * The SQL Standard defines the interval literal
+ * '-1 1:00:00'
+ * to mean "negative 1 days and negative one hours"
+ * while Postgres traditionally treated this as
+ * meaning "negative 1 days and positive one hours".
+ * In SQL_STANDARD style, flip the sign to conform
+ * to the standard's interpretation.
and not liking it very much. Yes, it does the intended thing for strict
SQL-spec input, but it seems to produce a bunch of weird corner cases
for non-spec input. Consider
-1 1:00:00 flips the sign
- 1 1:00:00 doesn't flip the sign
-1 day 1:00:00 doesn't flip the sign
-2008-10 1:00:00flips the sign
-2008-10 1 doesn't flip the sign
-2008 years 1:00:00 doesn't flip the sign
If the rule were that it never flipped the sign for non-SQL-spec input
then I think that'd be okay, but case 4 here puts the lie to that.
I'm also not entirely sure if case 2 is allowed by SQL spec or not,
but if it is then we've got a problem with that; and even if it isn't
it's awfully hard to explain why it's treated differently from case 1.
I'm inclined to think we need a more semantically-based instead of
syntactically-based rule. For instance, if first field is negative and
no other field has an explicit sign, then force all fields to be <= 0.
This would probably have to be applied at the end of DecodeInterval
instead of on-the-fly within the loop.
Thoughts?
regards, tom lane
--
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 for SQL-Standard Interval output and decoupling DateStyle from IntervalStyle
Tom Lane wrote: BTW, I just noticed that CVS HEAD has a bug in reading negative SQL-spec literals: regression=# select interval '-2008-10'; regression=# select interval '--10'; Surely the latter must mean -10 months. This is orthogonal to the current patch ... Perhaps the below patch fixes that? (though line numbers probably won't match since this was based off of the patched version) *** a/src/backend/utils/adt/datetime.c --- b/src/backend/utils/adt/datetime.c *** *** 2879,2885 DecodeInterval(char **field, int *ftype, int nf, int range, if (*cp != '\0') return DTERR_BAD_FORMAT; type = DTK_MONTH; ! if (val < 0) val2 = -val2; val = val * MONTHS_PER_YEAR + val2; fval = 0; --- 2879,2885 if (*cp != '\0') return DTERR_BAD_FORMAT; type = DTK_MONTH; ! if (field[0][0] == '-') val2 = -val2; val = val * MONTHS_PER_YEAR + val2; fval = 0; [5]lt:/home/ramayer/proj/pg/postgresql/src/backend/utils/adt% -- 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] Block-level CRC checks
Alvaro Herrera wrote: > Alvaro Herrera wrote: > >Alvaro Herrera wrote: > > > > Hmm, oh I see another problem here -- the bit is not restored when > > replayed heap_update's WAL record. I'm now wondering what other bits > > are set without much care about correctly restoring them during replay. > > > > I'm now wondering whether it'd be easier to just ignore pd_flags in > > calculating the checksum. > > Okay, so this is what I've done. pd_flags is skipped. Also the WAL > routine logs both HeapTupleHeader infomasks and ItemId->lp_flags. On > the latter point I'm not 100% sure of the cases where lp_flags must be > logged; right now I'm only logging if the item is marked as "having > storage" (the logic being that if an item does not have storage, then > making it have requires a WAL entry, and vice versa). Might it make sense to move such flags to another data structure which may or may not need to be logged, thereby maintaining the crc integrity of the data pages themselves? (I pre-apologize if this is a silly, as I honestly don't understand how once a page has been logically committed to storage, it can ever be subsequently validly modified unless first removed as being committed to storage; as if it's write were interrupted prior to being completed, it seems most correct to simply consider the page as not having been stored and simply resume the process from the beginning if a partial store is suspected; thereby implying that any buffers storing the logical page are not released until the page as a whole is known to have been successfully stored; thereby retaining the entire page to either to remain committed for storage, or possibly alternatively made re-available for mutation with it's crc marked as invalid if ever mutated prior to being re-committed to storage, it seems.) -- 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 for SQL-Standard Interval output and decoupling DateStyle from IntervalStyle
Tom Lane wrote: Oh, I see what you're trying to do. The answer is no. We're not going to totally destroy back-portability of dumps, especially not for a problem that won't even affect most people (negative intervals are hardly common). Similarly I wonder if pg_dump should add a "fail if version < 8.2" right before it outputs SET standard_conforming_strings = on; which IMHO is far more common than negative intervals and AFAICT has the same risk. For intervals, we would only add the fail code if intervalstyle was set to one of the new interval styles (if the ISO8601 interval's accepted it'll have the problem too). For backward compatible patches, they could still have their GUC settingse specify standard_conforming_strings and interval_style values that are supported by whichever versions they want to 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] Patch for SQL-Standard Interval output and decoupling DateStyle from IntervalStyle
Ron Mayer <[EMAIL PROTECTED]> writes: >>> select * from (select substring(version() from '[0-9\.]+') as version) as a >>> join (select generate_series(0,1000)) as b on(version<'8.4'); >>> set intervalstyle = something; >> >> [ shrug... ] It's still just one easily missable bleat. > Not here. > On my system it hangs forever on 8.3 or less and proceeds > harmlessly with 8.4. Oh, I see what you're trying to do. The answer is no. We're not going to totally destroy back-portability of dumps, especially not for a problem that won't even affect most people (negative intervals are hardly common). regards, tom lane -- 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 for SQL-Standard Interval output and decoupling DateStyle from IntervalStyle
Tom Lane wrote: Ron Mayer <[EMAIL PROTECTED]> writes: Tom Lane wrote: The trouble is that older servers will (by default) report an error on that line and keep right on chugging. Not necessarily. Couldn't we put select * from (select substring(version() from '[0-9\.]+') as version) as a join (select generate_series(0,1000)) as b on(version<'8.4'); set intervalstyle = something; [ shrug... ] It's still just one easily missable bleat. Not here. On my system it hangs forever on 8.3 or less and proceeds harmlessly with 8.4. -- 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 for SQL-Standard Interval output and decoupling DateStyle from IntervalStyle
Ron Mayer <[EMAIL PROTECTED]> writes: > Tom Lane wrote: >> There isn't any way to do that, unless you have a time machine in >> your hip pocket. The trouble with putting >> set intervalstyle = something; >> into the dump script is that older servers will (by default) report >> an error on that line and keep right on chugging. > Not necessarily. Couldn't we put > select * from (select substring(version() from '[0-9\.]+') as version) as a > join (select generate_series(0,1000)) as b on(version<'8.4'); > set intervalstyle = something; > Or something similar in the dump file. [ shrug... ] It's still just one easily missable bleat. regards, tom lane -- 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 for SQL-Standard Interval output and decoupling DateStyle from IntervalStyle
Tom Lane wrote: Ron Mayer <[EMAIL PROTECTED]> writes: (3) Put something into the dump file that will make the old server reject the file rather than successfully loading wrong data? (Some "if intervalstyle==std and version<8.3 abort loading the restore" logic?) There isn't any way to do that, unless you have a time machine in your hip pocket. The trouble with putting set intervalstyle = something; into the dump script is that older servers will (by default) report an error on that line and keep right on chugging. Not necessarily. Couldn't we put select * from (select substring(version() from '[0-9\.]+') as version) as a join (select generate_series(0,1000)) as b on(version<'8.4'); set intervalstyle = something; Or something similar in the dump file. -- 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 for SQL-Standard Interval output and decoupling DateStyle from IntervalStyle
BTW, I just noticed that CVS HEAD has a bug in reading negative SQL-spec literals: regression=# select interval '-2008-10'; interval -- -2008 years -10 mons (1 row) regression=# select interval '--10'; interval -- 10 mons (1 row) Surely the latter must mean -10 months. This is orthogonal to the current patch ... regards, tom lane -- 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] auto_explain contrib moudle
On Fri, 2008-11-07 at 18:03 +0200, Martin Pihlak wrote:
> Another thing is a feature I am interested in -- ability to auto-explain
> statements
> execututed from within functions. I'm thinking of adding an extra boolean GUC
> --
> "explain.log_nested_statements" (defaults to false). Quick test seems to give
> expected results, but there maybe something I missed.
Thanks.
I applied this patch for log_nested_statements and I merged with HEAD.
Current patch attached.
One thing I'm unsure of (this question is for ITAGAKI Takahiro): why is
it necessary to define a new function DefineCustomVariable(), when there
are already functions DefineCustomBoolVariable() and
DefineCustomIntVariable()?
Regards,
Jeff Davis
diff --git a/contrib/Makefile b/contrib/Makefile
index 30f75c7..b25af3c 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -6,6 +6,7 @@ include $(top_builddir)/src/Makefile.global
WANTED_DIRS = \
adminpack \
+ auto_explain \
btree_gist \
chkpass \
citext \
diff --git a/contrib/auto_explain/Makefile b/contrib/auto_explain/Makefile
new file mode 100644
index 000..0d0ccc2
--- /dev/null
+++ b/contrib/auto_explain/Makefile
@@ -0,0 +1,13 @@
+MODULE_big = auto_explain
+OBJS = auto_explain.o
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/auto_explain
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c
new file mode 100644
index 000..5f582c3
--- /dev/null
+++ b/contrib/auto_explain/auto_explain.c
@@ -0,0 +1,213 @@
+/*
+ * auto_explain.c
+ */
+#include "postgres.h"
+
+#include "commands/explain.h"
+#include "executor/instrument.h"
+#include "miscadmin.h"
+#include "utils/guc_tables.h"
+
+PG_MODULE_MAGIC;
+
+#define GUCNAME(name) ("explain." name)
+
+static int explain_log_min_duration = -1; /* msec or -1 */
+static bool explain_log_analyze = false;
+static bool explain_log_verbose = false;
+static bool explain_log_nested = false;
+
+static bool toplevel = true;
+static ExecutorRun_hook_type prev_ExecutorRun = NULL;
+
+void _PG_init(void);
+void _PG_fini(void);
+
+static void explain_ExecutorRun(QueryDesc *queryDesc,
+ ScanDirection direction,
+ long count);
+static bool assign_log_min_duration(int newval, bool doit, GucSource source);
+static bool assign_log_analyze(bool newval, bool doit, GucSource source);
+static bool assign_log_verbose(bool newval, bool doit, GucSource source);
+static bool assign_log_nested(bool newval, bool doit, GucSource source);
+
+static struct config_int def_log_min_duration =
+{
+ {
+ GUCNAME("log_min_duration"),
+ PGC_USERSET,
+ STATS_MONITORING,
+ "Sets the minimum execution time above which plans will be logged.",
+ "Zero prints all plans. -1 turns this feature off.",
+ GUC_UNIT_MS
+ },
+ &explain_log_min_duration,
+ -1, -1, INT_MAX / 1000, assign_log_min_duration, NULL
+};
+
+static struct config_bool def_log_analyze =
+{
+ {
+ GUCNAME("log_analyze"),
+ PGC_USERSET,
+ STATS_MONITORING,
+ "Use EXPLAIN ANALYZE for plan logging."
+ },
+ &explain_log_analyze,
+ false, assign_log_analyze, NULL
+};
+
+static struct config_bool def_log_verbose =
+{
+ {
+ GUCNAME("log_verbose"),
+ PGC_USERSET,
+ STATS_MONITORING,
+ "Use EXPLAIN VERBOSE for plan logging."
+ },
+ &explain_log_verbose,
+ false, assign_log_verbose, NULL
+};
+
+static struct config_bool def_log_nested_statements =
+{
+ {
+ GUCNAME("log_nested_statements"),
+ PGC_USERSET,
+ STATS_MONITORING,
+ "Log nested statements."
+ },
+ &explain_log_nested,
+ false, assign_log_nested, NULL
+};
+
+void
+_PG_init(void)
+{
+ DefineCustomVariable(PGC_INT, &def_log_min_duration);
+ DefineCustomVariable(PGC_BOOL, &def_log_analyze);
+ DefineCustomVariable(PGC_BOOL, &def_log_verbose);
+ DefineCustomVariable(PGC_BOOL, &def_log_nested_statements);
+
+ /* install ExecutorRun_hook */
+ prev_ExecutorRun = ExecutorRun_hook;
+ ExecutorRun_hook = explain_ExecutorRun;
+}
+
+void
+_PG_fini(void)
+{
+ /* uninstall ExecutorRun_hook */
+ ExecutorRun_hook = prev_ExecutorRun;
+}
+
+void
+explain_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction, long count)
+{
+ if ((toplevel || explain_log_nested) && explain_log_min_duration >= 0)
+ {
+ instr_time starttime;
+ instr_time duration;
+ double msec;
+
+ /* Disable our hooks temporarily during the top-level query. */
+ toplevel = false;
+ PG_TRY();
+ {
+ INSTR_TIME_SET_CURRENT(starttime);
+
+ if (prev_ExecutorRun)
+prev_ExecutorRun(queryDesc, direction, count);
+ else
+standard_ExecutorRun(queryDesc, direction, count);
+
+ INSTR_TIME_SET_CURRENT(duration);
+ INSTR_TIME_SUBTRACT(duration, starttime);
+ msec = INSTR_TIME_GET_MILLISEC(duration);
+
+ /* Log plan if duration is exceeded. */
+ if (msec > explain_log_min_duration)
+ {
+StringInfoData buf;
+
+
Re: [HACKERS] Patch for SQL-Standard Interval output and decoupling DateStyle from IntervalStyle
Ron Mayer <[EMAIL PROTECTED]> writes: > So the options seem to be: > (1) Don't output a SQL-standard interval literal for the > value "negative one days and negative one hours"; perhaps > by sticking an extra '+' sign in there? This is pretty much what the postgres style does... > (2) Force pg_dump to a non-standard mode, at least until 8.3's > deprecated in many years? IOW, same as above. > (3) Put something into the dump file that will make the old > server reject the file rather than successfully loading > wrong data? (Some "if intervalstyle==std and version<8.3 > abort loading the restore" logic?) There isn't any way to do that, unless you have a time machine in your hip pocket. The trouble with putting set intervalstyle = something; into the dump script is that older servers will (by default) report an error on that line and keep right on chugging. The same is true of standard_conforming_strings BTW, which is one of the reasons why that's not a very good solution. But at least you're reasonably likely to get additional errors later in the dump if you try to load it into a server that doesn't handle standard_conforming_strings. What's scaring me about the interval stuff is that it will *silently* adopt the wrong reading of ambiguous interval strings. A DBA who missed seeing that one bleat early in the restore would not know anything was wrong. You're right that we don't have to be frozen into this forever, but I fear that any change is going to be a long way off. We couldn't really change pg_dump's output style until we have obsoleted all pre-8.4 releases. regards, tom lane -- 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] TABLE command
"Robert Haas" <[EMAIL PROTECTED]> writes: > Incidentally, I noticed while looking at this that "\h with" also > fails, even though WITH can now be the first word of a valid SQL > statement. I think we ought to patch psql to return the same help for > WITH as it does for SELECT. Hmm. Given the current infrastructure for \h, the only way to do that would be to make a separate ref page for WITH, which feels like the wrong thing. And the objection I have to TABLE is not the code but the apparent need to give it its own ref page (as we already did for VALUES, and I found that pretty ugly too). Is there a way to make all of these point at the SELECT ref page? Something cleaner than a special hack in psql would be nice, but I guess I'd settle for that as still an improvement over considering that TABLE is a command. The problem with documenting VALUES and TABLE as commands is that this doesn't reflect their principal use as elements of a SELECT; and it also becomes quite unclear why you can't use, say, EXPLAIN or SHOW as elements of a SELECT. regards, tom lane -- 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] TABLE command
Hi,
I was assigned to code-review this patch by pgsql-rrreviewers. I
don't have much to add to what's already been written, but here are my
thoughts.
1. I agree with Tom Lane's earlier comments that table_ref is not the
correct non-terminal. For example, this seems pretty strange:
rhaas=# table position('i' in 'team');
position
--
0
As far as I can tell from looking around a bit (I don't actually have
a copy of the SQL:2008 spec), the intention is to allow only base
tables or views or references to WITH tables that are in scope. I'm
not sure there's any good reason for that, but with TABLE as the key
word it's just too weird to allow random functions, function-like
operators, etc.
2. Realizing that this patch may have only been intended as a
proof-of-concept, it's pretty incomplete. In addition to updating the
SGML documentation, it needs to update the psql documentation and tab
completion code, and maybe other, similar things that I'm not thinking
of.
rhaas=# \h table
No help available for "table".
Try \h with no arguments to see available help.
Incidentally, I noticed while looking at this that "\h with" also
fails, even though WITH can now be the first word of a valid SQL
statement. I think we ought to patch psql to return the same help for
WITH as it does for SELECT.
3. Although I don't feel strongly about it, I tend to disagree with
the notion that "we don't need this". It's not the most useful
feature in the world, but it's in the spec, and there are situations
where it may save a bit of typing. Since SQL tends to be somewhat
wordy, I think this is a good thing. YMMV.
I will update the status of this patch on the Wiki to "Waiting on author".
...Robert
--
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] Windowing Function Patch Review -> Standard Conformance
Instead of a patch it might be easier to submit the new columns as a perl script or sed command. We do something like that to make merging pg_proc easier. greg On 8 Nov 2008, at 01:36 PM, Tom Lane <[EMAIL PROTECTED]> wrote: "David Rowley" <[EMAIL PROTECTED]> writes: patching file src/include/catalog/pg_proc.h Hunk #4 FAILED at 106. 1 out of 4 hunks FAILED -- saving rejects to file src/include/catalog/pg_proc.h.rej I imagine you'll find that "hunk #4" covers the entire DATA() body of the file :-(. It can't possibly apply cleanly if anyone's added or altered pg_proc entries since the patch was made. What you'd need to do is manually insert proiswfunc = 'f' entries in all the existing DATA lines (this is usually not too hard with sed or an emacs macro), then add whatever new functions the patch defines. Even figuring out the latter from the patch representation can be a serious PITA, since they'll be a few lines out of a multi-thousand- line failed diff hunk. I'm not sure if Hitoshi is in a position to submit the pg_proc changes as two separate diffs --- one to add the new column and a separate one to add in the new functions --- but it'd be a lot easier to deal with merge issues if he could. (Now I'll sit back and wait for some fanboy to claim that $his_favorite_scm could solve this automatically ...) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- 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] Windowing Function Patch Review -> Standard Conformance
"David Rowley" <[EMAIL PROTECTED]> writes: > patching file src/include/catalog/pg_proc.h > Hunk #4 FAILED at 106. > 1 out of 4 hunks FAILED -- saving rejects to file > src/include/catalog/pg_proc.h.rej I imagine you'll find that "hunk #4" covers the entire DATA() body of the file :-(. It can't possibly apply cleanly if anyone's added or altered pg_proc entries since the patch was made. What you'd need to do is manually insert proiswfunc = 'f' entries in all the existing DATA lines (this is usually not too hard with sed or an emacs macro), then add whatever new functions the patch defines. Even figuring out the latter from the patch representation can be a serious PITA, since they'll be a few lines out of a multi-thousand-line failed diff hunk. I'm not sure if Hitoshi is in a position to submit the pg_proc changes as two separate diffs --- one to add the new column and a separate one to add in the new functions --- but it'd be a lot easier to deal with merge issues if he could. (Now I'll sit back and wait for some fanboy to claim that $his_favorite_scm could solve this automatically ...) regards, tom lane -- 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] auto_explain contrib moudle
On Sat, 2008-11-08 at 12:18 +0200, Martin Pihlak wrote: > For me the primary use of auto-explain would be interactive troubleshooting. > The troublesome statements usually involve several nested function calls and > are tedious to trace manually. With auto-explain I fire up psql, load the > module, set the client log level, run the statements and immediately see > what's going on. I bet that lot of the developers and QA folk would use it > similarly. > I think the cost in terms of inconsistency here is just too high for the benefit. If you set the client_min_messages to LOG, you should really get *all* the log messages, not all except for those that happen to occur when psql is in some implicit mode that's invisible to the user. By the way, a possible solution for the problem you describe is to simply set explain.log_min_duration=1s or something, so that you won't see tab completion queries (or, if you do, this is the least of your problems). Or, just disable tab completion. > You are of course right about the log_min_duration_statement, also the > log_executor_stats etc. behave similarly. So indeed, the "ignore notices" > patch > is not necessarily part of auto-explain. > I agree that there might be room for improvement in psql's handling of notices, and that the logging system might be made more flexible. But let's just keep it simple so that we get something in 8.4. I think auto-explain will be very useful to a lot of people as-is. 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] Short CVS question
On Fri, Nov 7, 2008 at 8:24 PM, Dirk Riehle <[EMAIL PROTECTED]> wrote: > Hi, > > I have a short CVS question please: How do I go from a particular file > revision like > > pgsql/cvs/pgsql/src/backend/parser/parse_relation.c.1.3 > > to the complete commit? I.e. I would like to navigate back from this > particular file to the commit and see all the other files that were touched > by the commit. You can't, very easily. Every file commit in CVS is an independent action--they are not grouped as a changeset. You would have to look for other file commits that happened at the same time and have the same log message. Each file has its own revision number that has no relation to those of other files. -Doug -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: FWD: Re: [HACKERS] Updated backslash consistency patch
> 2. the help.c patch no longer applies
>
> 3. the help.c patch breaks alignment of the help output
Attached is a patch to fix problems 2 and 3: help.c clean application and
formatting of the output therein. I also put \z right after \dp and removed
the duplicate wording, to make it fit better, per comments in this thread.
--
Greg Sabino Mullane
Index: command.c
===
RCS file: /projects/cvsroot/pgsql/src/bin/psql/command.c,v
retrieving revision 1.196
diff -c -r1.196 command.c
*** command.c 15 Sep 2008 12:18:00 - 1.196
--- command.c 8 Nov 2008 16:55:19 -
***
*** 329,341
else if (cmd[0] == 'd')
{
char *pattern;
! bool show_verbose;
/* We don't do SQLID reduction on the pattern yet */
pattern = psql_scan_slash_option(scan_state,
OT_NORMAL, NULL, true);
show_verbose = strchr(cmd, '+') ? true : false;
switch (cmd[1])
{
--- 329,342
else if (cmd[0] == 'd')
{
char *pattern;
! bool show_verbose, show_system;
/* We don't do SQLID reduction on the pattern yet */
pattern = psql_scan_slash_option(scan_state,
OT_NORMAL, NULL, true);
show_verbose = strchr(cmd, '+') ? true : false;
+ show_system = strchr(cmd, 'S') ? true: false;
switch (cmd[1])
{
***
*** 345,372
success = describeTableDetails(pattern, show_verbose);
else
/* standard listing of interesting things */
! success = listTables("tvs", NULL, show_verbose);
break;
case 'a':
! success = describeAggregates(pattern, show_verbose);
break;
case 'b':
success = describeTablespaces(pattern, show_verbose);
break;
case 'c':
! success = listConversions(pattern);
break;
case 'C':
success = listCasts(pattern);
break;
case 'd':
! success = objectDescription(pattern);
break;
case 'D':
! success = listDomains(pattern);
break;
case 'f':
! success = describeFunctions(pattern, show_verbose);
break;
case 'g':
/* no longer distinct from \du */
--- 346,373
success = describeTableDetails(pattern, show_verbose);
else
/* standard listing of interesting things */
! success = listTables("tvs", NULL, show_verbose, show_system);
break;
case 'a':
! success = describeAggregates(pattern, show_verbose, show_system);
break;
case 'b':
success = describeTablespaces(pattern, show_verbose);
break;
case 'c':
! success = listConversions(pattern, show_system);
break;
case 'C':
success = listCasts(pattern);
break;
case 'd':
! success = objectDescription(pattern, show_system);
break;
case 'D':
! success = listDomains(pattern, show_system);
break;
case 'f':
! success = describeFunctions(pattern, show_verbose, show_system);
break;
case 'g':
/* no longer distinct from \du */
***
*** 379,398
success = listSchemas(pattern, show_verbose);
break;
case 'o':
! success = describeOperators(pattern);
break;
case 'p':
success = permissionsList(pattern);
break;
case 'T':
! success = describeTypes(pattern, show_verbose);
break;
case 't':
case 'v':
case 'i':
case 's':
case 'S':
! success = listTables(&cmd[1], pattern, show_verbose);
break;
case 'u':
success = describeRoles(pattern, show_verbose);
--- 380,399
success = listSchemas(pattern, show_verbose);
break;
case 'o':
! success = describeOperators(pattern, show_system);
break;
case 'p':
success = permissionsList(pattern);
break;
case 'T':
! success = describeTypes(pattern, show_verbose, show_system);
break;
case 't':
case 'v':
case 'i':
case 's':
case 'S':
! success = listTables(&cmd[1], pattern, show_verbose, show_system);
break;
case 'u':
success = describeRoles(pattern, show_verbose);
Index: describe.c
===
RCS file: /projects/cvsroot/pgsql/src/bin/psql/describe.c,v
retrieving revision 1.187
diff -c -r1.187 describe.c
*** describe.c 6 Nov 2008 15:18:35 - 1.187
--- describe.c 8 Nov 2008 16:55:19 -
***
*** 52,58
* Takes an optional regexp to select particular aggregates
*/
bool
! describeAggregates(const char *pattern, bool verbose)
{
PQExpBufferData buf;
PGresult *res;
--- 52,58
* Takes an optional regexp to select particular aggregates
*/
bool
! describeAggregates(const char *pattern, bool verbose, bool showSystem)
{
PQExpBufferData buf;
PGresult *res;
***
*** 75,81
"ELSE\n"
"pg_catalog.array_to_string(ARRAY(\n"
" SELECT\n"
!
Re: [HACKERS] Short CVS question
In general, this is pretty hard to do in CVS - you basically have to look for other commits with the same time stamp and log message, and I don't think the tool provides any real support for that. In the case of pgsql, you might want to look at the commit message and then google the pgsql-committters mailing list archives... for example I just found where my TRUNCATE patch was applied by googling this: site:archives.postgresql.org/pgsql-committers/ truncate Note that sometimes one commit generates multiple messages in the archives, and other times not, so you may want to look at the date or thread index. ...Robert On Fri, Nov 7, 2008 at 8:24 PM, Dirk Riehle <[EMAIL PROTECTED]> wrote: > Hi, > > I have a short CVS question please: How do I go from a particular file > revision like > > pgsql/cvs/pgsql/src/backend/parser/parse_relation.c.1.3 > > to the complete commit? I.e. I would like to navigate back from this > particular file to the commit and see all the other files that were touched > by the commit. > > Thanks! > > Dirk > > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > -- 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] Windowing Function Patch Review -> Standard Conformance
2008/11/9 David Rowley <[EMAIL PROTECTED]>: > Hitoshi Harada Wrote: >> > although attached is the whole (split) patch. > > I'm having some trouble getting these patches to patch cleanly. I think it's > because of this that I can't get postgres to compile after applying the > patch. > > It errors out at tuptoaster.c some constants seem to be missing from > fmgroids.h > > If I open src/include/utils/fmgroids.h > > The only constaint being defined is: > > #define F__NULL_ 31 > > I'd assume it was a problem with my setup, but the CVS head compiles fine. > > Let me know if I'm doing something wrong. > patching file src/include/catalog/pg_proc.h > Hunk #4 FAILED at 106. > 1 out of 4 hunks FAILED -- saving rejects to file > src/include/catalog/pg_proc.h.rej As it says pg_proc.h may have conflicts. The patch is not against HEAD but based on the same as the previous (v08) patch. I am remote now so I'm not sure but could you try to find conflicts in pg_proc.h? The pg_proc catalog has been added 1 column called prociswfunc (bool) in the patch. All you have to do is add 'f' in the middle of new-coming lines. Sorry for annoying, and I'll be back in hours. Regards, -- 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] Patch for SQL-Standard Interval output and decoupling DateStyle from IntervalStyle
Tom Lane wrote:
Ron Mayer <[EMAIL PROTECTED]> writes:
Rather than forcing Postgres mode; couldn't it put a
"set intervalstyle = [whatever the current interval style is]"
in the dump file?
This would work for loading into a PG >= 8.4 server, and fail miserably
for loading into pre-8.4 servers. Even though we don't guarantee
backwards compatibility of dump files, I'm loath to adopt a solution
that will successfully load wrong data into an older server.
How is the case different from standard_conforming_strings; where ISTM
depending on postgresql.conf 8.4 will happily dump either
SET standard_conforming_strings = off;
...
INSERT INTO dumptest VALUES ('');
or
SET standard_conforming_strings = on;
...
INSERT INTO dumptest VALUES ('\\');
and AFAICT the latter will happily load wrong data into 8.1 with
only the error message
ERROR: parameter "standard_conforming_strings" cannot be changed
I imagine the use-case for "standard_conforming_strings = 0 " and
"intervalstyle = sql_standard" are petty similar as well.
I wonder if the best solution is that any dump file with
standard_conforming_strings=on include some SQL that will refuse
to load in pre-8.2 systems; and that any dump file with
intervalstyle=sql_standard refuse to load in pre-8.4 systems.
It seems pretty easy to add a sql fragment that checks version()
and put that in the beginning of a dump file that uses these GUCs
to enforce this.
--
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] Windowing Function Patch Review -> Standard Conformance
Hitoshi Harada Wrote: > > although attached is the whole (split) patch. I'm having some trouble getting these patches to patch cleanly. I think it's because of this that I can't get postgres to compile after applying the patch. It errors out at tuptoaster.c some constants seem to be missing from fmgroids.h If I open src/include/utils/fmgroids.h The only constaint being defined is: #define F__NULL_ 31 I'd assume it was a problem with my setup, but the CVS head compiles fine. Let me know if I'm doing something wrong. Here is the output from patch and the compile errors. [EMAIL PROTECTED]:~/pg8.4/pgsql$ patch -p1 < window_functions.patch.20081107-1 patching file doc/src/sgml/func.sgml Hunk #1 succeeded at 10061 (offset 11 lines). patching file doc/src/sgml/keywords.sgml patching file doc/src/sgml/queries.sgml patching file doc/src/sgml/query.sgml patching file doc/src/sgml/ref/select.sgml patching file doc/src/sgml/syntax.sgml patching file src/backend/catalog/pg_aggregate.c patching file src/backend/catalog/pg_proc.c patching file src/backend/commands/explain.c patching file src/backend/executor/Makefile patching file src/backend/executor/execAmi.c patching file src/backend/executor/execGrouping.c patching file src/backend/executor/execProcnode.c patching file src/backend/executor/execQual.c Hunk #3 succeeded at 4167 (offset 14 lines). patching file src/backend/executor/nodeAgg.c patching file src/backend/executor/nodeWindow.c patching file src/backend/nodes/copyfuncs.c patching file src/backend/nodes/equalfuncs.c patching file src/backend/nodes/nodeFuncs.c patching file src/backend/nodes/outfuncs.c patching file src/backend/nodes/readfuncs.c patching file src/backend/optimizer/path/allpaths.c patching file src/backend/optimizer/path/equivclass.c patching file src/backend/optimizer/plan/createplan.c patching file src/backend/optimizer/plan/planagg.c patching file src/backend/optimizer/plan/planmain.c patching file src/backend/optimizer/plan/planner.c patching file src/backend/optimizer/plan/setrefs.c patching file src/backend/optimizer/plan/subselect.c patching file src/backend/optimizer/prep/prepjointree.c patching file src/backend/optimizer/util/clauses.c patching file src/backend/optimizer/util/tlist.c patching file src/backend/optimizer/util/var.c patching file src/backend/parser/analyze.c patching file src/backend/parser/gram.y Hunk #6 succeeded at 6433 (offset 8 lines). Hunk #7 succeeded at 6443 (offset 8 lines). Hunk #8 succeeded at 8212 (offset 9 lines). Hunk #9 succeeded at 8220 (offset 9 lines). Hunk #10 succeeded at 8232 (offset 9 lines). Hunk #11 succeeded at 8244 (offset 9 lines). Hunk #12 succeeded at 8256 (offset 9 lines). Hunk #13 succeeded at 8272 (offset 9 lines). Hunk #14 succeeded at 8284 (offset 9 lines). Hunk #15 succeeded at 8306 (offset 9 lines). Hunk #16 succeeded at 8754 (offset 9 lines). Hunk #17 succeeded at 9629 (offset 9 lines). Hunk #18 succeeded at 9637 (offset 9 lines). Hunk #19 succeeded at 9703 (offset 9 lines). Hunk #20 succeeded at 9720 (offset 9 lines). Hunk #21 succeeded at 9772 (offset 9 lines). Hunk #22 succeeded at 9959 (offset 9 lines). Hunk #23 succeeded at 9980 (offset 9 lines). patching file src/backend/parser/keywords.c patching file src/backend/parser/parse_agg.c patching file src/backend/parser/parse_clause.c patching file src/backend/parser/parse_expr.c patching file src/backend/parser/parse_func.c patching file src/backend/rewrite/rewriteManip.c patching file src/backend/utils/adt/Makefile patching file src/backend/utils/adt/ruleutils.c patching file src/backend/utils/adt/wfunc.c patching file src/backend/utils/sort/tuplestore.c patching file src/include/catalog/pg_attribute.h patching file src/include/catalog/pg_class.h [EMAIL PROTECTED]:~/pg8.4/pgsql$ patch -p1 < window_functions.patch.20081107-2 (Stripping trailing CRs from patch.) patching file src/include/catalog/pg_proc.h Hunk #4 FAILED at 106. 1 out of 4 hunks FAILED -- saving rejects to file src/include/catalog/pg_proc.h.rej (Stripping trailing CRs from patch.) patching file src/include/executor/executor.h (Stripping trailing CRs from patch.) patching file src/include/executor/nodeWindow.h (Stripping trailing CRs from patch.) patching file src/include/nodes/execnodes.h Hunk #3 succeeded at 509 (offset 2 lines). Hunk #4 succeeded at 1586 (offset 2 lines). (Stripping trailing CRs from patch.) patching file src/include/nodes/nodes.h (Stripping trailing CRs from patch.) patching file src/include/nodes/parsenodes.h (Stripping trailing CRs from patch.) patching file src/include/nodes/plannodes.h (Stripping trailing CRs from patch.) patching file src/include/nodes/primnodes.h (Stripping trailing CRs from patch.) patching file src/include/nodes/relation.h (Stripping trailing CRs from patch.) patching file src/include/optimizer/planmain.h (Stripping trailing CRs from patch.) patching file src/include/optimizer/var.h (Stripping trailing CRs from patch.) patching file src/include/parser/parse_agg.h (S
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1197)
Simon Riggs wrote: > On Sat, 2008-11-08 at 18:58 +0900, KaiGai Kohei wrote: > >> This document gives us some of hints to be considered when we >> apply mandatory access control facilities on database systems. >> >> However, it is not a specification of SE-PostgreSQL. >> The series of documents assumes traditional multi-level-security >> system, so it does not care about flexible policy, type-enforcement >> rules and collaborating with operating system. > > I'm sorry, but I don't understand your answer. What I wanted to say is that the security design of SELinux is combination of TE(type enforcement), RBAC(role based access controls) and MLS(multi level security) so we cannot apply specification of the document as-is. In addition, its security policy is not hard-wired. These differences gives us some more technical hurdles. > The wiki seemed to indicate, to me, that the FK situation was a problem, > so I was trying to provide a solution. Personally, I could live with it > either way. But the important thing is: will this aspect prevent > SEPostgreSQL from achieving Common Criteria certification, or not? Please note that I've learned the common criteria for a few years but not a authority, and the answer may depends on certification agency. In my understanding, it depends on assurance level of the certification and what functional components are required by the its environment to be used and threats to be considered here. If we don't consider who can be a sponsor of the certification, it has enough functionalities to pass the certification expect for extreme requirements which well over enterprise class systems. The covert channel analysis is contained at the FDP_IFF section in the Common Criteria part 2, and it defines several classes of requirements in information flow controls. It defines six components and FDP_IFF.3, 4, 5 mentions the handling of covert channels, but the 3 and 4 does not require there is no covert channels. FYI, some of certified database products also don't mention them. For example, the certified Oracle Label Security 10g is evaluated as EAL4+ class, but it mentions only FDP_IFF.2, not 3, 4 and 5. The FDP_IFF.2 is label based mandatory access controls as SE-PostgreSQL provides. > Please could you update the Wiki docs to explain the agreed > resolution, its reasons and references? The design choices we make will > be questioned again in the future, so it will be good to have them > clear. Thanks. OK, I'll add it to the wiki document. Thanks, -- KaiGai Kohei <[EMAIL PROTECTED]> -- 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] Updates of SE-PostgreSQL 8.4devel patches (r1197)
Simon Riggs wrote: > On Sat, 2008-11-08 at 14:21 +0900, KaiGai Kohei wrote: > >>> Some users will be able to take advantage of the facilities without >>> implementing full MLS. Yet we want the full facilities for Government. >>> Many people currently run multiple customers in different schemas, >>> though this would let them just run a single set of tables so is much >>> better for running many small customers. >> SELinux community also provides a MLS enabled policy, but it is not >> a default one now. SE-PostgreSQL has all the its access controls >> decision externally, so it is out of our coltrols. > > I have a couple of concerns: > > 1. if we make a direct link with SELinux then the code will be *much* > less used and tested. It will be a constant battle to maintain > SEPostgres in a bug-free state. I want to decouple the link so this code > goes into normal Postgres. Other comments below are made with that > thought in mind. I don't deny that SELinux has not been our majority yet. However, I also think other configuratin options have similar issues less or more. We cannot test all the combinations of the options, so it should not be a reason. > 2. I see another use case here. For example, a customer runs a > Software-as-a-service company where the same service is offered to 500 > customers. The same database schema is there for each customer, yet they > must never see each other's data. Today, that is implemented as 500 > schemas, plus 1 schema with common data in it. ISTM we would be able to > implement this better using SEPostgreSQL, with/without using SELinux. > The need for common data is why in some cases we would want access > controls placed only on certain tables. > > Fulfilling use case (2) will also meet my concerns in (1). We can assign a proper label for common tables, which allows everyone to access it. I guess it can be implement wih MCS policy so well. Any customer has its individual category (c0 to c499) and their data is also labeled as same category, but the common table is kept uncategorized. > So I would prefer it if the solution was designed closely with SELinux, > but usable and useful in other cases also. The link to SELinux could be > done via a contrib plugin. A plugin is then optional. But the main > plugin we provide can be built directly with SELinux. The reason why SE-PostgreSQL feature is implemented on the PGACE security framework is to provide end-users options. We can switch preferable security feature or turn it off at this level. If necessary, we can implement another option on the PGACE. The rowacl is proof of the concept. It seems to me you consider a pluggable interface to operating system enables to apply other security server, but, they can have different security model, different security attribute and so on. I think it is an appropriate decision to switch security features at the hook level which does not provide any security model, more than integration of different security design. >>> The security context on each row could be an optional column present >>> only if HEAP_HASSECURITYCONTEXT is set (0x0010 see htup.h), just like >>> OIDs. Use a specific datatype rather than TEXT. That datatype could be >>> an identifier to pg_security. Security people have big databases too, so >>> we need to compress the security context more and take out parse time of >>> string handling. Don't think we should use Oids, they're too big. Might >>> be easier to use a 2byte field and restrict access to 32,000 contexts, >>> which is easily enough. TEXT also makes me nervous, just in case there >>> is some collation/encoding weirdness that allows contexts to be >>> subverted. Fixed integers are hard to compromise in that respect. >> An issue is who can decide the existance or needs of security system >> attribute. If the table owner can disable it, we cannot say it as >> a mandatory access control feature, so the security attribute has to >> be always appeared when the security feature is enabled. >> >> Here is an answer for the expected question. >> When we refer the "security_context" system column of tuples without >> HEAP_HAS_SECURITY, it returns an alternative label called as "unlabeled_t", >> because it has no labels. > > Not really an issue. Just use a parameter. security_controls = mandatory > or change the meaning of the existing one. Similar to default_with_oids, > just that we have a setting where it isn't optional. This choice should be contained in the security model which can be choosen at the PGACE hook level. At least, I cannot imagine SELinux without mandatory feature. However, I don't deny other security model which allows mandatory or discretionary. >> The reason why we adopt TEXT type is SELinux requires userspace >> object manager makes queries via text represented security context, >> and it also can be used for other security feature to show client >> its security attribute in human readable format. > > AFAICS, neither of those things means t
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1197)
On Sat, 2008-11-08 at 18:58 +0900, KaiGai Kohei wrote: > This document gives us some of hints to be considered when we > apply mandatory access control facilities on database systems. > > However, it is not a specification of SE-PostgreSQL. > The series of documents assumes traditional multi-level-security > system, so it does not care about flexible policy, type-enforcement > rules and collaborating with operating system. I'm sorry, but I don't understand your answer. The wiki seemed to indicate, to me, that the FK situation was a problem, so I was trying to provide a solution. Personally, I could live with it either way. But the important thing is: will this aspect prevent SEPostgreSQL from achieving Common Criteria certification, or not? If it will pass, then I'm happy, even if a different, better solution exists. If it will fail, then we must act. I'm not qualified to say what will happen, but it would be good to see a very clear answer on this. If it was already resolved, then please accept my apologies for raising the issue again. Please could you update the Wiki docs to explain the agreed resolution, its reasons and references? The design choices we make will be questioned again in the future, so it will be good to have them clear. Thanks. -- Simon Riggs www.2ndQuadrant.com PostgreSQL Training, Services and 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] pg_stop_backup wait bug fix
On Fri, 2008-11-07 at 18:32 -0300, Alvaro Herrera wrote: > Simon Riggs wrote: > > Minor bug fix for pg_stop_backup() to prevent it waiting longer than > > necessary in certain circumstances. > > As far as I can tell, this patch needs to be applied to HEAD, 8.3 and > 8.2 (when xlog switch was implemented), and in fact it applies cleanly > to them. Please confirm. There are other patches that we should consider along with this one. This particular patch only has meaning when applied with the changes to pg_stop_backup() earlier in 8.4dev cycle. It isn't a bug in xlog switch, it is a bug in how the new waiting code interprets the return value from xlog switch. -- Simon Riggs www.2ndQuadrant.com PostgreSQL Training, Services and 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] auto_explain contrib moudle
Jeff Davis wrote: > I still don't understand why this psql patch is desirable. Who sets > their client_min_messages to LOG in psql? And if they do, why would they > expect different behavior that they always got from the already-existing > GUC log_min_duration_statement? > I know a few ;) In my environment the auto-explain is especially useful when used from within psql. Server logs are not always easy to get to, and it is difficult to extract the interesting bits (large files and lots of log traffic). For me the primary use of auto-explain would be interactive troubleshooting. The troublesome statements usually involve several nested function calls and are tedious to trace manually. With auto-explain I fire up psql, load the module, set the client log level, run the statements and immediately see what's going on. I bet that lot of the developers and QA folk would use it similarly. You are of course right about the log_min_duration_statement, also the log_executor_stats etc. behave similarly. So indeed, the "ignore notices" patch is not necessarily part of auto-explain. Regards, Martin -- 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] Updates of SE-PostgreSQL 8.4devel patches (r1197)
Simon Riggs wrote: > On Fri, 2008-11-07 at 16:52 -0500, Bruce Momjian wrote: >> Simon Riggs wrote: > So if somebody with context x tries to delete value1 from TableB, they > will be refused because of a row they cannot see. In this case the > correct action is to update the tuple in TableB so it now has a > security_context = y. The user with x cannot see it and can be persuaded > he deleted it, while the user with y can still see it. It seems odd for a low-privilege user to be able to elevate the privilege of a tuple above their own privilege level. I also don't believe that the privilege level is a total order, which might make this something of a sticky wicket. But those are just my thoughts as a non-guru. >>> The low-privilege user isn't elevating the label. If the tuple was >>> visible by multiple labels it was already elevated. All I am suggesting >>> is the system remove the one it can see, leaving the other ones intact. >>> This makes the row appear to be deleted by the lower privileged user, >>> whereas in fact it was merely updated. There need not be any ordering to >>> the labels for this scheme to work. >> Simon, would you read the chapter on "covert channels"? You might >> understand it better than I do and it might give you some ideas: >> >> http://citeseerx.ist.psu.edu/viewdoc/summary?doi=10.1.1.33.5950 > > It's probably easier just to say "here is the specification we;re > working to implement". This document gives us some of hints to be considered when we apply mandatory access control facilities on database systems. However, it is not a specification of SE-PostgreSQL. The series of documents assumes traditional multi-level-security system, so it does not care about flexible policy, type-enforcement rules and collaborating with operating system. Thanks, -- KaiGai Kohei <[EMAIL PROTECTED]> -- 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] [PATCHES] Infrastructure changes for recovery (v8)
On Fri, 2008-11-07 at 15:44 -0800, Jeff Davis wrote: > Is there a way to avoid wiping A and making a new base backup? rsync -- Simon Riggs www.2ndQuadrant.com PostgreSQL Training, Services and 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] Updates of SE-PostgreSQL 8.4devel patches (r1197)
On Sat, 2008-11-08 at 14:21 +0900, KaiGai Kohei wrote: > > Some users will be able to take advantage of the facilities without > > implementing full MLS. Yet we want the full facilities for Government. > > Many people currently run multiple customers in different schemas, > > though this would let them just run a single set of tables so is much > > better for running many small customers. > > SELinux community also provides a MLS enabled policy, but it is not > a default one now. SE-PostgreSQL has all the its access controls > decision externally, so it is out of our coltrols. I have a couple of concerns: 1. if we make a direct link with SELinux then the code will be *much* less used and tested. It will be a constant battle to maintain SEPostgres in a bug-free state. I want to decouple the link so this code goes into normal Postgres. Other comments below are made with that thought in mind. 2. I see another use case here. For example, a customer runs a Software-as-a-service company where the same service is offered to 500 customers. The same database schema is there for each customer, yet they must never see each other's data. Today, that is implemented as 500 schemas, plus 1 schema with common data in it. ISTM we would be able to implement this better using SEPostgreSQL, with/without using SELinux. The need for common data is why in some cases we would want access controls placed only on certain tables. Fulfilling use case (2) will also meet my concerns in (1). So I would prefer it if the solution was designed closely with SELinux, but usable and useful in other cases also. The link to SELinux could be done via a contrib plugin. A plugin is then optional. But the main plugin we provide can be built directly with SELinux. > > The security context on each row could be an optional column present > > only if HEAP_HASSECURITYCONTEXT is set (0x0010 see htup.h), just like > > OIDs. Use a specific datatype rather than TEXT. That datatype could be > > an identifier to pg_security. Security people have big databases too, so > > we need to compress the security context more and take out parse time of > > string handling. Don't think we should use Oids, they're too big. Might > > be easier to use a 2byte field and restrict access to 32,000 contexts, > > which is easily enough. TEXT also makes me nervous, just in case there > > is some collation/encoding weirdness that allows contexts to be > > subverted. Fixed integers are hard to compromise in that respect. > > An issue is who can decide the existance or needs of security system > attribute. If the table owner can disable it, we cannot say it as > a mandatory access control feature, so the security attribute has to > be always appeared when the security feature is enabled. > > Here is an answer for the expected question. > When we refer the "security_context" system column of tuples without > HEAP_HAS_SECURITY, it returns an alternative label called as "unlabeled_t", > because it has no labels. Not really an issue. Just use a parameter. security_controls = mandatory or change the meaning of the existing one. Similar to default_with_oids, just that we have a setting where it isn't optional. > The reason why we adopt TEXT type is SELinux requires userspace > object manager makes queries via text represented security context, > and it also can be used for other security feature to show client > its security attribute in human readable format. AFAICS, neither of those things means the datatype has to be TEXT. > For your information, in-kernel SELinux can handle 2^32 - 1 of security > context internally in theoretical maximum, so using Oid as a security > identifier is a fair decision to avoid an implementation to handle overflow > cases. OK, so we need 4 bytes. I can live with that - at least its not ~10 bytes/row. Does it need to be an Oid, or just the size of an Oid? > > The section on LOAD doesn't sound very convincing. Loading a module > > could immediately subvert security. We could probably tighten up on that > > for general use as well. ISTM we need something like a new catalog table > > for loadable modules, so we can give them specific security contexts and > > potentially store some kind of verification information about them. > > Having a single "can load modules" isn't enough with Postgres, since it > > would effectively prevent us from loading *any* modules in a secure > > database. Which kinda removes much of the benefit of using Postgres. > > SE-PostgreSQL applies access controls for individual loadable modules. > When a function implemented within an external modules tries to be loaded, > it checks security context between the database and the file of loadable > modules. (No need to say, in-kernel SELinux assigns its security context > for files in common format, so we can compare them each other.) > > Perhaps, the description I wrote can easily make misunderstanding. > If you can read it says widespread load modules permission, I'll r
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1197)
On Fri, 2008-11-07 at 16:52 -0500, Bruce Momjian wrote: > Simon Riggs wrote: > > > > So if somebody with context x tries to delete value1 from TableB, they > > > > will be refused because of a row they cannot see. In this case the > > > > correct action is to update the tuple in TableB so it now has a > > > > security_context = y. The user with x cannot see it and can be persuaded > > > > he deleted it, while the user with y can still see it. > > > > > > It seems odd for a low-privilege user to be able to elevate the > > > privilege of a tuple above their own privilege level. I also don't > > > believe that the privilege level is a total order, which might make > > > this something of a sticky wicket. But those are just my thoughts as > > > a non-guru. > > > > The low-privilege user isn't elevating the label. If the tuple was > > visible by multiple labels it was already elevated. All I am suggesting > > is the system remove the one it can see, leaving the other ones intact. > > This makes the row appear to be deleted by the lower privileged user, > > whereas in fact it was merely updated. There need not be any ordering to > > the labels for this scheme to work. > > Simon, would you read the chapter on "covert channels"? You might > understand it better than I do and it might give you some ideas: > > http://citeseerx.ist.psu.edu/viewdoc/summary?doi=10.1.1.33.5950 It's probably easier just to say "here is the specification we;re working to implement". -- Simon Riggs www.2ndQuadrant.com PostgreSQL Training, Services and 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] pg_dump roles support
Hi, Thank you for your review. On 2008-11-07 21:20, Alvaro Herrera wrote: The patch contains the following things: - pg_dump and pg_dumpall accepts the --role=rolename parameter, and sends a SET ROLE command on their connections Minor comment -- I think you need to quote the role name in the SET command. Otherwise roles with funny names will fail (try a role with a space for example) Of course you need to quote the role names with special characters in it. I tested it this way (from bash): $ src/bin/pg_dump/pg_dump -h localhost -p 4003 --role "asd ' \" qwe" test Note the bash style escaping of the string [asd ' " qwe]. It created a dump file with SET role = "asd ' "" qwe"; line in it. Seems fine for me. The SGML patch seems to contain unnecessary whitespace changes; please clean that up. Maybe you missed an updated version of the patch? Available here: http://archives.postgresql.org/pgsql-hackers/2008-11/msg00391.php + /* te->defn should have the form SET role = 'foo'; */ + char *defn = strdup(te->defn); + char *ptr1; + char *ptr2 = NULL; + + ptr1 = strchr(defn, '\''); + if (ptr1) + ptr2 = strchr(++ptr1, '\''); Does this work if the role name contains a ' ? Right, this one fails with ' in the role name. An update coming soon closing this issue. Regards, Benedek Laszlo -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Short CVS question
Hi, I have a short CVS question please: How do I go from a particular file revision like pgsql/cvs/pgsql/src/backend/parser/parse_relation.c.1.3 to the complete commit? I.e. I would like to navigate back from this particular file to the commit and see all the other files that were touched by the commit. Thanks! Dirk -- 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] Updates of SE-PostgreSQL 8.4devel patches (r1197)
Simon, Thanks for your comments. > Some initial thoughts based upon reading the Wiki. I've not been > involved in things up to now, so if this dredges up old discussions, > well, these are my thoughts. > > I want SEPostgreSQL, but I'd like it to work without needing to be a > compile time option so people can just use it as/when needed. Plus we > don't want to support what would be/is essentially a fork of Postgres. > Most CPUs will optimise away a simple if-test in various places. As Bruce also mentioned, it has to be linked with libselinux to communicate in-kernel SELinux, but it is not onw of the universal libraries, so, it has to be enabled/disabled on build-time option. In addition, we can also disable the feature by the following configuration. sepostgresql = disabled (at $PGDATA/postgresql.conf ) TODO: add a description about the guc variable. It can have four state: "default", "enforcing", "permissive" and "disabled". > Some users will be able to take advantage of the facilities without > implementing full MLS. Yet we want the full facilities for Government. > Many people currently run multiple customers in different schemas, > though this would let them just run a single set of tables so is much > better for running many small customers. SELinux community also provides a MLS enabled policy, but it is not a default one now. SE-PostgreSQL has all the its access controls decision externally, so it is out of our coltrols. > I'm very unhappy about putting a nonoptional value on tuple headers, > especially because it is updatable. Do we expect MVCC will work with > updatable security contexts? i.e. when you update the security context > of a tuple that won't effect its visibility to existing system users. I > can't imagine you'd want that would you? It's kind of difficult to *not* > get it though. When a user updates the security context of some tuples, it is invisible for other clients until its commit, like as other normal data. Sorry, it is unclear for me what is the concern you mention. > Looks to me that this feature is useless without things working at row > level. So we can't leave that part out. I guess you are saying the core PostgreSQL also has table and column level granularities, but its criteria to make a decision is different. SE-PostgreSQL makes its decision based on the privileges of peer process, not a database role. > The security context on each row could be an optional column present > only if HEAP_HASSECURITYCONTEXT is set (0x0010 see htup.h), just like > OIDs. Use a specific datatype rather than TEXT. That datatype could be > an identifier to pg_security. Security people have big databases too, so > we need to compress the security context more and take out parse time of > string handling. Don't think we should use Oids, they're too big. Might > be easier to use a 2byte field and restrict access to 32,000 contexts, > which is easily enough. TEXT also makes me nervous, just in case there > is some collation/encoding weirdness that allows contexts to be > subverted. Fixed integers are hard to compromise in that respect. An issue is who can decide the existance or needs of security system attribute. If the table owner can disable it, we cannot say it as a mandatory access control feature, so the security attribute has to be always appeared when the security feature is enabled. Here is an answer for the expected question. When we refer the "security_context" system column of tuples without HEAP_HAS_SECURITY, it returns an alternative label called as "unlabeled_t", because it has no labels. The reason why we adopt TEXT type is SELinux requires userspace object manager makes queries via text represented security context, and it also can be used for other security feature to show client its security attribute in human readable format. For your information, in-kernel SELinux can handle 2^32 - 1 of security context internally in theoretical maximum, so using Oid as a security identifier is a fair decision to avoid an implementation to handle overflow cases. > How will unique indexes work? Do you implicitly add security context as > last column on every unique index, or does the uniqueness violation only > occurs within security contexts, or does the uniqueness violation tested > against all contextx that the inserter can currently see? Is there a > change to system catalogs? The unique/primary key constraint works at the lowest level. So, it has a possibility that invisible tuple prevent to insert a tuple. > Foreign Key deletions could be handled correctly if you treat them as > updates. If we have the following example > > TableA > security_context=y value=2 fk=1 > > TableB > security_context=x value=1 > > TableA refers to TableB. Context x cannot see context y. > > So if somebody with context x tries to delete value1 from TableB, they > will be refused because of a row they cannot see. In this case the > correct action is to update the tuple in TableB so it
Re: [HACKERS] Updates of SE-PostgreSQL 8.4devel patches (r1197)
Simon Riggs wrote: > On Fri, 2008-11-07 at 15:12 -0500, Robert Haas wrote: >>> Foreign Key deletions could be handled correctly if you treat them as >>> updates. If we have the following example >>> >>> TableA >>> security_context=y value=2 fk=1 >>> >>> TableB >>> security_context=x value=1 >>> >>> TableA refers to TableB. Context x cannot see context y. >>> >>> So if somebody with context x tries to delete value1 from TableB, they >>> will be refused because of a row they cannot see. In this case the >>> correct action is to update the tuple in TableB so it now has a >>> security_context = y. The user with x cannot see it and can be persuaded >>> he deleted it, while the user with y can still see it. >> It seems odd for a low-privilege user to be able to elevate the >> privilege of a tuple above their own privilege level. I also don't >> believe that the privilege level is a total order, which might make >> this something of a sticky wicket. But those are just my thoughts as >> a non-guru. > > The low-privilege user isn't elevating the label. If the tuple was > visible by multiple labels it was already elevated. All I am suggesting > is the system remove the one it can see, leaving the other ones intact. > This makes the row appear to be deleted by the lower privileged user, > whereas in fact it was merely updated. There need not be any ordering to > the labels for this scheme to work. SELinux does not allow a object has two or more labels. You implicitly assume the security mechanism allows users to access when one of the labels allows it, but there is no consensus. At the past, an idea of multiple labels is discussed in SELinux community, but it was finally dropped because we cannot define the behavior when the security policy makes different decision for two different labels. Thanks, -- KaiGai Kohei <[EMAIL PROTECTED]> -- 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] restore PD_PAGE_FULL on WAL update replay
On Fri, Nov 7, 2008 at 9:12 PM, Alvaro Herrera <[EMAIL PROTECTED]> wrote: > This patch ensures that the PD_PAGE_FULL bit is restored after replaying > a heap_update WAL record. I think this must have been overlooked on the > HOT patch. > > Since PD_PAGE_FULL is just a hint, I think we might have deliberately left it that way. Even if we want to fix this, we should also replay the action of clearing the bit in heap_xlog_clean() Thanks, Pavan -- Pavan Deolasee EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
