Re: [HACKERS] Perl coding error in msvc build system?
On Tue, Feb 17, 2015 at 6:34 AM, Peter Eisentraut wrote: >> This patch been reviewed by 4 people, resulting in 2 minor suggestions >> for changes (adding an m modifier, and removing a bogus last). >> >> It has 2 clear upvotes and 0 downvotes. >> >> I think it should be revised along the lines suggested and committed >> without further ado. > > My patch actually only covered the first of the two faulty locations I > pointed out. Attached is a patch that also fixes the second one. I > noticed that DetermineVisualStudioVersion() is never called with an > argument, so I removed that branch altogether. +1 for those simplifications. And FWIW, it passed my series of tests with MSVC 2010. -- Michael -- 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] Strange assertion using VACOPT_FREEZE in vacuum.c
On Wed, Feb 18, 2015 at 10:06 AM, Michael Paquier wrote: > Yes, the existing assertion is right. My point is that it is strange > that we do not check the values of freeze parameters for an ANALYZE > query, which should be set to -1 all the time. If this is thought as > not worth checking, I'll drop this patch and my concerns. Perhaps this explains better what I got in mind, aka making the assertion stricter: Assert((vacstmt->options & VACOPT_VACUUM) || - !(vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE))); + ((vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE)) == 0 && + vacstmt->freeze_min_age < 0 && + vacstmt->freeze_table_age < 0 && + vacstmt->multixact_freeze_min_age < 0 && + vacstmt->multixact_freeze_table_age < 0)); Regards, -- Michael diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 2f3f79d..e1472ad 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -110,7 +110,11 @@ vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast, /* sanity checks on options */ Assert(vacstmt->options & (VACOPT_VACUUM | VACOPT_ANALYZE)); Assert((vacstmt->options & VACOPT_VACUUM) || - !(vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE))); + ((vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE)) == 0 && + vacstmt->freeze_min_age < 0 && + vacstmt->freeze_table_age < 0 && + vacstmt->multixact_freeze_min_age < 0 && + vacstmt->multixact_freeze_table_age < 0)); Assert((vacstmt->options & VACOPT_ANALYZE) || vacstmt->va_cols == NIL); stmttype = (vacstmt->options & VACOPT_VACUUM) ? "VACUUM" : "ANALYZE"; -- 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] pgaudit - an auditing extension for PostgreSQL
At 2015-02-17 13:01:46 -0500, sfr...@snowman.net wrote: > > I have to admit that I'm confused by this. Patches don't stabilise > through sitting in the archives, they stabilise when the comments are > being addressed, the patch updated, and further comments are > addressing less important issues. The issues which Robert and I had > both commented on didn't appear to be getting addressed. I'm confused and unhappy about your characterisation of the state of this patch. You make it seem as though there was broad consensus about the changes that were needed, and that I left the patch sitting in the archives for a long time without addressing important issues. You revised and refined your GRANT proposal in stages, and I tried to adapt the code to suit. I'm sorry that my efforts were not fast enough or responsive enough to make you feel that progress was being made. But nobody else commented in detail on the GRANT changes except to express general misgivings, and nobody else even disagreed when I inferred, in light of the lack of consensus that Robert pointed out, that the code was unlikely to make it into 9.5. Given that I've maintained the code over the past year despite its being rejected in an earlier CF, and given the circumstances outlined above, I do not think it's reasonable to conclude after a couple of weeks without a new version that it was abandoned. As I had mentioned earlier, there are people who already use pgaudit as-is, and complain if I break it. Anyway, I guess there is no such thing as a constructive history discussion, so I'll drop it. -- Abhijit -- 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] ADD FOREIGN KEY locking
On Wed, Feb 18, 2015 at 9:06 AM, James Sewell wrote: > I've just noticed something in the Commit fest post > - Reducing lock strength of trigger and foreign key DDL > This reduces the lock taken for ADD FOREIGN KEY to ShareRowExclusiveLock, authorizing SELECT and SELECT FOR [SHARE | UPDATE ... ]. > Perhaps I just need to be more patient. > Yup. -- Michael
Re: [HACKERS] Sequence Access Method WIP
On 18/02/15 02:59, Petr Jelinek wrote: On 17/02/15 23:11, Robert Haas wrote: On Sun, Feb 15, 2015 at 1:40 PM, Petr Jelinek wrote: sending new version that is updated along the lines of what we discussed at FOSDEM, which means: - back to single bytea amdata column (no custom columns) Well, the main argument is still future possibility of moving into single table for storage. And when we discussed about it in person we agreed that there is not too big advantage in having separate columns since there need to be dump/restore state interfaces anyway so you can see the relevant state via those as we made the output more human readable (and the psql support reflects that). Also makes the patch a little simpler obviously. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- 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] Sequence Access Method WIP
On 17/02/15 23:11, Robert Haas wrote: On Sun, Feb 15, 2015 at 1:40 PM, Petr Jelinek wrote: sending new version that is updated along the lines of what we discussed at FOSDEM, which means: - back to single bytea amdata column (no custom columns) Well, the main argument is still future possibility of moving into single table for storage. And when we discussed about it in person we agreed that there is not too big advantage in having separate columns since there need to be dump/restore state interfaces anyway so you can see the relevant state via those as we made the output more human readable (and the psql support reflects that). -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- 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] mogrify and indent features for jsonb
Hi,
I looked at the patch and have several comments.
First let me say that modifying the individual paths of the json is the
feature I miss the most in the current implementation so I am happy that
this patch was submitted.
I would prefer slightly the patch split into two parts, one for the
indent printing and one with the manipulation functions, but it's not
too big patch so it's not too bad as it is.
There is one compiler warning that I see:
jsonfuncs.c:3533:1: warning: no previous prototype for
‘jsonb_delete_path’ [-Wmissing-prototypes]
jsonb_delete_path(PG_FUNCTION_ARGS)
I think it would be better if the ident printing didn't put the start of
array ([) and start of dictionary ({) on separate line since most
"pretty" printing implementations outside of Postgres (like the
JSON.stringify or python's json module) don't do that. This is purely
about consistency with the rest of the world.
The json_ident might be better named as json_pretty perhaps?
I don't really understand the point of h_atoi() function. What's wrong
with using strtol like pg_atoi does? Also there is no integer overflow
check anywhere in that function.
There is tons of end of line whitespace mess in jsonb_indent docs.
Otherwise everything I tried so far works as expected. The code looks ok
as well except maybe the replacePath could use couple of comments (for
example the line which uses the h_atoi) to make it easier to follow.
About the:
+ /* XXX : why do we need this assertion? The functions is declared to
take text[] */
+ Assert(ARR_ELEMTYPE(path) == TEXTOID);
I wonder about this also, some functions do that, some don't, I am not
really sure what is the rule there myself.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
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] Add min and max execute statement time in pg_stat_statement
On 1/20/15 6:32 PM, David G Johnston wrote: > In fact, as far as > the database knows, the values provided to this function do represent an > entire population and such a correction would be unnecessary. I guess it > boils down to whether "future" queries are considered part of the population > or whether the population changes upon each query being run and thus we are > calculating the ever-changing population variance. I think we should be calculating the population variance. We are clearly taking the population to be all past queries (from the last reset point). Otherwise calculating min and max wouldn't make sense. -- 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] Strange assertion using VACOPT_FREEZE in vacuum.c
On Wed, Feb 18, 2015 at 12:09 AM, Robert Haas wrote: > On Thu, Feb 12, 2015 at 11:54 PM, Michael Paquier > wrote: >> Hi all, >> >> When calling vacuum(), there is the following assertion using VACOPT_FREEZE: >> Assert((vacstmt->options & VACOPT_VACUUM) || >> !(vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE))); >> I think that this should be changed with sanity checks based on the >> parameter values of freeze_* in VacuumStmt as we do not set up >> VACOPT_FREEZE when VACUUM is used without options in parenthesis, for >> something like that: >> Assert((vacstmt->options & VACOPT_VACUUM) || >> - !(vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE))); >> + ((vacstmt->options & VACOPT_FULL) == 0 && >> + vacstmt->freeze_min_age < 0 && >> + vacstmt->freeze_table_age < 0 && >> + vacstmt->multixact_freeze_min_age < 0 && >> + vacstmt->multixact_freeze_table_age < 0)); >> This would also have the advantage to limit the use of VACOPT_FREEZE >> in the query parser. >> A patch is attached. >> Thoughts? > > I think it's right the way it is. The parser constructs a VacuumStmt > for either a VACUUM or an ANALYZE command. That statement is checking > that if you are doing an ANALYZE, you can't specify FULL or FREEZE. > That makes sense, because there is no ANALYZE FULL or ANALYZE FREEZE > command. Yes, the existing assertion is right. My point is that it is strange that we do not check the values of freeze parameters for an ANALYZE query, which should be set to -1 all the time. If this is thought as not worth checking, I'll drop this patch and my concerns. -- Michael -- 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] Commit fest 2015-12 enters money time
Michael Paquier wrote: > On Wed, Feb 18, 2015 at 9:54 AM, Alvaro Herrera > wrote: > > Michael Paquier wrote: > >> On Wed, Feb 18, 2015 at 5:55 AM, Corey Huinker > >> wrote: > >> > What is required to get the New Patch superpower? I'm also in need of it. > >> > >> CCing Magnus... I am not sure myself, but I would imagine that the > >> commit fest needs to be "Open" and not "In Process" to be able to add > >> patches. > > > > Um. Then, would you please add this patch > > https://www.postgresql.org/message-id/20150215044814.gl3...@alvh.no-ip.org > > Here you go: > https://commitfest.postgresql.org/4/168/ Many thanks. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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] Commit fest 2015-12 enters money time
On Wed, Feb 18, 2015 at 9:54 AM, Alvaro Herrera wrote: > Michael Paquier wrote: >> On Wed, Feb 18, 2015 at 5:55 AM, Corey Huinker >> wrote: >> > What is required to get the New Patch superpower? I'm also in need of it. >> >> CCing Magnus... I am not sure myself, but I would imagine that the >> commit fest needs to be "Open" and not "In Process" to be able to add >> patches. > > Um. Then, would you please add this patch > https://www.postgresql.org/message-id/20150215044814.gl3...@alvh.no-ip.org Here you go: https://commitfest.postgresql.org/4/168/ -- Michael -- 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] Commit fest 2015-12 enters money time
Michael Paquier wrote: > On Wed, Feb 18, 2015 at 5:55 AM, Corey Huinker > wrote: > > What is required to get the New Patch superpower? I'm also in need of it. > > CCing Magnus... I am not sure myself, but I would imagine that the > commit fest needs to be "Open" and not "In Process" to be able to add > patches. Um. Then, would you please add this patch https://www.postgresql.org/message-id/20150215044814.gl3...@alvh.no-ip.org -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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] pgaudit - an auditing extension for PostgreSQL
* Jim Nasby (jim.na...@bluetreble.com) wrote: > On 2/17/15 1:10 PM, Stephen Frost wrote: > >>What I'd prefer to see is a way to decouple the OS account from any > >>>DB account. Clearly that doesn't protect us from the OS user doing > >>>something bad, but at least then there's no way for them to just > >>>silently run SQL commands. > >If the DB account isn't a superuser then everything changes. There's no > >point fighting with the OS user- they can run some other PG binary which > >they've copied over locally and run SQL with that, or copy all the files > >over to another server which they have complete access to. The fact > >that they can also connect to the DB and run SQL isn't really an issue. > > I disagree. The difference here is that the OS can audit whatever > commands they're running, but not what happens inside something like > psql. Even if you did run a keylogger, trying to use that to > interpret a psql session would be a real pain, if not impossible. So > I don't think we can rely on the OS to audit SQL at all. But > obviously if they did something like copy the files somewhere else, > or bring in a new binary, the OS would at least have record that > that happened. From my experience, logging the commands is no where near enough.. psql is hardly the only complex CLI process one can run. Further, I'm not suggesting that we rely on the OS to audit SQL, merely stating that anything which deals with auditing the connection to PG needs to be outside of the PG process space (and that of processes owned by the user which PG is running as). > Though... I wonder if there's some way we could disallow *all* > superuser access to the database, and instead create a special > non-interactive CLI. That would allow us to throw the problem over > the wall to the OS. That sounds like a horrible approach. A non-interactive CLI would be terrible and it's not clear to me how that'd really help. What happens when they run emacs or vim? > In any case, I think it's clear that there's a lot of value in at > least handling the non-SU case, so we should try and do that now. > Even if it's only in contrib. Absolutely. Glad we agree on that. :) > One thing that does occur to me though... perhaps we should > specifically disable auditing of SU activities, so we're not > providing a false sense of security. I don't agree with this. Done properly (auditing enabled on startup, using a remote auditing target, etc), it might be possible for such auditing to catch odd superuser behavior. What we don't want to do is claim that we provide full superuser auditing, as that's something we can't provide. Appropriate and clear documentation is absolutely critical, of course. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] ADD FOREIGN KEY locking
Oh, I've just noticed something in the Commit fest post - Reducing lock strength of trigger and foreign key DDL Perhaps I just need to be more patient. Cheers, James Sewell, Solutions Architect __ Level 2, 50 Queen St, Melbourne VIC 3000 *P *(+61) 3 8370 8000 *W* www.lisasoft.com *F *(+61) 3 8370 8099 On Wed, Feb 18, 2015 at 10:57 AM, James Sewell wrote: > Hello all, > > When I add a FK with a statement like this: > > ALTER TABLE a ADD FOREIGN KEY (id) REFERENCES b(id); > > I see a lock on table b: > > select locktype,mode,granted from pg_locks, pg_stat_activity where > relation::regclass::text = 'b' AND pg_locks.pid = pg_stat_activity.pid; > > locktype | relation > mode | AccessShareLock > granted | t > query | SOME LONG RUNNING QUERY WHICH SELECTS FROM b > > locktype | relation > mode | AccessExclusiveLock > granted | f > query | ALTER TABLE a ADD FOREIGN KEY (id) REFERENCES b(id); > > > This means that my add key won't complete until my long running query > does. That seems a bit odd to me? In this database there are lots of > datawarehouse type queries running, which makes it a bit hard for me to > schedule this operation. > > Is this just a manifestation of adding the key being in an ALTER TABLE, > which always needs an AccessExclusiveLock? Or am I missing some edge case > when this lock would be required in this circumstance? > > No real urgency on this question, I just found it a bit strange and > thought someone might be able to shed some light. > > James Sewell, > Solutions Architect > __ > > > Level 2, 50 Queen St, Melbourne VIC 3000 > > *P *(+61) 3 8370 8000 *W* www.lisasoft.com *F *(+61) 3 8370 8099 > > -- -- The contents of this email are confidential and may be subject to legal or professional privilege and copyright. No representation is made that this email is free of viruses or other defects. If you have received this communication in error, you may not copy or distribute any part of it or otherwise disclose its contents to anyone. Please advise the sender of your incorrect receipt of this correspondence.
[HACKERS] ADD FOREIGN KEY locking
Hello all, When I add a FK with a statement like this: ALTER TABLE a ADD FOREIGN KEY (id) REFERENCES b(id); I see a lock on table b: select locktype,mode,granted from pg_locks, pg_stat_activity where relation::regclass::text = 'b' AND pg_locks.pid = pg_stat_activity.pid; locktype | relation mode | AccessShareLock granted | t query | SOME LONG RUNNING QUERY WHICH SELECTS FROM b locktype | relation mode | AccessExclusiveLock granted | f query | ALTER TABLE a ADD FOREIGN KEY (id) REFERENCES b(id); This means that my add key won't complete until my long running query does. That seems a bit odd to me? In this database there are lots of datawarehouse type queries running, which makes it a bit hard for me to schedule this operation. Is this just a manifestation of adding the key being in an ALTER TABLE, which always needs an AccessExclusiveLock? Or am I missing some edge case when this lock would be required in this circumstance? No real urgency on this question, I just found it a bit strange and thought someone might be able to shed some light. James Sewell, Solutions Architect __ Level 2, 50 Queen St, Melbourne VIC 3000 *P *(+61) 3 8370 8000 *W* www.lisasoft.com *F *(+61) 3 8370 8099 -- -- The contents of this email are confidential and may be subject to legal or professional privilege and copyright. No representation is made that this email is free of viruses or other defects. If you have received this communication in error, you may not copy or distribute any part of it or otherwise disclose its contents to anyone. Please advise the sender of your incorrect receipt of this correspondence.
Re: [HACKERS] Configurable location for extension .control files
On 2/17/15 4:39 PM, Oskari Saarenmaa wrote: 10.06.2013, 17:51, Dimitri Fontaine kirjoitti: Andres Freund writes: In any case, no packager is going to ship an insecure-by-default configuration, which is what Dimitri seems to be fantasizing would happen. It would have to be local option to relax the permissions on the directory, no matter where it is. *I* don't want that at all. All I'd like to have is a postgresql.conf option specifying additional locations. Same from me. I think I would even take non-plural location. Here's a patch to allow overriding extension installation directory. The patch allows superusers to change it at runtime, but we could also restrict it to postgresql.conf if needed. I don't really see a point in restricting it (or not implementing the option at all) as the superuser can already load custom C code and sql from anywhere in the filesystem; not having configurable extension directory just makes it harder to test extensions and to create private clusters of PG data in a custom location without using custom binaries. I'm interested in this because it could potentially make it possible to install SQL extensions without OS access. (My understanding is there's some issue with writing the extension files even if LIBDIR is writable by the OS account). I don't think anyone should be packaging and shipping PG with extension_directory set, but we really should allow it for superusers and postgresql.conf so people can test extensions without hacks like this: https://github.com/ohmu/pgmemcache/blob/master/localtests.sh#L23 FWIW I'd like to see is breaking the cluster setup/teardown capability in pg_regress into it's own tool. It wouldn't solve the extension test problem, but it would eliminate the need for most of what that script is doing, and it would do it more robustly. It would make it very easy to unit test things with frameworks other than pg_regress. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.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] pgaudit - an auditing extension for PostgreSQL
On 2/17/15 1:10 PM, Stephen Frost wrote: What I'd prefer to see is a way to decouple the OS account from any >DB account. Clearly that doesn't protect us from the OS user doing >something bad, but at least then there's no way for them to just >silently run SQL commands. If the DB account isn't a superuser then everything changes. There's no point fighting with the OS user- they can run some other PG binary which they've copied over locally and run SQL with that, or copy all the files over to another server which they have complete access to. The fact that they can also connect to the DB and run SQL isn't really an issue. I disagree. The difference here is that the OS can audit whatever commands they're running, but not what happens inside something like psql. Even if you did run a keylogger, trying to use that to interpret a psql session would be a real pain, if not impossible. So I don't think we can rely on the OS to audit SQL at all. But obviously if they did something like copy the files somewhere else, or bring in a new binary, the OS would at least have record that that happened. Though... I wonder if there's some way we could disallow *all* superuser access to the database, and instead create a special non-interactive CLI. That would allow us to throw the problem over the wall to the OS. In any case, I think it's clear that there's a lot of value in at least handling the non-SU case, so we should try and do that now. Even if it's only in contrib. One thing that does occur to me though... perhaps we should specifically disable auditing of SU activities, so we're not providing a false sense of security. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.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] Commit fest 2015-12 enters money time
On Wed, Feb 18, 2015 at 5:55 AM, Corey Huinker wrote: > What is required to get the New Patch superpower? I'm also in need of it. CCing Magnus... I am not sure myself, but I would imagine that the commit fest needs to be "Open" and not "In Process" to be able to add patches. -- Michael -- 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] ExplainModifyTarget doesn't work as expected
Etsuro Fujita writes: > On 2015/02/10 14:49, Etsuro Fujita wrote: >> On 2015/02/07 1:09, Tom Lane wrote: >>> I think your basic idea of preserving the original parent table's relid >>> is correct; but instead of doing it like this patch does, I'd be inclined >>> to make ModifyTable inherit from Scan not Plan, and use the scan.scanrelid >>> field to carry the parent RTI. Then you would probably end up with a net >>> savings of code rather than net addition; certainly ExplainModifyTarget >>> would go away entirely since you'd just treat ModifyTable like any other >>> Scan in this part of EXPLAIN. >> Will follow your revision. > Done. Attached is an updated version of the patch. I looked at this and was not really pleased with the results, in particular the way that you'd moved a bare minimum number of the code stanzas for struct Scan so that things still compiled. The new placement of those stanzas didn't make any sense in context, and it was a significant violation of our layout rule that files dealing with various types of Nodes should whenever possible handle those Nodes in the same order that they're listed in nodes.h, so that it's clear where new bits of code ought to be placed. (I'm aware that there are historical violations of this policy in a few places, but that doesn't make it a bad policy to follow.) I experimented with relocating ModifyTable down into the group of Scan nodes in this global ordering, but soon decided that that would involve far more code churn than the idea was worth. So I went back to your v1 patch and have now committed that with some cosmetic modifications. Sorry for making you put time into a dead end. 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] sloppy back-patching of column-privilege leak
* Stephen Frost (sfr...@snowman.net) wrote: > Thanks for the review and comments. I'll remove the extraneous #define, > the comment change which was made to the other #define (as it's not > relevant since the #define is only located in one place) and fix the > regression test comments to match the behavior in the older branches. This has been done, though as noted in the commit, I simply removed the regression tests as they weren't relevant in those branches. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
Etsuro,
* Etsuro Fujita (fujita.ets...@lab.ntt.co.jp) wrote:
> On 2015/02/11 4:06, Stephen Frost wrote:
> >I had been trying to work out an FDW-specific way to address this, but I
> >think Dean's right that this should be addressed in
> >expand_security_qual(), which means it'll apply to all cases and not
> >just these FDW calls. I don't think that's actually an issue though and
> >it'll match up to how SELECT FOR UPDATE is handled today.
>
> Sorry, my explanation was not accurate, but I also agree with Dean's
> idea. In the above, I just wanted to make it clear that such a lock
> request done by expand_security_qual() should be limited to the case
> where the relation that is a former result relation is a foreign
> table.
Attached is a patch which should address this. Would love your (or
anyone else's) feedback on it. It appears to address the issue which
you raised and the regression test changes are all in-line with
inserting a LockRows into the subquery, as anticipated.
Thanks!
Stephen
From 0719cbb3b2b4c6bc1c7f52f825f1e14ec27c4b7b Mon Sep 17 00:00:00 2001
From: Stephen Frost
Date: Tue, 17 Feb 2015 15:43:33 -0500
Subject: [PATCH] Add locking clause for SB views for update/delete
In expand_security_qual(), we were handling locking correctly when a
PlanRowMark existed, but not when we were working with the target
relation (which doesn't have any PlanRowMarks, but the subquery created
for the security barrier quals still needs to lock the rows under it).
Noted by Etsuro Fujita when working with the Postgres FDW, which wasn't
properly issuing a SELECT ... FOR UPDATE to the remote side under a
DELETE.
Back-patch to 9.4 where updatable security barrier views were
introduced.
---
src/backend/optimizer/prep/prepsecurity.c | 24 ++-
src/test/regress/expected/updatable_views.out | 264 ++
2 files changed, 163 insertions(+), 125 deletions(-)
diff --git a/src/backend/optimizer/prep/prepsecurity.c b/src/backend/optimizer/prep/prepsecurity.c
index 51f10a4..bb5c397 100644
--- a/src/backend/optimizer/prep/prepsecurity.c
+++ b/src/backend/optimizer/prep/prepsecurity.c
@@ -37,7 +37,7 @@ typedef struct
} security_barrier_replace_vars_context;
static void expand_security_qual(PlannerInfo *root, List *tlist, int rt_index,
- RangeTblEntry *rte, Node *qual);
+ RangeTblEntry *rte, Node *qual, bool targetRelation);
static void security_barrier_replace_vars(Node *node,
security_barrier_replace_vars_context *context);
@@ -63,6 +63,7 @@ expand_security_quals(PlannerInfo *root, List *tlist)
Query *parse = root->parse;
int rt_index;
ListCell *cell;
+ bool targetRelation = false;
/*
* Process each RTE in the rtable list.
@@ -98,6 +99,12 @@ expand_security_quals(PlannerInfo *root, List *tlist)
{
RangeTblEntry *newrte = copyObject(rte);
+ /*
+ * We need to let expand_security_qual know if this is the target
+ * relation, as it has additional work to do in that case.
+ */
+ targetRelation = true;
+
parse->rtable = lappend(parse->rtable, newrte);
parse->resultRelation = list_length(parse->rtable);
@@ -147,7 +154,8 @@ expand_security_quals(PlannerInfo *root, List *tlist)
rte->securityQuals = list_delete_first(rte->securityQuals);
ChangeVarNodes(qual, rt_index, 1, 0);
- expand_security_qual(root, tlist, rt_index, rte, qual);
+ expand_security_qual(root, tlist, rt_index, rte, qual,
+ targetRelation);
}
}
}
@@ -160,7 +168,7 @@ expand_security_quals(PlannerInfo *root, List *tlist)
*/
static void
expand_security_qual(PlannerInfo *root, List *tlist, int rt_index,
- RangeTblEntry *rte, Node *qual)
+ RangeTblEntry *rte, Node *qual, bool targetRelation)
{
Query *parse = root->parse;
Oid relid = rte->relid;
@@ -256,6 +264,16 @@ expand_security_qual(PlannerInfo *root, List *tlist, int rt_index,
}
/*
+ * We need to handle the case where this is the target relation
+ * explicitly since it won't have any row marks, because we still
+ * need to lock the records coming back from the with-security-quals
+ * subquery. This might not appear obivous, but it matches what
+ * we're doing above and keeps FDWs happy too.
+ */
+ if (targetRelation)
+applyLockingClause(subquery, 1, LCS_FORUPDATE,
+ false, false);
+ /*
* Replace any variables in the outer query that refer to the
* original relation RTE with references to columns that we will
* expose in the new subquery, building the subquery's targetlist
diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out
index 507b6a2..a1d03f3 100644
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -1842,24 +1842,26 @@ EXPLAIN (costs off) SELECT * FROM rw_view1 WHERE snoop(person);
(4 rows)
EXPLAIN (costs off) UPDATE rw_view1 SET person=person WHERE snoop(pe
Re: [HACKERS] Configurable location for extension .control files
10.06.2013, 17:51, Dimitri Fontaine kirjoitti:
> Andres Freund writes:
>>> In any case, no packager is going to ship an insecure-by-default
>>> configuration, which is what Dimitri seems to be fantasizing would
>>> happen. It would have to be local option to relax the permissions
>>> on the directory, no matter where it is.
>>
>> *I* don't want that at all. All I'd like to have is a postgresql.conf
>> option specifying additional locations.
>
> Same from me. I think I would even take non-plural location.
Here's a patch to allow overriding extension installation directory.
The patch allows superusers to change it at runtime, but we could also
restrict it to postgresql.conf if needed. I don't really see a point in
restricting it (or not implementing the option at all) as the superuser
can already load custom C code and sql from anywhere in the filesystem;
not having configurable extension directory just makes it harder to test
extensions and to create private clusters of PG data in a custom
location without using custom binaries.
I don't think anyone should be packaging and shipping PG with
extension_directory set, but we really should allow it for superusers
and postgresql.conf so people can test extensions without hacks like
this: https://github.com/ohmu/pgmemcache/blob/master/localtests.sh#L23
/ Oskari
From 35cae53aa5e9cf89b19a3ae276e635b42fbe5313 Mon Sep 17 00:00:00 2001
From: Oskari Saarenmaa
Date: Tue, 17 Feb 2015 23:27:59 +0200
Subject: [PATCH] Allow overriding extension_directory
Add a new GUC, 'extension_directory' to override the default directory for
extensions. This allows users running their own PostgreSQL clusters using
the system binaries to install custom extensions and makes testing
extensions easier.
---
doc/src/sgml/config.sgml | 38 +++
src/backend/commands/extension.c | 21 +--
src/backend/utils/misc/guc.c | 12 +
src/backend/utils/misc/postgresql.conf.sample | 2 ++
src/include/commands/extension.h | 2 ++
5 files changed, 67 insertions(+), 8 deletions(-)
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index c669f75..f0c762a 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -6341,6 +6341,44 @@ dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
+
+ extension_directory (string)
+
+ extension_directory configuration parameter
+
+ extensions
+
+
+
+Look up extensions from this path when an extension is created using
+the CREATE EXTENSION.
+
+
+
+The value for extension_directory must be an
+existing directory containing .control files for
+extensions.
+
+
+
+By default this is the empty string, which uses a directory based
+on the compiled-in PostgreSQL package
+share data directory; this is where the extensions provided by the
+standard PostgreSQL distribution are
+installed.
+
+
+
+This parameter can be changed at run time by superusers, but a
+setting done that way will only persist until the end of the
+client connection, so this method should be reserved for
+development purposes. The recommended way to set this parameter
+is in the postgresql.conf configuration
+file.
+
+
+
+
gin_fuzzy_search_limit (integer)
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 9a0afa4..365ad59 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -56,6 +56,9 @@
#include "utils/tqual.h"
+/* GUC */
+char *Extension_directory;
+
/* Globally visible state variables */
bool creating_extension = false;
Oid CurrentExtensionObject = InvalidOid;
@@ -348,6 +351,9 @@ get_extension_control_directory(void)
char sharepath[MAXPGPATH];
char *result;
+ if (Extension_directory != NULL)
+ return pstrdup(Extension_directory);
+
get_share_path(my_exec_path, sharepath);
result = (char *) palloc(MAXPGPATH);
snprintf(result, MAXPGPATH, "%s/extension", sharepath);
@@ -358,13 +364,11 @@ get_extension_control_directory(void)
static char *
get_extension_control_filename(const char *extname)
{
- char sharepath[MAXPGPATH];
char *result;
- get_share_path(my_exec_path, sharepath);
result = (char *) palloc(MAXPGPATH);
- snprintf(result, MAXPGPATH, "%s/extension/%s.control",
- sharepath, extname);
+ snprintf(result, MAXPGPATH, "%s/%s.control",
+ get_extension_control_directory(), extname);
return result;
}
@@ -372,12 +376,12 @@ get_extension_control_filename(const char *extname)
static char *
get_extension_script_directory(ExtensionControlFile *control)
{
- char sharepath[MAXPGPATH];
char *result;
+ char *extdir;
/*
* The directory parameter c
Re: [HACKERS] Sequence Access Method WIP
On Sun, Feb 15, 2015 at 1:40 PM, Petr Jelinek wrote: > sending new version that is updated along the lines of what we discussed at > FOSDEM, which means: > > - back to single bytea amdata column (no custom columns) Why? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] GiST kNN search queue (Re: KNN-GiST with recheck)
On 02/17/2015 02:56 PM, Alexander Korotkov wrote: Hi! On Mon, Dec 22, 2014 at 1:07 PM, Heikki Linnakangas wrote: Ok, thanks for the review! I have committed this, with some cleanup and more comments added. ISTM that checks in pairingheap_GISTSearchItem_cmp is incorrect. This function should perform inverse comparison. Thus, if item a should be checked first function should return 1. Current behavior doesn't lead to incorrect query answers, but it could be slower than correct version. Good catch. Fixed, thanks. While testing this, I also noticed a bug in the pairing heap code itself. Fixed that too. - Heikki -- 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] Commit fest 2015-12 enters money time
What is required to get the New Patch superpower? I'm also in need of it. On Mon, Feb 16, 2015 at 6:59 PM, Kouhei Kaigai wrote: > > On Tue, Feb 17, 2015 at 8:50 AM, Kouhei Kaigai > wrote: > > >> > I couldn't find operation to add new entry on the current CF. > > >> > > >> Go to the bottom of this CF: > > >> https://commitfest.postgresql.org/4/ > > >> Then click on "New patch". > > >> > > > I couldn't find the "New patch" link on both of IE and Chrome, even > > > though I logged in. Did you do something special? > > > > Err... No. Perhaps I can do it only thanks to my superpowers :) Well, no > > worries I just added it here and it has the same data: > > https://commitfest.postgresql.org/4/167/ > > > That's exactly superpower. Thanks for your help. > -- > NEC OSS Promotion Center / PG-Strom Project > 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] pgaudit - an auditing extension for PostgreSQL
* Jim Nasby (jim.na...@bluetreble.com) wrote: > On 2/17/15 12:50 PM, Stephen Frost wrote: > >* Jim Nasby (jim.na...@bluetreble.com) wrote: > >>We may need to bite the bullet and allow changing the user that the > >>postgres process runs under so it doesn't match who owns the files. > >>Maybe there's a way to allow that other than having the process > >>start as root. > > > >That's an interesting thought but it doesn't seem too likely to work out > >for us. The process still has to be able to read and write the files, > >create new files in the PGDATA directories, etc. > > Right, but if we don't allow things like loading C functions from > wherever you please then it's a lot less likely that a DB SU could > disable auditing. It's not just C functions, there's also a whole slew of untrusted languages, and a superuser can modify the catalog directly. They could change the relfileno for a relation to some other relation that they're really interested in, or use pageinspect, etc. > >>Or maybe there's some other way we could restrict what a DB > >>superuser can do in the shell. > > > >This could be done with SELinux and similar tools, but at the end of the > >day the answer, in my view really, is to have fewer superusers and for > >those superusers to be understood to have OS-level shell access. We > >don't want to deal with all of the security implications of trying to > >provide a "trusted" superuser when that user can create functions in > >untrusted languages, modify the catalog directly, etc, it really just > >doesn't make sense. > > The part I don't like about that is then you still have this highly > trusted account that can also run SQL. The big issue with that is > that no OS-level auditing is going to catch what happens inside the > database itself (well, I guess short of a key logger...) Right- you would need an independent process which acts as an intermediary between the database and the account connecting which simply logs everything. That's really not all *that* hard to do, but it's clearly outside of the scope of PG. > What I'd prefer to see is a way to decouple the OS account from any > DB account. Clearly that doesn't protect us from the OS user doing > something bad, but at least then there's no way for them to just > silently run SQL commands. If the DB account isn't a superuser then everything changes. There's no point fighting with the OS user- they can run some other PG binary which they've copied over locally and run SQL with that, or copy all the files over to another server which they have complete access to. The fact that they can also connect to the DB and run SQL isn't really an issue. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] pgaudit - an auditing extension for PostgreSQL
On 2/17/15 12:50 PM, Stephen Frost wrote: Jim, * Jim Nasby (jim.na...@bluetreble.com) wrote: We may need to bite the bullet and allow changing the user that the postgres process runs under so it doesn't match who owns the files. Maybe there's a way to allow that other than having the process start as root. That's an interesting thought but it doesn't seem too likely to work out for us. The process still has to be able to read and write the files, create new files in the PGDATA directories, etc. Right, but if we don't allow things like loading C functions from wherever you please then it's a lot less likely that a DB SU could disable auditing. Or maybe there's some other way we could restrict what a DB superuser can do in the shell. This could be done with SELinux and similar tools, but at the end of the day the answer, in my view really, is to have fewer superusers and for those superusers to be understood to have OS-level shell access. We don't want to deal with all of the security implications of trying to provide a "trusted" superuser when that user can create functions in untrusted languages, modify the catalog directly, etc, it really just doesn't make sense. The part I don't like about that is then you still have this highly trusted account that can also run SQL. The big issue with that is that no OS-level auditing is going to catch what happens inside the database itself (well, I guess short of a key logger...) What I'd prefer to see is a way to decouple the OS account from any DB account. Clearly that doesn't protect us from the OS user doing something bad, but at least then there's no way for them to just silently run SQL commands. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.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] pgaudit - an auditing extension for PostgreSQL
Jim, * Jim Nasby (jim.na...@bluetreble.com) wrote: > We may need to bite the bullet and allow changing the user that the > postgres process runs under so it doesn't match who owns the files. > Maybe there's a way to allow that other than having the process > start as root. That's an interesting thought but it doesn't seem too likely to work out for us. The process still has to be able to read and write the files, create new files in the PGDATA directories, etc. > Or maybe there's some other way we could restrict what a DB > superuser can do in the shell. This could be done with SELinux and similar tools, but at the end of the day the answer, in my view really, is to have fewer superusers and for those superusers to be understood to have OS-level shell access. We don't want to deal with all of the security implications of trying to provide a "trusted" superuser when that user can create functions in untrusted languages, modify the catalog directly, etc, it really just doesn't make sense. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] pgaudit - an auditing extension for PostgreSQL
On 2/17/15 10:23 AM, Simon Riggs wrote: > I vote to include pgaudit in 9.5, albeit with any changes. In > particular, David may have some changes to recommend, but I haven't > seen a spec or a patch, just a new version of code (which isn't how we > do things...). I submitted the new patch in my name under a separate thread "Auditing extension for PostgreSQL (Take 2)" (54e005cc.1060...@pgmasters.net) because I was not getting any response from the original authors and I wanted to make sure the patch got in for the 2015-02 CF. Of course credit should be given where it is due - to that end I sent email to Ian and Abhijit over the weekend but haven't heard back from them yet. I appreciate that 2ndQuadrant spearheaded this effort and though I made many changes, the resulting code follows the same general design. > I'm happy to do final review and commit. Assuming we are in agreement, > what changes are needed prior to commit? I've incorporated most of Stephen's changes but I won't have time to get another patch out today. However, since most of his comments were about comments, I'd be happy to have your review as is and I appreciate your feedback. -- - David Steele da...@pgmasters.net signature.asc Description: OpenPGP digital signature
Re: [HACKERS] pgaudit - an auditing extension for PostgreSQL
On 2/17/15 12:23 PM, Stephen Frost wrote: * Jim Nasby (jim.na...@bluetreble.com) wrote: On 2/17/15 12:07 PM, Stephen Frost wrote: I agree that it's not the auditing job to stop or control access to data, but it's not so simple to audit the superuser completely. The issue is that even if you have a hard-coded bit in the binary which says "audit everything", a superuser can change the running code to twiddle that bit off, redirect the output of whatever auditing is happening, gain OS-level (eg: shell) access to the system and then make changes to the files under PG directly, etc. Setting a bit in a binary and then not allowing that binary to be unchanged does not actually solve the issue. If we've allowed a superuser *in the database* that kind of power at the OS level then we have a problem. There needs to be *something* that a database SU can't do at the OS level, otherwise we'll never be able to audit database SU activity. This isn't a question. The database superuser has essentially OS-level privileges as the user which PG runs as. I'm all for coming up with a less powerful superuser and the work I've been involved in around adding more role attributes is along the lines to get us there, but I don't think we're ever going to really reduce the power that the PG superuser has, for a variety of reasons. Improving the documentation of what a superuser can do and how granting such access is the same as giving OS shell-level access to the system as the user that PG runs as would certainly be good. It certainly would. I'm honestly not totally clear on what all the holes are. We may need to bite the bullet and allow changing the user that the postgres process runs under so it doesn't match who owns the files. Maybe there's a way to allow that other than having the process start as root. Or maybe there's some other way we could restrict what a DB superuser can do in the shell. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.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] pgaudit - an auditing extension for PostgreSQL
* Jim Nasby (jim.na...@bluetreble.com) wrote: > On 2/17/15 12:07 PM, Stephen Frost wrote: > >I agree that it's not the auditing job to stop or control access to > >data, but it's not so simple to audit the superuser completely. The > >issue is that even if you have a hard-coded bit in the binary which says > >"audit everything", a superuser can change the running code to twiddle > >that bit off, redirect the output of whatever auditing is happening, > >gain OS-level (eg: shell) access to the system and then make changes to > >the files under PG directly, etc. Setting a bit in a binary and then > >not allowing that binary to be unchanged does not actually solve the > >issue. > > If we've allowed a superuser *in the database* that kind of power at > the OS level then we have a problem. There needs to be *something* > that a database SU can't do at the OS level, otherwise we'll never > be able to audit database SU activity. This isn't a question. The database superuser has essentially OS-level privileges as the user which PG runs as. I'm all for coming up with a less powerful superuser and the work I've been involved in around adding more role attributes is along the lines to get us there, but I don't think we're ever going to really reduce the power that the PG superuser has, for a variety of reasons. Improving the documentation of what a superuser can do and how granting such access is the same as giving OS shell-level access to the system as the user that PG runs as would certainly be good. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] pgaudit - an auditing extension for PostgreSQL
On 2/17/15 12:07 PM, Stephen Frost wrote: My views are from working with FDA validated environments, and I don’t really understand the above. It is not db auditing’s job to stop or control the access to data or to log what happens to data outside of PostgreSQL. To audit a db superuser is very simple, keep a log of everything a super user does and to write that log to a write append, no read filesystem or location. Since the db superuser can do anything there is no value in configuring what to log. This should be an option that once set, cannot be changed without reinstalling the PostgreSQL binary. The responsibility for auditing/controlling any binary replacement is the operating system’s, not PostgreSQL. In this environment the db superuser will not have authorized root access for the os. I agree that it's not the auditing job to stop or control access to data, but it's not so simple to audit the superuser completely. The issue is that even if you have a hard-coded bit in the binary which says "audit everything", a superuser can change the running code to twiddle that bit off, redirect the output of whatever auditing is happening, gain OS-level (eg: shell) access to the system and then make changes to the files under PG directly, etc. Setting a bit in a binary and then not allowing that binary to be unchanged does not actually solve the issue. If we've allowed a superuser *in the database* that kind of power at the OS level then we have a problem. There needs to be *something* that a database SU can't do at the OS level, otherwise we'll never be able to audit database SU activity. -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.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] "multiple backends attempting to wait for pincount 1"
Andres Freund writes: > On 2015-02-14 14:10:53 -0500, Tom Lane wrote: >> Andres Freund writes: >>> If you just gdb into the VACUUM process with 6647248e370884 checked out, >>> and do a PGSemaphoreUnlock(&MyProc->sem) you'll hit it as well. I think >>> we should simply move the buf->flags &= ~BM_PIN_COUNT_WAITER (Inside >>> LockBuffer) to LockBufferForCleanup, besides the PinCountWaitBuf = >>> NULL. Afaics, that should do the trick. >> If we're moving the responsibility for clearing that flag from the waker >> to the wakee, > I'm not sure if that's the best plan. Some buffers are pinned at an > incredible rate, sending a signal everytime might actually delay the > pincount waiter from actually getting through the loop. Hm, good point. On the other hand, should we worry about the possibility of a lost signal? Moving the flag-clearing would guard against that, which the current code does not. But we've not seen field reports of such issues AFAIR, so this might not be an important consideration. > Unless we block > further buffer pins by any backend while the flag is set, which I think > would likely not be a good idea, there seem to be little benefit in > moving the responsibility. I concur that we don't want the flag to block other backends from acquiring pins. The whole point here is for VACUUM to lurk in the background until it can proceed with deletion; we don't want it to take priority over foreground queries. > I think just adding something like > ... > /* > * Make sure waiter flag is reset - it might not be if > * ProcWaitForSignal() returned for another reason than UnpinBuffer() > * signalling us. > */ > LockBufHdr(bufHdr); > buf->flags &= ~BM_PIN_COUNT_WAITER; > Assert(bufHdr->wait_backend_pid == MyProcPid); > UnlockBufHdr(bufHdr); > PinCountWaitBuf = NULL; > /* Loop back and try again */ > } > to the bottom of the loop would suffice. No, I disagree. If we maintain the rule that the signaler clears BM_PIN_COUNT_WAITER, then once that happens there is nothing to stop a third party from trying to LockBufferForCleanup on the same buffer (except for table-level locking conventions, which IMO this mechanism shouldn't be dependent on). So this coding would potentially clear the BM_PIN_COUNT_WAITER flag belonging to that third party, and then fail the Assert --- but only in debug builds, not in production, where it would just silently lock up the third-party waiter. So I think having a test to verify that it's still "our" BM_PIN_COUNT_WAITER flag is essential. 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] pgaudit - an auditing extension for PostgreSQL
Neil, * Neil Tiffin (ne...@neiltiffin.com) wrote: > > On Feb 17, 2015, at 3:40 AM, Yeb Havinga wrote: > > Auditing superuser access means auditing beyond the running database. > > The superuser can dump a table, and pipe this data everywhere outside of > > the auditing domain. I cannot begin to imagine the kind of security > > restrictions you'd need to audit what happens with data after libpq has > > produced the results. My best guess would be to incorporate some kind of > > separation of duty mechanism; only allow certain superuser operations > > with two people involved. > > My views are from working with FDA validated environments, and I don’t really > understand the above. It is not db auditing’s job to stop or control the > access to data or to log what happens to data outside of PostgreSQL. To > audit a db superuser is very simple, keep a log of everything a super user > does and to write that log to a write append, no read filesystem or location. > Since the db superuser can do anything there is no value in configuring what > to log. This should be an option that once set, cannot be changed without > reinstalling the PostgreSQL binary. The responsibility for > auditing/controlling any binary replacement is the operating system’s, not > PostgreSQL. In this environment the db superuser will not have authorized > root access for the os. I agree that it's not the auditing job to stop or control access to data, but it's not so simple to audit the superuser completely. The issue is that even if you have a hard-coded bit in the binary which says "audit everything", a superuser can change the running code to twiddle that bit off, redirect the output of whatever auditing is happening, gain OS-level (eg: shell) access to the system and then make changes to the files under PG directly, etc. Setting a bit in a binary and then not allowing that binary to be unchanged does not actually solve the issue. > The use case examples, that I am familiar with, are that procedural policies > control what the db superuser can do. If the policy says that the db > superuser cannot dump a table and pipe this data someplace without being > supervised by a third party auditor (building on the above), then what you > want in the log is that the data was dumped by whom with a date and time. > Thats it. Its up to policies, audit review, management, and third party > audit tools, to pick up the violation. Auditing’s job is to keep a complete > report, not prevent. Prevention is the role of security. Agreed, policy is how to handle superuser-level access and auditing can assist with that but without having an independent process (one which the PG superuser can't control, which means it needs to be a different OS-level user), it's not possible to guarantee that the audit is complete when the superuser is involved. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] pgaudit - an auditing extension for PostgreSQL
* Simon Riggs (si...@2ndquadrant.com) wrote: > David's work is potentially useful, but having two versions of a > feature slows things down. Since he is new to development here, I have > made those comments so he understands, not so you would pick up on > that. I have a bad tendency of replying to email which is replying to comments which I made. :) > "didn't seem to be moving forwards" is strange comment. We usually > wait for patches to stabilise, not for them to keep changing as > evidence of worth. I have to admit that I'm confused by this. Patches don't stabilise through sitting in the archives, they stabilise when the comments are being addressed, the patch updated, and further comments are addressing less important issues. The issues which Robert and I had both commented on didn't appear to be getting addressed. That seems to be due to Abhijit being out, which is certainly fine, but that wasn't clear, at least to me. > David, please submit your work to pgsql-hackers as a patch on > Abhijit's last version. There is no need for a pull request to > 2ndQuadrant. Thanks. Ugh. For my part, at least, having patches on top of patches does *not* make things easier to work with or review. I'm very glad to hear that Abhijit is back and has time to work on this, but as it relates to submitting patches for review to the list or to the commitfest, I'd really like those to be complete patches and not just .c files or patches on top of other patches. To the extent that it helps move this along, I'm not going to object if such patches are posted, but I would object to patches-on-patches being included in the commmitfest. I've not spent much time with the new commitfest app yet, but hopefully it won't be hard to note which patches are the complete ones. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Proposal : REINDEX xxx VERBOSE
On 2/16/15 9:43 PM, Kyotaro HORIGUCHI wrote:
Hello, I had a look on gram.y and found other syntaxes using WITH
option clause.
At Wed, 11 Feb 2015 14:34:17 -0600, Jim Nasby wrote
in<54dbbcc9.1020...@bluetreble.com>
>I suspect at least some of this stems from how command line programs
>tend to process options before arguments. I tend to agree with you
>Tom, but I think what's more important is that we're consistent. COPY
>is already a bit of an oddball because it uses WITH, but both EXPLAIN
>and VACUUM use parenthesis immediately after the first
>verb. Introducing a parenthesis version that goes at the end instead
>of the beginning is just going to make this worse.
>
>If we're going to take a stand on this, we need to do it NOW, before
>we have even more commands that use ().
>
>I know you were worried about accepting options anywhere because it
>leads to reserved words, but perhaps we could support it just for
>EXPLAIN and VACUUM, and then switch to trailing options if people
>think that would be better.
I agree with the direction, but I see two issues here; how many
syntax variants we are allowed to have for one command at a time,
and how far we should/may extend the unified options syntax on
other commands.
Let me put the issues aside for now, VACUUM can have trailing
options naturally but it seems to change, but, IMHO, EXPLAIN
should have the target statement at the tail end. Are you
thinking of syntaxes like following?
VACUUM [FULL] [FREEZE] ... [ANALYZE] [tname [(cname, ...)]
| VACUUM [({FULL [bool]|FREEZE [bool]|...}[,...])] [tname [(cname, ...)]
| VACUUM [tname [(cname, ...)] [[WITH ]({FULL [bool]|FREEZE [bool]|...})]
REINDEX [{INDEX|TABLE|...}] name [[WITH] (VERBOSE [bool]|...)]
EXPLAIN [[WITH] ({ANALYZE [bool]|VERBOSE [bool]|... [,...]})]
For concrete examples, the lines prefixed by asterisk are in new
syntax.
If I could choose only one for explain, I would find it easier to be up
front. That way you do the explain part on one line and just paste the
query after that.
VACUUM FULL table1;
VACUUM ANALYZE table1 (col1);
VACUUM (ANALYZE, VERBOSE) table1 (col1);
*VACUUM table1 WITH (FREEZE on)
*VACUUM table1 (cola) WITH (ANALYZE)
*VACUUM table1 WITH (ANALYZE)
*VACUUM table1 (FREEZE on)
The fifth example looks quite odd.
I don't think we need to allow both () and WITH... I'd say one or the
other, preferably ().
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.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] pgaudit - an auditing extension for PostgreSQL
> On Feb 17, 2015, at 3:40 AM, Yeb Havinga wrote: > > Hi list, > . . . . . > Auditing superuser access means auditing beyond the running database. > The superuser can dump a table, and pipe this data everywhere outside of > the auditing domain. I cannot begin to imagine the kind of security > restrictions you'd need to audit what happens with data after libpq has > produced the results. My best guess would be to incorporate some kind of > separation of duty mechanism; only allow certain superuser operations > with two people involved. My views are from working with FDA validated environments, and I don’t really understand the above. It is not db auditing’s job to stop or control the access to data or to log what happens to data outside of PostgreSQL. To audit a db superuser is very simple, keep a log of everything a super user does and to write that log to a write append, no read filesystem or location. Since the db superuser can do anything there is no value in configuring what to log. This should be an option that once set, cannot be changed without reinstalling the PostgreSQL binary. The responsibility for auditing/controlling any binary replacement is the operating system’s, not PostgreSQL. In this environment the db superuser will not have authorized root access for the os. The use case examples, that I am familiar with, are that procedural policies control what the db superuser can do. If the policy says that the db superuser cannot dump a table and pipe this data someplace without being supervised by a third party auditor (building on the above), then what you want in the log is that the data was dumped by whom with a date and time. Thats it. Its up to policies, audit review, management, and third party audit tools, to pick up the violation. Auditing’s job is to keep a complete report, not prevent. Prevention is the role of security. Neil -- 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] "multiple backends attempting to wait for pincount 1"
On 2015-02-14 14:10:53 -0500, Tom Lane wrote: > Andres Freund writes: > > I don't think it's actually 675333 at fault here. I think it's a > > long standing bug in LockBufferForCleanup() that can just much easier be > > hit with the new interrupt code. > > > Imagine what happens in LockBufferForCleanup() when ProcWaitForSignal() > > returns spuriously - something it's documented to possibly do (and which > > got more likely with the new patches). In the normal case UnpinBuffer() > > will have unset BM_PIN_COUNT_WAITER - but in a spurious return it'll > > still be set and LockBufferForCleanup() will see it still set. > > Yeah, you're right: LockBufferForCleanup has never coped with the > possibility that ProcWaitForSignal returns prematurely. I'm not sure > if that was possible when this code was written, but we've got it > documented as being possible at least back to 8.2. So this needs to > be fixed in all branches. Agreed. > I think it would be smarter to duplicate all the logic > that's currently in UnlockBuffers(), just to make real sure we don't > drop somebody else's waiter flag. ISTM that in LockBufferForCleanup() such a state shouldn't be accepted - it'd be a sign of something going rather bad. I think asserting that it's "our" flag is a good idea, but silently ignoring the fact sounds like a bad plan. As LockBufferForCleanup() really is only safe when holding a SUE lock or heavier (otherwise one wait_backend_pid field obviously would not be sufficient), there should never ever be another waiter. > > If you just gdb into the VACUUM process with 6647248e370884 checked out, > > and do a PGSemaphoreUnlock(&MyProc->sem) you'll hit it as well. I think > > we should simply move the buf->flags &= ~BM_PIN_COUNT_WAITER (Inside > > LockBuffer) to LockBufferForCleanup, besides the PinCountWaitBuf = > > NULL. Afaics, that should do the trick. > > If we're moving the responsibility for clearing that flag from the waker > to the wakee, I'm not sure if that's the best plan. Some buffers are pinned at an incredible rate, sending a signal everytime might actually delay the pincount waiter from actually getting through the loop. Unless we block further buffer pins by any backend while the flag is set, which I think would likely not be a good idea, there seem to be little benefit in moving the responsibility. The least invasive fix would be to to weaken the error check to not trigger if it's not the first iteration through the loop... But that's not particularly pretty. I think just adding something like ... /* * Make sure waiter flag is reset - it might not be if * ProcWaitForSignal() returned for another reason than UnpinBuffer() * signalling us. */ LockBufHdr(bufHdr); buf->flags &= ~BM_PIN_COUNT_WAITER; Assert(bufHdr->wait_backend_pid == MyProcPid); UnlockBufHdr(bufHdr); PinCountWaitBuf = NULL; /* Loop back and try again */ } to the bottom of the loop would suffice. I can't see a extra buffer spinlock cycle matter in comparison to all the the other cost (like ping/pong ing around between processes). Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- 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] pgaudit - an auditing extension for PostgreSQL
At 2015-02-17 10:52:55 -0500, sfr...@snowman.net wrote: > > From the old thread, David had offered to submit a pull request if > there was interest and I didn't see any response... For whatever it's worth, that's because I've been away from work, and only just returned. I had it on my list to look at the code tomorrow. -- Abhijit -- 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] pgaudit - an auditing extension for PostgreSQL
On 17 February 2015 at 15:52, Stephen Frost wrote: > * Simon Riggs (si...@2ndquadrant.com) wrote: >> I vote to include pgaudit in 9.5, albeit with any changes. In >> particular, David may have some changes to recommend, but I haven't >> seen a spec or a patch, just a new version of code (which isn't how we >> do things...). > > Hrm. I thought David's new patch actually looked quite good and it's > certainly quite a bit different from the initial patch (which didn't > seem like it was moving forward..). Guess I'm confused how a new patch > is different from a 'new version of code' and I didn't see a spec for > either patch. From the old thread, David had offered to submit a pull > request if there was interest and I didn't see any response... My comment was that the cycle of development is discuss then develop. David's work is potentially useful, but having two versions of a feature slows things down. Since he is new to development here, I have made those comments so he understands, not so you would pick up on that. "didn't seem to be moving forwards" is strange comment. We usually wait for patches to stabilise, not for them to keep changing as evidence of worth. David, please submit your work to pgsql-hackers as a patch on Abhijit's last version. There is no need for a pull request to 2ndQuadrant. Thanks. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, RemoteDBA, Training & Services -- 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] Parallel Seq Scan
On 2015-02-11 15:49:17 -0500, Robert Haas wrote: > On Tue, Feb 10, 2015 at 3:56 PM, Andres Freund wrote: > >> On Tue, Feb 10, 2015 at 9:08 AM, Andres Freund > >> wrote: > >> > And good chunk sizes et al depend on higher layers, > >> > selectivity estimates and such. And that's planner/executor work, not > >> > the physical layer (which heapam.c pretty much is). > >> > >> If it's true that a good chunk size depends on the higher layers, then > >> that would be a good argument for doing this differently, or at least > >> exposing an API for the higher layers to tell heapam.c what chunk size > >> they want. I hadn't considered that possibility - can you elaborate > >> on why you think we might want to vary the chunk size? > > > > Because things like chunk size depend on the shape of the entire > > plan. If you have a 1TB table and want to sequentially scan it in > > parallel with 10 workers you better use some rather large chunks. That > > way readahead will be efficient in a cpu/socket local manner, > > i.e. directly reading in the pages into the directly connected memory of > > that cpu. Important for performance on a NUMA system, otherwise you'll > > constantly have everything go over the shared bus. But if you instead > > have a plan where the sequential scan goes over a 1GB table, perhaps > > with some relatively expensive filters, you'll really want a small > > chunks size to avoid waiting. > > I see. That makes sense. > > > The chunk size will also really depend on > > what other nodes are doing, at least if they can run in the same worker. > > Example? A query whose runetime is dominated by a sequential scan (+ attached filter) is certainly going to require a bigger prefetch size than one that does other expensive stuff. Imagine parallelizing SELECT * FROM largetable WHERE col = low_cardinality_value; and SELECT * FROM largetable JOIN gigantic_table ON (index_nestloop_condition) WHERE col = high_cardinality_value; The first query will be a simple sequential and disk reads on largetable will be the major cost of executing it. In contrast the second query might very well sensibly be planned as a parallel sequential scan with the nested loop executing in the same worker. But the cost of the sequential scan itself will likely be completely drowned out by the nestloop execution - index probes are expensive/unpredictable. My guess is that the batch size can wil have to be computed based on the fraction of cost of the parallized work it has. > > Even without things like NUMA and readahead I'm pretty sure that you'll > > want a chunk size a good bit above one page. The locks we acquire for > > the buffercache lookup and for reading the page are already quite bad > > for performance/scalability; even if we don't always/often hit the same > > lock. Making 20 processes that scan pages in parallel acquire yet a > > another lock (that's shared between all of them!) for every single page > > won't be fun, especially without or fast filters. > > This is possible, but I'm skeptical. If the amount of other work we > have to do that page is so little that the additional spinlock cycle > per page causes meaningful contention, I doubt we should be > parallelizing in the first place. It's easy to see contention of buffer mapping (many workloads), buffer content and buffer header (especially btree roots and small foreign key target tables) locks. And for most of them we already avoid acquiring the same spinlock in all backends. Right now to process a page in a sequential scan we acquire a nonblocking buffer mapping lock (which doesn't use a spinlock anymore *because* it proved to be a bottleneck), a nonblocking content lock and a the buffer header spinlock. All of those are essentially partitioned - another spinlock shared between all workers will show up. > > As pointed out above (moved there after reading the patch...) I don't > > think a chunk size of 1 or any other constant size can make sense. I > > don't even believe it'll necessarily be constant across an entire query > > execution (big initially, small at the end). Now, we could move > > determining that before the query execution into executor > > initialization, but then we won't yet know how many workers we're going > > to get. We could add a function setting that at runtime, but that'd mix > > up responsibilities quite a bit. > > I still think this belongs in heapam.c somehow or other. If the logic > is all in the executor, then it becomes impossible for any code that > doensn't use the executor to do a parallel heap scan, and that's > probably bad. It's not hard to imagine something like CLUSTER wanting > to reuse that code, and that won't be possible if the logic is up in > some higher layer. Yea. > If the logic we want is to start with a large chunk size and then > switch to a small chunk size when there's not much of the relation > left to scan, there's still no reason that can't be encapsulated in > heapam.c. I don't mind having
Re: [HACKERS] parallel mode and parallel contexts
On 2015-02-11 13:59:04 -0500, Robert Haas wrote:
> On Tue, Feb 10, 2015 at 8:45 PM, Andres Freund wrote:
> > The only reason I'd like it to be active is because that'd *prohibit*
> > doing the crazier stuff. There seems little reason to not da it under
> > the additional protection against crazy things that'd give us?
>
> Trying to load additional libraries once parallel mode is in progress
> can provide failures, because the load of the libraries causes the
> system to do GUC_ACTION_SET on the GUCs whose initialization was
> deferred, and that trips up the
> no-changing-things-while-in-parallel-mode checks.
Oh, that's a good point.
> I'm not sure if there's anything we can, or should, do about that.
Fine with me.
> > Well, ExportSnapshot()/Import has quite a bit more restrictions than
> > what you're doing... Most importantly it cannot import in transactions
> > unless using read committed isolation, forces xid assignment during
> > export, forces the old snapshot to stick around for the whole
> > transaction and only works on a primary. I'm not actually sure it makes
> > a relevant difference, but it's certainly worth calling attention to.
> >
> > The fuding I was wondering about certainly is unnecessary though -
> > GetSnapshotData() has already performed it...
> >
> > I'm not particularly awake right now and I think this needs a closer
> > look either by someone awake... I'm not fully convinced this is safe.
>
> I'm not 100% comfortable with it either, but I just spent some time
> looking at it and can't see what's wrong with it. Basically, we're
> trying to get the parallel worker into a state that matches the
> master's state after doing GetTransactionSnapshot() - namely,
> CurrentSnapshot should point to the same snapshot on the master, and
> FirstSnapshotSet should be true, plus the same additional processing
> that GetTransactionSnapshot() would have done if we're in a higher
> transaction isolation level. It's possible we don't need to mimic
> that state, but it seems like a good idea.
I plan to look at this soonish.
> Still, I wonder if we ought to be banning GetTransactionSnapshot()
> altogether. I'm not sure if there's ever a time when it's safe for a
> worker to call that.
Why?
> >> > Also, you don't seem to
> >> > prohibit popping the active snapshot (should that be prohibitted
> >> > maybe?) which might bump the initiator's xmin horizon.
> >>
> >> I think as long as our transaction snapshot is installed correctly our
> >> xmin horizon can't advance; am I missing something?
> >
> > Maybe I'm missing something, but why would that be the case in a read
> > committed session? ImportSnapshot() only ever calls
> > SetTransactionSnapshot it such a case (why does it contain code to cope
> > without it?), but your patch doesn't seem to guarantee that.
> >
> > But as don't actually transport XactIsoLevel anywhere (omission
> > probably?) that seems to mean that the SetTransactionSnapshot() won't do
> > much, even if the source transaction is repeatable read.
>
> Doesn't XactIsoLevel get set by assign_XactIsoLevel() when we restore
> the GUC state?
Yes, but afaics the transaction start will just overwrite it again:
static void
StartTransaction(void)
{
...
XactDeferrable = DefaultXactDeferrable;
XactIsoLevel = DefaultXactIsoLevel;
...
For a client issued BEGIN it works because utility.c does:
case TRANS_STMT_BEGIN:
case TRANS_STMT_START:
{
ListCell *lc;
BeginTransactionBlock();
foreach(lc, stmt->options)
{
DefElem*item = (DefElem *) lfirst(lc);
if (strcmp(item->defname,
"transaction_isolation") == 0)
SetPGVariable("transaction_isolation",
list_make1(item->arg),
true);
else if (strcmp(item->defname,
"transaction_read_only") == 0)
SetPGVariable("transaction_read_only",
list_make1(item->arg),
true);
else if (strcmp(item->defname,
"transaction_deferrable") == 0)
SetPGVariable("transaction_deferrable",
list_make1(item->arg),
true);
}
Pretty, isn't it?
> > Your manual ones don't either, that's what made me
> > complain. E.g. pg_advisory_lock_int8 isn't called that on the SQL
> > level. Instead they use the C name + parens.
>
> On further reflection, I think I should probab
Re: [HACKERS] pgaudit - an auditing extension for PostgreSQL
* Simon Riggs (si...@2ndquadrant.com) wrote: > I vote to include pgaudit in 9.5, albeit with any changes. In > particular, David may have some changes to recommend, but I haven't > seen a spec or a patch, just a new version of code (which isn't how we > do things...). Hrm. I thought David's new patch actually looked quite good and it's certainly quite a bit different from the initial patch (which didn't seem like it was moving forward..). Guess I'm confused how a new patch is different from a 'new version of code' and I didn't see a spec for either patch. From the old thread, David had offered to submit a pull request if there was interest and I didn't see any response... > I'm happy to do final review and commit. Assuming we are in agreement, > what changes are needed prior to commit? I'm all about getting something done here for 9.5 also and would certainly prefer to focus on that. The recent discussion has all moved towards the approach that I was advocating where we use GRANT simimlar to how AUDIT exists in other RDBMS's. Both the latest version of the code from Abhijit and David's code do that and I found what David did quite easy to follow- no big #ifdef blocks (something I complained about earlier but didn't see any progress on..) and no big switch statements that would likely get out-dated very quickly. I'm not against going back to the code submitted by Abhijit, if it's cleaned up and has the #ifdef blocks and whatnot removed that were discussed previously. I don't fault David for moving forward though, given the lack of feedback. Perhaps there's an issue where the classes provided by David's approach aren't granular enough but it's certainly better than what we have today. The event-trigger based approach depends on as-yet-uncommitted code, as I understand it. I'd certainly rather have fewer audit classes which cover everything than more audit classes which end up not covering everything because we don't have all the deparse code or event triggers we need completed and committed yet. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Add min and max execute statement time in pg_stat_statement
On 17/02/15 16:12, Andres Freund wrote: On 2015-02-17 15:50:39 +0100, Petr Jelinek wrote: On 17/02/15 03:07, Petr Jelinek wrote: On 17/02/15 03:03, Andrew Dunstan wrote: On 02/16/2015 08:57 PM, Andrew Dunstan wrote: Average of 3 runs of read-only pgbench on my system all with pg_stat_statement activated: HEAD: 20631 SQRT: 20533 SQRTD: 20592 So using sqrtd the cost is 0.18%. I think that's acceptable. Actually, sqrt/sqrtd is not called in accumulating the stats, only in the reporting function. So it looks like the difference here should be noise. Maybe we need some longer runs. Yes there are variations between individual runs so it might be really just that, I can leave it running for much longer time tomorrow. Ok so I let it run for more than hour on a different system, the difference is negligible - 14461 vs 14448 TPS. I think there is bigger difference between individual runs than between the two versions... These numbers sound like you measured them without concurrency, am I right? If so, the benchmark isn't that interesting - the computation happens while a spinlock is held, and that'll mainly matter if there are many backends running at the same time. It's pgbench -j16 -c32 -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- 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] Add pg_settings.pending_restart column
On Sat, Feb 14, 2015 at 10:18 PM, Peter Eisentraut wrote:
> When managing configuration changes through automatic systems like Chef
> or Puppet, there is a problem: How do you manage changes requiring a
> restart?
>
> Generally, you'd set it up so that when a configuration file is changed,
> the server is reloaded. But for settings that require a restart, well,
> I don't know. From discussions with others, it emerged that a way to
> ask the server whether a restart is necessary would be useful. Then you
> can either automate the restart, or have a monitoring system warn you
> about it, and possibly schedule a restart separately or undo the
> configuration file change.
>
> So here is a patch for that. It adds a column pending_restart to
> pg_settings that is true when the configuration file contains a changed
> setting that requires a restart. We already had the logic to detect
> such changes, for producing the log entry. I have also set it up so
> that if you change your mind and undo the setting and reload the server,
> the pending_restart flag is reset to false.
You don't really need the "else" here, and in parallel cases:
if (*conf->variable != newval)
{
+record->status |= GUC_PENDING_RESTART;
ereport(elevel,
(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
errmsg("parameter \"%s\" cannot be
changed without restarting the server",
name)));
return 0;
}
+else
+record->status &= ~GUC_PENDING_RESTART;
return -1;
The if-statement ends with "return 0" so there is no reason for the "else".
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
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] Auditing extension for PostgreSQL (Take 2)
David, I've CC'd Abhijit, the original author of pgaudit, as it seems likely he'd also be interested in this. * David Steele (da...@pgmasters.net) wrote: > I've posted a couple of messages over the last few weeks about the work > I've been doing on the pg_audit extension. The lack of response could > be due to either universal acclaim or complete apathy, but in any case I > think this is a very important topic so I want to give it another try. Thanks! It's certainly an important topic to a lot of folks; I imagine the lack of response is simply because people are busy. > I've extensively reworked the code that was originally submitted by > 2ndQuandrant. This is not an indictment of their work, but rather an > attempt to redress concerns that were expressed by members of the > community. I've used core functions to determine how audit events > should be classified and simplified and tightened the code wherever > possible. I've removed deparse and event triggers and opted for methods > that rely only on existing hooks. In my last message I provided > numerous examples of configuration, usage, and output which I hoped > would alleviate concerns of complexity. I've also written a ton of unit > tests to make sure that the code works as expected. The configuration approach you posted is definitely in-line with what I was trying to get at previously- thanks for putting some actual code behind it! While not a big fan of that other big RDBMS, I do like that this approach ends up being so similar in syntax. > I realize this is not an ideal solution. Everybody (including me) wants > something that is in core with real policies and more options. It's > something that I am personally really eager to work on. But the reality > is that's not going to happen for 9.5 and probably not for 9.6 either. > Meanwhile, I believe the lack of some form of auditing is harming > adoption of PostgreSQL, especially in the financial and government sectors. Agreed. > The patch I've attached satisfies the requirements that I've had from > customers in the past. I'm confident that if it gets out into the wild > it will bring all kinds of criticism and comments which will be valuable > in designing a robust in-core solution. This is definitely something that makes sense to me, particularly for such an important piece. I had argued previously that a contrib based solution would make it difficult to build an in-core solution, but others convinced me that it'd not only be possible but would probably be preferrable as we'd gain experience with the contrib module and, as you say, we'd be able to build a better in-core solution based on that experience. > I'm submitting this patch to the Commitfest. I'll do everything I can > to address the concerns of the community and I'm happy to provide more > examples as needed. I'm hoping the sgml docs I've provided with the > patch will cover any questions, but of course feedback is always > appreciated. Glad you submitted it to the CommitFest. Just glancing through the code, it certainly looks a lot closer to being something which we could move forward with. Using existing functions to work out the categories (instead of massive switch statements) is certainly much cleaner and removing those large #ifdef blocks has made the code a lot easier to follow. Lastly, I really like all the unit tests.. Additional comments in-line follow. > diff --git a/contrib/pg_audit/pg_audit.c b/contrib/pg_audit/pg_audit.c > new file mode 100644 > index 000..b3914ac > --- /dev/null > +++ b/contrib/pg_audit/pg_audit.c > @@ -0,0 +1,1099 @@ > +/*-- > + * pg_audit.c > + * > + * An auditing extension for PostgreSQL. Improves on standard statement > logging > + * by adding more logging classes, object level logging, and providing > + * fully-qualified object names for all DML and many DDL statements. It'd be good to quantify what 'many' means above. > + * Copyright (c) 2014-2015, PostgreSQL Global Development Group > + * > + * IDENTIFICATION > + * contrib/pg_prewarm/pg_prewarm.c Pretty sure this isn't pg_prewarm.c :) > +/* > + * String contants for audit types - used when logging to distinguish session > + * vs. object auditing. > + */ "String constants" > +/* > + * String contants for log classes - used when processing tokens in the > + * pgaudit.log GUC. > + */ Ditto. > +/* String contants for logging commands */ Ditto. :) > +/* > + * This module collects AuditEvents from various sources (event triggers, and > + * executor/utility hooks) and passes them to the log_audit_event() function. This isn't using event triggers any more, right? Doesn't look like it. I don't think that's a problem and it certainly seems to have simplified things quite a bit, but the comment should be updated. > +/* > + * Returns the oid of the role specified in pgaudit.role. > + */ > +static Oid > +audit_role_oid() Couldn'
Re: [HACKERS] pg_check_dir comments and implementation mismatch
On Thu, Feb 12, 2015 at 9:31 AM, Marco Nenciarini wrote: > Il 02/02/15 21:48, Robert Haas ha scritto: >> On Sat, Jan 31, 2015 at 8:28 AM, Marco Nenciarini >> wrote: >>> Il 30/01/15 03:54, Michael Paquier ha scritto: On Fri, Jan 30, 2015 at 2:49 AM, Tom Lane wrote: > There is at least one other bug in that function now that I look at it: > in event of a readdir() failure, it neglects to execute closedir(). > Perhaps not too significant since all existing callers will just exit() > anyway after a failure, but still ... I would imagine that code scanners like coverity or similar would not be happy about that. ISTM that it is better to closedir() appropriately in all the code paths. >>> >>> I've attached a new version of the patch fixing the missing closedir on >>> readdir error. >> >> If readir() fails and closedir() succeeds, the return will be -1 but >> errno will be 0. >> > > The attached patch should fix it. Looks nice. Committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] GSoC 2015 - mentors, students and admins.
I'd like to remind "Be Early" advice for GSoC students. http://www.postgresql.org/developer/summerofcodeadvice/ Students which starts discussion of their project early have much more chances to show favourable sides of their proposals. As result they have more chances to get proposals accepted. -- With best regards, Alexander Korotkov.
Re: [HACKERS] pgaudit - an auditing extension for PostgreSQL
On 17 February 2015 at 14:44, Stephen Frost wrote: >> The patch as it is, is targeted at auditing user/application level >> access to the database, and as such it matches the use case of auditing >> user actions. > > Right, and that's a *very* worthwhile use-case. Agreed. So, we are still at the same place we were at 7-8 months ago: Some people would like an AUDIT command, but this isn't it. We have neither a design nor a developer willing to implement it (or funding to do so). That may change in the future, but if it does, we will not have auditing in production version of Postgres before September 2016, earliest if we wait for that. I vote to include pgaudit in 9.5, albeit with any changes. In particular, David may have some changes to recommend, but I haven't seen a spec or a patch, just a new version of code (which isn't how we do things...). In my understanding, the following people are in favour of pgaudit, in some form: Simon, Yeb, David, Stephen and other developers have spoken earlier in favour of including it. Abhijit, Jim and Robert have voiced recent doubts of various kinds, but there seems to be no outstanding objection to including pgaudit, only a wish that we had something better. (Please correct me). I'm happy to do final review and commit. Assuming we are in agreement, what changes are needed prior to commit? -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, RemoteDBA, Training & Services -- 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] Strange assertion using VACOPT_FREEZE in vacuum.c
On Thu, Feb 12, 2015 at 11:54 PM, Michael Paquier wrote: > Hi all, > > When calling vacuum(), there is the following assertion using VACOPT_FREEZE: > Assert((vacstmt->options & VACOPT_VACUUM) || > !(vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE))); > I think that this should be changed with sanity checks based on the > parameter values of freeze_* in VacuumStmt as we do not set up > VACOPT_FREEZE when VACUUM is used without options in parenthesis, for > something like that: > Assert((vacstmt->options & VACOPT_VACUUM) || > - !(vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE))); > + ((vacstmt->options & VACOPT_FULL) == 0 && > + vacstmt->freeze_min_age < 0 && > + vacstmt->freeze_table_age < 0 && > + vacstmt->multixact_freeze_min_age < 0 && > + vacstmt->multixact_freeze_table_age < 0)); > This would also have the advantage to limit the use of VACOPT_FREEZE > in the query parser. > A patch is attached. > Thoughts? I think it's right the way it is. The parser constructs a VacuumStmt for either a VACUUM or an ANALYZE command. That statement is checking that if you are doing an ANALYZE, you can't specify FULL or FREEZE. That makes sense, because there is no ANALYZE FULL or ANALYZE FREEZE command. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] Add min and max execute statement time in pg_stat_statement
On 2015-02-17 15:50:39 +0100, Petr Jelinek wrote: > On 17/02/15 03:07, Petr Jelinek wrote: > >On 17/02/15 03:03, Andrew Dunstan wrote: > >>On 02/16/2015 08:57 PM, Andrew Dunstan wrote: > Average of 3 runs of read-only pgbench on my system all with > pg_stat_statement activated: > HEAD: 20631 > SQRT: 20533 > SQRTD: 20592 > >>>So using sqrtd the cost is 0.18%. I think that's acceptable. > >>Actually, sqrt/sqrtd is not called in accumulating the stats, only in > >>the reporting function. So it looks like the difference here should be > >>noise. Maybe we need some longer runs. > >Yes there are variations between individual runs so it might be really > >just that, I can leave it running for much longer time tomorrow. > Ok so I let it run for more than hour on a different system, the difference > is negligible - 14461 vs 14448 TPS. I think there is bigger difference > between individual runs than between the two versions... These numbers sound like you measured them without concurrency, am I right? If so, the benchmark isn't that interesting - the computation happens while a spinlock is held, and that'll mainly matter if there are many backends running at the same time. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- 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_basebackup -x/X doesn't play well with archive_mode & wal_keep_segments
On 2015-02-17 12:18:41 +0900, Fujii Masao wrote: > On Fri, Feb 13, 2015 at 9:18 AM, Andres Freund wrote: > > On 2015-02-12 11:44:05 -0800, Sergey Konoplev wrote: > >> On Thu, Feb 12, 2015 at 11:40 AM, Andres Freund > >> wrote: > >> > This obviously should not be the case. I'll have a look in a couple of > >> > hours. Until then you can likely just work around the problem by > >> > creating the archive_status directory. > >> > >> Thank you. Just let me know if you need some extra info or debugging. > > > > No need for debugging. It's plain and simply a (cherry-pick) conflict I > > resolved wrongly during backpatching. 9.3, 9.4 and master do not have > > that problem. That whole fix was quite painful because every single > > release had significantly different code :(. pg_basebackup/ is pretty > > messy. > > I'm not sure why my testsuite didn't trigger that problem. Possibly > > because a retry makes things work :( > > > > Somewhat uckily it's 9.2 only (9.3, 9.4 and master look correct, earlier > > releases don't have pg_receivexlog) > > Are you planning to back-patch the fix to 9.2? Now done. Thanks Sergey, Fujii. And sorry for the 9.2 screwup. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- 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] Add min and max execute statement time in pg_stat_statement
On 17/02/15 03:07, Petr Jelinek wrote: On 17/02/15 03:03, Andrew Dunstan wrote: On 02/16/2015 08:57 PM, Andrew Dunstan wrote: On 02/16/2015 08:48 PM, Petr Jelinek wrote: On 17/02/15 01:57, Peter Geoghegan wrote: On Mon, Feb 16, 2015 at 4:44 PM, Petr Jelinek wrote: We definitely want this feature, I wished to have this info many times. I would still like to see a benchmark. Average of 3 runs of read-only pgbench on my system all with pg_stat_statement activated: HEAD: 20631 SQRT: 20533 SQRTD: 20592 So using sqrtd the cost is 0.18%. I think that's acceptable. Actually, sqrt/sqrtd is not called in accumulating the stats, only in the reporting function. So it looks like the difference here should be noise. Maybe we need some longer runs. Yes there are variations between individual runs so it might be really just that, I can leave it running for much longer time tomorrow. Ok so I let it run for more than hour on a different system, the difference is negligible - 14461 vs 14448 TPS. I think there is bigger difference between individual runs than between the two versions... -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- 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] pgaudit - an auditing extension for PostgreSQL
Yeb, * Yeb Havinga (yebhavi...@gmail.com) wrote: > On 20/01/15 23:03, Jim Nasby wrote:> On 1/20/15 2:20 PM, Robert Haas wrote: > > +1. In particular I'm very concerned with the idea of doing this via > > roles, because that would make it trivial for any superuser to disable > > auditing. > > Rejecting the audit administration through the GRANT system, on the > grounds that it easy for the superuser to disable it, seems unreasonable > to me, since superusers are different from non-superusers in a > fundamental way. Agreed. > The patch as it is, is targeted at auditing user/application level > access to the database, and as such it matches the use case of auditing > user actions. Right, and that's a *very* worthwhile use-case. > Auditing superuser access means auditing beyond the running database. Exactly! :) Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] __attribute__ for non-gcc compilers
17.02.2015, 15:46, Andres Freund kirjoitti: > On 2015-02-17 15:41:45 +0200, Oskari Saarenmaa wrote: >> 15.01.2015, 21:58, Robert Haas kirjoitti: >>> On Wed, Jan 14, 2015 at 5:54 PM, Andres Freund >>> wrote: I think I'd for now simply not define pg_attribute_aligned() on platforms where it's not supported, instead of defining it empty. If we need a softer variant we can name it pg_attribute_aligned_if_possible or something. Sounds sane? >>> >>> Yes, that sounds like a much better plan. >> >> Attached an updated patch rebased on today's git master that never >> defines aligned or packed empty. > > Cool, that looks good. Besides allowing other compilers to use the > __attribute__ stuff, it also seems like a readability win to > me. Especially the various printf stuff looks much better to me this > way. > > I guess you've tested this on solaris and x86 linux? Yeah, tested on x86-64 Linux using gcc version 4.9.2 20150212 (Red Hat 4.9.2-6) and on Solaris 10 using Sun C 5.12 SunOS_sparc. / Oskari -- 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] pgaudit - an auditing extension for PostgreSQL
On 2/17/15 4:40 AM, Yeb Havinga wrote: > On 20/01/15 23:03, Jim Nasby wrote:> On 1/20/15 2:20 PM, Robert Haas wrote: >>> On Tue, Jan 20, 2015 at 1:05 AM, Abhijit >>> Menon-Sen wrote: > So when I'm trying to decide what to audit, I have to: > > (a) check if the current user is mentioned in .roles; if so, audit. > > (b) check if the current user is a descendant of one of the roles > mentioned in .roles; if not, no audit. > > (c) check for permissions granted to the "root" role and see if that > tells us to audit. > > Is that right? If so, it would work fine. I don't look forward to trying > to explain it to people, but yes, it would work (for anything you could > grant permissions for). >>> I think this points to fundamental weakness in the idea of doing this >>> through the GRANT system. Some people are going want to audit >>> everything a particular user does, some people are going to want to >>> audit all access to particular objects, and some people will have more >>> complicated requirements. Some people will want to audit even >>> super-users, others especially super-users, others only non >>> super-users. None of this necessarily matches up to the usual >>> permissions framework. >> >> +1. In particular I'm very concerned with the idea of doing this via >> roles, because that would make it trivial for any superuser to disable >> auditing. > > Rejecting the audit administration through the GRANT system, on the > grounds that it easy for the superuser to disable it, seems unreasonable > to me, since superusers are different from non-superusers in a > fundamental way. Agreed. This logic could be applied to any database object: why have tables when a superuser can so easily drop them and cause data loss? > The patch as it is, is targeted at auditing user/application level > access to the database, and as such it matches the use case of auditing > user actions. Exactly. This patch (and my rework) are focused entirely on auditing the actions of normal users in the database. While auditing can be enabled for superusers, there's no guarantee that it's reliable since it would be easy for a superuser to disable. Normal users can be configured to not have that capability, so auditing them is reliable. -- - David Steele da...@pgmasters.net signature.asc Description: OpenPGP digital signature
Re: [HACKERS] __attribute__ for non-gcc compilers
On 2015-02-17 15:41:45 +0200, Oskari Saarenmaa wrote: > 15.01.2015, 21:58, Robert Haas kirjoitti: > > On Wed, Jan 14, 2015 at 5:54 PM, Andres Freund > > wrote: > >> I think I'd for now simply not define pg_attribute_aligned() on > >> platforms where it's not supported, instead of defining it empty. If we > >> need a softer variant we can name it pg_attribute_aligned_if_possible or > >> something. > >> > >> Sounds sane? > > > > Yes, that sounds like a much better plan. > > Attached an updated patch rebased on today's git master that never > defines aligned or packed empty. Cool, that looks good. Besides allowing other compilers to use the __attribute__ stuff, it also seems like a readability win to me. Especially the various printf stuff looks much better to me this way. I guess you've tested this on solaris and x86 linux? Greetings, Andres Freund -- 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] Bug in pg_dump
On Tue, Jan 20, 2015 at 8:06 PM, Gilles Darold wrote:
> Le 19/01/2015 14:41, Robert Haas a écrit :
>> On Thu, Jan 15, 2015 at 6:26 AM, Gilles Darold
>> wrote:
>>> I attach a patch that solves the issue in pg_dump, let me know if it might
>>> be included in Commit Fest or if the three other solutions are a better
>>> choice.
>> I think a fix in pg_dump is appropriate, so I'd encourage you to add
>> this to the next CommitFest.
>>
> Ok, thanks Robert. The patch have been added to next CommitFest under
> the Bug Fixes section.
>
> I've also made some review of the patch and more test. I've rewritten
> some comments and added a test when TableInfo is NULL to avoid potential
> pg_dump crash.
>
> New patch attached: pg_dump.c-extension-FK-v2.patch
So, I looked at your patch and I have some comments...
The approach taken by the patch looks correct to me as we cannot
create FK constraints after loading the data in the case of an
extension, something that can be done with a data-only dump.
Now, I think that the routine hasExtensionMember may impact
performance unnecessarily in the case of databases with many tables,
say thousands or more. And we can easily avoid this performance
problem by checking the content of each object dumped by doing the
legwork directly in getTableData. Also, most of the NULL pointer
checks are not needed as most of those code paths would crash if
tbinfo is NULL, and actually only keeping the one in dumpConstraint is
fine.
On top of those things, I have written a small extension able to
reproduce the problem that I have included as a test module in
src/test/modules. The dump/restore check is done using the TAP tests,
and I have hacked prove_check to install as well the contents of the
current folder to be able to use the TAP routines with an extension
easily. IMO, as we have no coverage of pg_dump with extensions I think
that it would be useful to ensure that this does not break again in
the future.
All the hacking I have done during the review results in the set of
patch attached:
- 0001 is your patch, Gilles, with some comment fixes as well as the
performance issue with hasExtensionMember fix
- 0002 is the modification of prove_check that makes TAP tests usable
with extensions
- 0003 is the test module covering the tests needed for pg_dump, at
least for the problem of this thread.
Gilles, how does that look to you?
(Btw, be sure to generate your patches directly with git next time. :) )
Regards,
--
Michael
From 8005cffd08c57b77564bb0294d8ebd4600bc1dcf Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Tue, 17 Feb 2015 07:39:23 +
Subject: [PATCH 1/3] Fix ordering of tables part of extensions linked with
constraints
The same mechanism as data-only dumps ensuring that data is loaded
respecting foreign key constains is used if it at least one dumped
object is found as being part of an extension. This commit reinforces
as well a couple of code paths to not dump objects that are directly
part of an extension.
Patch by Gilles Darold.
---
src/bin/pg_dump/pg_dump.c | 99 +++
1 file changed, 83 insertions(+), 16 deletions(-)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 7e92b74..c95ae3d 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -208,7 +208,8 @@ static void addBoundaryDependencies(DumpableObject **dobjs, int numObjs,
DumpableObject *boundaryObjs);
static void getDomainConstraints(Archive *fout, TypeInfo *tyinfo);
-static void getTableData(DumpOptions *dopt, TableInfo *tblinfo, int numTables, bool oids);
+static void getTableData(DumpOptions *dopt, TableInfo *tblinfo, int numTables,
+ bool oids, bool *has_ext_member);
static void makeTableDataInfo(DumpOptions *dopt, TableInfo *tbinfo, bool oids);
static void buildMatViewRefreshDependencies(Archive *fout);
static void getTableDataFKConstraints(void);
@@ -730,9 +731,12 @@ main(int argc, char **argv)
if (!dopt.schemaOnly)
{
- getTableData(&dopt, tblinfo, numTables, dopt.oids);
+ bool has_ext_member = false;
+
+ getTableData(&dopt, tblinfo, numTables, dopt.oids, &has_ext_member);
buildMatViewRefreshDependencies(fout);
- if (dopt.dataOnly)
+
+ if (dopt.dataOnly || has_ext_member)
getTableDataFKConstraints();
}
@@ -1866,7 +1870,8 @@ refreshMatViewData(Archive *fout, TableDataInfo *tdinfo)
* set up dumpable objects representing the contents of tables
*/
static void
-getTableData(DumpOptions *dopt, TableInfo *tblinfo, int numTables, bool oids)
+getTableData(DumpOptions *dopt, TableInfo *tblinfo, int numTables,
+ bool oids, bool *has_ext_member)
{
int i;
@@ -1874,6 +1879,8 @@ getTableData(DumpOptions *dopt, TableInfo *tblinfo, int numTables, bool oids)
{
if (tblinfo[i].dobj.dump)
makeTableDataInfo(dopt, &(tblinfo[i]), oids);
+ if (!(*has_ext_member) && tblinfo[i].dobj.ext_member)
+ *has_ext_member = true;
}
}
@@ -2052,13 +2059,15 @@ buildMatViewRefreshDependenci
Re: [HACKERS] postgres_fdw foreign keys with default sequence
Slight typo on my local host example there. s/clone/local/
More like the below:
CREATE FOREIGN TABLE IF NOT EXISTS live.devices (
device_id bigint NOT NULL
);
CREATE MATERIALISED VIEW local.devices;
CREATE test_table (device_id bigint FOREIGN KEY (device_id) REFERENCES
*local*.devices (device_id) );
ERROR: referenced relation "devices" is not a table
On Tue, Feb 17, 2015 at 1:08 PM, Tim Kane wrote:
> Hi all,
>
> Not sure if this has been reported already, it seems to be a variation on
> this thread:
>
> http://www.postgresql.org/message-id/20130515151059.go4...@tamriel.snowman.net
>
>
> One minor difference is, in my scenario - my source table field is defined
> as BIGINT (not serial) - though it does have a default nextval on a
> sequence, so ultimately - the same dependence.
>
> The primary difference (IMHO), is that I am actually foreign keying on a
> local materialised view of the fdw'ed foreign table.
>
>
>
> On the foreign host:
> Table "live.devices"
>Column | Type | Modifiers
>
> ++---
> device_id | bigint | not null default
> nextval('devices_id_sequence'::regclass)
>
>
> On the local host:
>
>
> CREATE FOREIGN TABLE IF NOT EXISTS live.devices (
> device_id bigint NOT NULL
> );
>
> CREATE MATERIALISED VIEW local.devices;
>
> CREATE test_table (device_id bigint FOREIGN KEY (device_id) REFERENCES
> clone.devices (device_id) );
>
> ERROR: referenced relation "devices" is not a table
>
>
>
> Though this is a similar scenario to the previous thread, I would have
> expected foreign keying from a materialised view to behave independently of
> the FDW, as if from a regular local table.
>
> FYI, I'm running postgresql 9.3.4
>
> Cheers,
>
> Tim
>
>
>
Re: [HACKERS] KNN-GiST with recheck
On Sun, Feb 15, 2015 at 2:08 PM, Alexander Korotkov
wrote:
> Following changes has been made in attached patch:
>
> * Get sort operators from pathkeys.
> * Recheck argument of distance function has been reverted.
>
Few comments were added and pairing heap comparison function was fixed in
attached version of patch (knn-gist-recheck-6.patch).
Also I expected that reordering in executor would be slower than reordering
in GiST because of maintaining two heaps instead of one. I've revised
version of patch with reordering in GiST to use pairing heap. I compare two
types of reordering on 10^7 random points and polygons. Results are below.
Test shows that overhead of reordering in executor is insignificant (less
than statistical error).
Reorder in GiST Reorder in executor
points
limit=10 0.10615 0.0880125
limit=1000.236668750.2292375
limit=1000 1.514868751.5208375
polygons
limit=10 0.116506250.1347
limit=1000.462793750.45294375
limit=1000 3.5170125 3.54868125
Revised patch with reordering in GiST is attached
(knn-gist-recheck-in-gist.patch) as well as testing script (test.py).
--
With best regards,
Alexander Korotkov.
knn-gist-recheck-6.patch
Description: Binary data
knn-gist-recheck-in-gist.patch
Description: Binary data
#!/usr/bin/env python
import psycopg2
import json
import sys
# Create test tables with following statements.
#
# create table p as (select point(random(), random()) from generate_series(1,1000));
# create index p_idx on p using gist(v);
# create table g as (select polygon(3 + (random()*5)::int, circle(point(random(), random()), 0.001)) v from generate_series(1,1000));
# create index g_idx on g using gist(v);
dbconn = psycopg2.connect("dbname='postgres' user='smagen' host='/tmp' password='' port=5431")
points = []
pointsCount = 16
tableName = None
def generatePoints(n):
global points
m = 1
d = 0.5
points.append((0, 0))
while m <= n:
for i in range(0, m):
points.append((points[i][0] + d, points[i][1]))
points.append((points[i][0], points[i][1] + d))
points.append((points[i][0] + d, points[i][1] + d))
d /= 2.0
m *= 4
generatePoints(pointsCount)
def runKnn(point, limit):
cursor = dbconn.cursor()
cursor.execute("EXPLAIN (ANALYZE, FORMAT JSON) SELECT * FROM " + tableName + " ORDER BY v <-> %s::point LIMIT %s;", (point, limit))
plan = cursor.fetchone()[0][0]
cursor.close()
return (plan['Planning Time'], plan['Execution Time'])
def makeTests(n, limit):
planningTime = 0
executionTime = 0
for i in range(0, n):
for j in range(0, pointsCount):
point = '(' + str(points[j][0]) + ',' + str(points[j][1]) + ')'
result = runKnn(point, limit)
planningTime += result[0]
executionTime += result[1]
planningTime /= n * pointsCount
executionTime /= n * pointsCount
return (planningTime, executionTime)
if (len(sys.argv) < 2):
print "Usage: %s table_name" % sys.argv[0]
sys.exit(2)
tableName = sys.argv[1]
for limit in [10, 100, 1000]:
result = makeTests(10, limit)
print "limit: %s\nplanning: %s\nexecution: %s" % (limit, result[0], result[1])
dbconn.close()
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] postgres_fdw foreign keys with default sequence
Hi all,
Not sure if this has been reported already, it seems to be a variation on
this thread:
http://www.postgresql.org/message-id/20130515151059.go4...@tamriel.snowman.net
One minor difference is, in my scenario - my source table field is defined
as BIGINT (not serial) - though it does have a default nextval on a
sequence, so ultimately - the same dependence.
The primary difference (IMHO), is that I am actually foreign keying on a
local materialised view of the fdw'ed foreign table.
On the foreign host:
Table "live.devices"
Column | Type | Modifiers
++---
device_id | bigint | not null default
nextval('devices_id_sequence'::regclass)
On the local host:
CREATE FOREIGN TABLE IF NOT EXISTS live.devices (
device_id bigint NOT NULL
);
CREATE MATERIALISED VIEW local.devices;
CREATE test_table (device_id bigint FOREIGN KEY (device_id) REFERENCES
clone.devices (device_id) );
ERROR: referenced relation "devices" is not a table
Though this is a similar scenario to the previous thread, I would have
expected foreign keying from a materialised view to behave independently of
the FDW, as if from a regular local table.
FYI, I'm running postgresql 9.3.4
Cheers,
Tim
Re: [HACKERS] GiST kNN search queue (Re: KNN-GiST with recheck)
Hi! On Mon, Dec 22, 2014 at 1:07 PM, Heikki Linnakangas wrote: > Ok, thanks for the review! I have committed this, with some cleanup and > more comments added. > ISTM that checks in pairingheap_GISTSearchItem_cmp is incorrect. This function should perform inverse comparison. Thus, if item a should be checked first function should return 1. Current behavior doesn't lead to incorrect query answers, but it could be slower than correct version. -- With best regards, Alexander Korotkov. pairing_heap_cmp_fix.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] How about to have relnamespace and relrole?
Hello, thank you for the comment. The current patch lacks change in documentation and dependency stuff. Current framework doesn't consider changing pg_shdepend from column default expressions so the possible measures are the followings, I think. 1. Make pg_shdepend to have refobjsubid and add some deptypes of pg_depend, specifically DEPENDENCY_NORMAL is needed. Then, change the dependency stuff. 2. Simplly inhibit specifying them in default expressions. Integer or OID can act as the replacement. =# create table t1 (a int, b regrole default 'horiguti'::regrole); ERROR: Type 'regrole' cannot have a default value 1 is quite overkill but hardly seems to be usable. So I will go on 2 and post additional patch. At Thu, 12 Feb 2015 17:12:24 -0600, Jim Nasby wrote in <54dd3358.9030...@bluetreble.com> > On 2/12/15 5:28 AM, Kyotaro HORIGUCHI wrote: > > Hello, I changed the subject. # Ouch! the subject remaines wrong.. > > 1. Expected frequency of use > ... > > For that reason, although the current policy of deciding whether > > to have reg* types seems to be whether they have schema-qualified > > names, I think regrole and regnamespace are valuable to have. > > Perhaps schema qualification was the original intent, but I think at > this point everyone uses them for convenience. Why would I want to > type out JOIN pg_namespace n ON n.oid = blah.???namespace when I could > simply do ???namespace::regnamespace? Yes, that is the most important point of this patch. > > 2. Anticipaed un-optimizability > > > > Tom pointed that these reg* types prevents planner from > > optimizing the query, so we should refrain from having such > > machinary. It should have been a long-standing issue but reg*s > > sees to be rather faster than joining corresponding catalogs > > for moderate number of the result rows, but this raises another > > more annoying issue. > > > > > > 3. Potentially breakage of MVCC > > > > The another issue Tom pointed is potentially breakage of MVCC by > > using these reg* types. Syscache is invalidated on updates so > > this doesn't seem to be a problem on READ COMMITTED mode, but > > breaks SERIALIZABLE mode. But IMHO it is not so serious problem > > as long as such inconsistency occurs only on system catalog and > > it is explicitly documented togethee with the un-optimizability > > issue. I'll add a patch for this later. > > The way I look at it, all these arguments went out the window when > regclass was introduced. > > The reality is that reg* is *way* more convenient than manual > joins. The arguments about optimization and MVCC presumably apply to > all reg* casts, which means that ship has long since sailed. I agree basically, but I think some caveat is needed. I'll include that in the coming documentation patch. > If we offered views that made it easier to get at this data then maybe > this wouldn't matter so much. But DBA's don't want to join 3 catalogs > together to try and answer a simple question. It has been annoying me these days. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- 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_basebackup may fail to send feedbacks.
Thank you for the comment. Three new patches are attached. I
forgot to give a revision number on the previous patch, but I
think this is the 2nd version.
1. walrcv_reply_fix_94_v2.patch
Walreceiver patch applyable on master/REL9_4_STBLE/REL9_3_STABLE
2. walrcv_reply_fix_92_v2.patch
Walreceiver patch applyable on REL9_2_STABLE
3. walrcv_reply_fix_91_v2.patch
Walreceiver patch applyable on REL9_1_STABLE
At Sat, 14 Feb 2015 03:01:22 +0900, Fujii Masao wrote
in
> On Tue, Feb 10, 2015 at 7:48 PM, Kyotaro HORIGUCHI
> wrote:
> >> Introduce the maximum number of records that we can receive and
> >> process between feedbacks? For example, we can change
> >> XLogWalRcvSendHSFeedback() so that it's called per at least
> >> 8 records. I'm not sure what number is good, though...
> >
> > At the beginning of the "while(len > 0){if (len > 0){" block,
> > last_recv_timestamp is always kept up to date (by using
> > gettimeotda():). So we can use the variable instead of
> > gettimeofday() in my previous proposal.
>
> Yes, but something like last_recv_timestamp doesn't exist in
> REL9_1_STABLE. So you don't think that your proposed fix
> should be back-patched to all supported versions. Right?
Back to 9.3 has the loop with the same structure. 9.2 is in a bit
differenct shape but looks to have the same issue. 9.1 and 9.0
has the same staps with 9.2 but 9.0 doesn't has wal sender
timeout and 9.1 does not have a variable having current time.
9.3 and later: the previous patch works, but revised as your
comment.
9.2: The time of the last reply is stored in the file-scope
variable reply_message, and the current time is stored in
walrcv->lastMsgReceiptTime. The timeout is determined using
theses variables.
9.1: walrcv doesn't have lastMsgReceiptTime. The current time
should be acquired explicitly in the innermost loop. I tried
calling gettimeofday() once per several loops. The skip count
is determined by anticipated worst duration to process a wal
record and wal_receiver_status_interval. However, I doubt the
necessity to do so.. The value of the worst duration is simply
a random guess.
9.0: No point to do this kind of fix.
> > The start time of the timeout could be last_recv_timestamp at the
> > beginning of the while loop, since it is a bit earlier than the
> > actual time at the point.
>
> Sounds strange to me. I think that last_recv_timestamp should be
> compared with the time when the last feedback message was sent,
> e.g., maybe you can expose sendTime in XLogWalRcvSendReplay()
> as global variable, and use it to compare with last_recv_timestamp.
You're right to do exactly right thing, but, as you mentioned,
exposing sendTime is requied to do so. The solution I proposed is
the second-best assuming that wal_sender_timeout is recommended
to be longer enough (several times longer) than
wal_receiver_status_interval. If no one minds to expose sendTime,
it is the best solution. The attached patch does it.
# The added comment in the patch was wrong, though.
> When we get out of the WAL receive loop due to the timeout check
> which your patch added, XLogWalRcvFlush() is always executed.
> I don't think that current WAL file needs to be flushed at that time.
Thank you for pointing it out. Run XLogWalRcvSendReply(force =
true) immediately instead of breaking from the loop.
> > The another solution would be using
> > RegisterTimeout/enable_timeout_after and it seemed to be work for
> > me. It can throw out the time counting stuff in the loop we are
> > talking about and that of XLogWalRcvSendHSFeedback and
> > XLogWalRcvSendReply, but it might be a bit too large for the
> > gain.
>
> Yes, sounds overkill.
However, gettimeofday() for each recv is also a overkill for most
cases. I'll post the patches for receivelog.c based on the patch
for 9.1 wal receiver.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
>From 184f72ac34e7b787527dfa8ed76c1fbd2d970407 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi
Date: Tue, 10 Feb 2015 14:56:23 +0900
Subject: [PATCH] Make walreceiver to keep regular reply message even on heavy
load v2.
Wal receiver cannot send receiver reply message while it is receiving
continuous WAL stream caused by heavy load or something else. This
patch makes wal receiver to send reply message even on such a
situation.
---
src/backend/replication/walreceiver.c | 59 ---
1 file changed, 33 insertions(+), 26 deletions(-)
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index c2d4ed3..43d218d 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -91,6 +91,12 @@ static XLogSegNo recvSegNo = 0;
static uint32 recvOff = 0;
/*
+ * The time when the last wal receiver reply was sent. This is used to escape
+ * from receiving loop so that replay messages are kept regulary.
+ */
+static TimestampTz walRcvReplySendTime = 0;
+
+/*
* Flags set by inte
Re: [HACKERS] restrict global access to be readonly
>?6?9Jim Nasby writes: > On 2/14/15 3:14 PM, Robert Haas wrote: >> Although I like the idea, it's not clear to me how to implement it. > Throw an error in AssignTransactionId/GetNewTransactionId? >A whole lot depends on what you choose to mean by "read only". If it >?6?9means the same thing as "all transactions are READ ONLY", that would be >?6?9useful for some purposes, and it would have the advantage of having a >?6?9visible relationship to a long-established feature with the same name. >?6?9If you want it to mean "no writes to disk at all", that's something >?6?9totally different, and possibly not all that useful (eg, do you really >?6?9want to make sorts fail if they'd spill to disk? How about hint bit >?6?9updates?). Jim's suggestion above would be somewhere in the middle, >?6?9as it would successfully block use of temp tables but not eg. VACUUM. >?6?9Another possibility that would be attractive for replication-related >?6?9use-cases would be "nothing that generates WAL thank you very much". >?6?9I'm inclined to think that we should agree on the desired semantics >?6?9before jumping to implementation. >?6?9regards, tom lane The first choice Tom pointed makes sense to me: adding this as eqivalent to setting all subsequent transactions as read only. It is useful enough in the scenarios where disk limit for the instance is reached, we want to block all write access(this limit is typically soft limit and vacuum logs or sort spills could be permitted). I previously thought of the choice of "not generating any WAL" semantics, but now doubt if thats practically useful. We are forced to restart the old master with recovery mode during switching roles of master-slave, which would make it into the state of not generating any WAL. And for logical replication, seems setting transactions as readonly could do the job to avoid logs to be shipped to slave. One other thing to consider is the user to be blocked. I expect this command to prevent write access even for the superusers, since there may be other internal apps that connect as superuser and do writes, they are expected to be blocked too. And sometime we may use this command to avoid any unexpected write operation. Last thing is when the command returns. I expected it to return immediately and not waiting for existing active transactions to finish. This is to avoid existing long running transactions to block it and let the user to decide whether to wait or kill existing transactions.
Re: [HACKERS] pgaudit - an auditing extension for PostgreSQL
Hi list, On 20/01/15 23:03, Jim Nasby wrote:> On 1/20/15 2:20 PM, Robert Haas wrote: >> On Tue, Jan 20, 2015 at 1:05 AM, Abhijit >> Menon-Sen wrote: >>> >So when I'm trying to decide what to audit, I have to: >>> > >>> > (a) check if the current user is mentioned in .roles; if so, >>> audit. >>> > >>> > (b) check if the current user is a descendant of one of the roles >>> > mentioned in .roles; if not, no audit. >>> > >>> > (c) check for permissions granted to the "root" role and see if >>> that >>> > tells us to audit. >>> > >>> >Is that right? If so, it would work fine. I don't look forward to >>> trying >>> >to explain it to people, but yes, it would work (for anything you could >>> >grant permissions for). >> I think this points to fundamental weakness in the idea of doing this >> through the GRANT system. Some people are going want to audit >> everything a particular user does, some people are going to want to >> audit all access to particular objects, and some people will have more >> complicated requirements. Some people will want to audit even >> super-users, others especially super-users, others only non >> super-users. None of this necessarily matches up to the usual >> permissions framework. > > +1. In particular I'm very concerned with the idea of doing this via > roles, because that would make it trivial for any superuser to disable > auditing. Rejecting the audit administration through the GRANT system, on the grounds that it easy for the superuser to disable it, seems unreasonable to me, since superusers are different from non-superusers in a fundamental way. The superuser operates on the administration level of the database, in contrast with users that need access to the actual information in the data to perform their job. An organization that wants to implement an auditing strategy needs to think in different terms to audit user/application level actions, and administrator/superuser actions. Consequently it should be no surprise when PostgreSQL mechanisms for auditing behave different for superusers vs non superusers. The patch as it is, is targeted at auditing user/application level access to the database, and as such it matches the use case of auditing user actions. Auditing superuser access means auditing beyond the running database. The superuser can dump a table, and pipe this data everywhere outside of the auditing domain. I cannot begin to imagine the kind of security restrictions you'd need to audit what happens with data after libpq has produced the results. My best guess would be to incorporate some kind of separation of duty mechanism; only allow certain superuser operations with two people involved. 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] Table-level log_autovacuum_min_duration
Hi, Michael
I found that definition of VERBOSE and log_autovacuum is not
pretty match. For example, "VERBOSE" can output logs of
scanning indices and scanning detail of analyze, but
log_autovacuum can't output them.
Please see following sequences.
1. execute these queries.
DROP TABLE t1;
CREATE TABLE t1(id integer primary key,name text);
ALTER TABLE t1 SET (log_autovacuum_min_duration=0);
ALTER TABLE t1 ALTER COLUMN name SET STORAGE EXTERNAL;
INSERT INTO t1 SELECT GENERATE_SERIES(1,100),repeat('a',3000);
UPDATE t1 SET name='update';
2. after a while, output the following logs by autovacuum movement.
(For your convenience, I put the "### TYPE ###" prefix on each logs.)
### VERBOSE ###INFO: vacuuming "public.t1"
### VERBOSE ###INFO: scanned index "t1_pkey" to remove 33 row versions
### VERBOSE ###DETAIL: CPU 0.00s/0.00u sec elapsed 0.00 sec.
### VERBOSE ###INFO: "t1": removed 33 row versions in 1 pages
### VERBOSE ###DETAIL: CPU 0.00s/0.00u sec elapsed 0.00 sec.
### VERBOSE ###INFO: index "t1_pkey" now contains 100 row versions in 2 pages
### VERBOSE ###DETAIL: 33 index row versions were removed.
### VERBOSE ###0 index pages have been deleted, 0 are currently
reusable.
### VERBOSE ###CPU 0.00s/0.00u sec elapsed 0.00 sec.
### VERBOSE ###INFO: "t1": found 100 removable, 100 nonremovable row
versions in 2 out of 2 pages
### VERBOSE ###DETAIL: 0 dead row versions cannot be removed yet.
### VERBOSE ###There were 0 unused item pointers.
### VERBOSE ###Skipped 0 pages due to buffer pins.
### VERBOSE ###0 pages are entirely empty.
### VERBOSE ###CPU 0.00s/0.00u sec elapsed 0.00 sec.
### LOG_AVAC ###LOG: automatic vacuum of table "naoya.public.t1": index
scans: 1
### LOG_AVAC ###pages: 0 removed, 2 remain, 0 skipped due to pins
### LOG_AVAC ###tuples: 100 removed, 100 remain, 0 are dead but not
yet removable
### LOG_AVAC ###buffer usage: 47 hits, 4 misses, 4 dirtied
### LOG_AVAC ###avg read rate: 14.192 MB/s, avg write rate: 14.192 MB/s
### LOG_AVAC ###system usage: CPU 0.00s/0.00u sec elapsed 0.00 sec
### VERBOSE ###INFO: analyzing "public.t1"
### VERBOSE ###INFO: "t1": scanned 2 of 2 pages, containing 100 live rows
and 0 dead rows; 100 rows in sample, 100 estimated total rows
### LOG_AVAC ###LOG: automatic analyze of table "naoya.public.t1" system
usage: CPU 0.00s/0.00u sec elapsed 0.04 sec
### VERBOSE ###INFO: vacuuming "pg_toast.pg_toast_72882"
### VERBOSE ###INFO: scanned index "pg_toast_72882_index" to remove 200 row
versions
### VERBOSE ###DETAIL: CPU 0.00s/0.00u sec elapsed 0.00 sec.
### VERBOSE ###INFO: "pg_toast_72882": removed 200 row versions in 50 pages
### VERBOSE ###DETAIL: CPU 0.00s/0.00u sec elapsed 0.00 sec.
### VERBOSE ###INFO: index "pg_toast_72882_index" now contains 0 row
versions in 2 pages
### VERBOSE ###DETAIL: 200 index row versions were removed.
### VERBOSE ###0 index pages have been deleted, 0 are currently
reusable.
### VERBOSE ###CPU 0.00s/0.00u sec elapsed 0.00 sec.
### VERBOSE ###INFO: "pg_toast_72882": found 200 removable, 0 nonremovable
row versions in 50 out of 50 pages
### VERBOSE ###DETAIL: 0 dead row versions cannot be removed yet.
### VERBOSE ###There were 0 unused item pointers.
### VERBOSE ###Skipped 0 pages due to buffer pins.
### VERBOSE ###0 pages are entirely empty.
### VERBOSE ###CPU 0.00s/0.00u sec elapsed 0.00 sec.
### VERBOSE ###INFO: "pg_toast_72882": truncated 50 to 0 pages
### VERBOSE ###DETAIL: CPU 0.00s/0.00u sec elapsed 0.03 sec.
### LOG_AVAC ###LOG: automatic vacuum of table
"naoya.pg_toast.pg_toast_72882": index scans: 1
### LOG_AVAC ###pages: 50 removed, 0 remain, 0 skipped due to pins
### LOG_AVAC ###tuples: 200 removed, 0 remain, 0 are dead but not yet
removable
### LOG_AVAC ###buffer usage: 223 hits, 2 misses, 1 dirtied
### LOG_AVAC ###avg read rate: 0.457 MB/s, avg write rate: 0.228 MB/s
### LOG_AVAC ### system usage: CPU 0.00s/0.00u sec elapsed 0.03 sec
I think VACOPT_VERBOSE should not be easily replaced to log_min_duration>=0.
And, in this v7 patch looks that VERBOSE log is always output
in INFO-Level when log_autovacuum_min_duration is set to 0.
Is this your point?
Regards,
---
Naoya
--
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] Transactions involving multiple postgres foreign servers
Hi All, Here are the steps and infrastructure for achieving atomic commits across multiple foreign servers. I have tried to address most of the concerns raised in this mail thread before. Let me know, if I have left something. Attached is a WIP patch implementing the same for postgres_fdw. I have tried to make it FDW-independent. A. Steps during transaction processing 1. When an FDW connects to a foreign server and starts a transaction, it registers that server with a boolean flag indicating whether that server is capable of participating in a two phase commit. In the patch this is implemented using function RegisterXactForeignServer(), which raises an error, thus aborting the transaction, if there is at least one foreign server incapable of 2PC in a multiserver transaction. This error thrown as early as possible. If all the foreign servers involved in the transaction are capable of 2PC, the function just updates the information. As of now, in the patch the function is in the form of a stub. Whether a foreign server is capable of 2PC, can be a. FDW level decision e.g. file_fdw as of now, is incapable of 2PC but it can build the capabilities which can be used for all the servers using file_fdw b. a decision based on server version type etc. thus FDW can decide that by looking at the server properties for each server c. a user decision where the FDW can allow a user to specify it in the form of CREATE/ALTER SERVER option. Implemented in the patch. For a transaction involving only a single foreign server, the current code remains unaltered as two phase commit is not needed. Rest of the discussion pertains to a transaction involving more than one foreign servers. At the commit or abort time, the FDW receives call backs with the appropriate events. FDW then takes following actions on each event. 2. On XACT_EVENT_PRE_COMMIT event, the FDW coins one prepared transaction id per foreign server involved and saves it along with xid, dbid, foreign server id and user mapping and foreign transaction status = PREPARING in-memory. The prepared transaction id can be anything represented as byte string. Same information is flushed to the disk to survive crashes. This is implemented in the patch as prepare_foreign_xact(). Persistent and in-memory storages and their usages are discussed later in the mail. FDW then prepares the transaction on the foreign server. If this step is successful, the foreign transaction status is changed to PREPARED. If the step is unsuccessful, the local transaction is aborted and each FDW will receive XACT_EVENT_ABORT (discussed later). The updates to the foreign transaction status need not be flushed to the disk, as they can be inferred from the status of local transaction. 3. If the local transaction is committed, the FDW callback will get XACT_EVENT_COMMIT event. Foreign transaction status is changed to COMMITTING. FDW tries to commit the foreign transaction with the prepared transaction id. If the commit is successful, the foreign transaction entry is removed. If the commit is unsuccessful because of local/foreign server crash or network failure, the foreign prepared transaction resolver takes care of the committing it at later point of time. 4. If the local transaction is aborted, the FDW callback will get XACT_EVENT_ABORT event. At this point, the FDW may or may not have prepared a transaction on foreign server as per step 1 above. If it has not prepared the transaction, it simply aborts the transaction on foreign server; a server crash or network failure doesn't alter the ultimate result in this case. If FDW has prepared the foreign transaction, it updates the foreign transaction status as ABORTING and tries to rollback the prepared transaction. If the rollback is successful, the foreign transaction entry is removed. If the rollback is not successful, the foreign prepared transaction resolver takes care of aborting it at later point of time. B. Foreign prepared transaction resolver --- In the patch this is implemented as a built-in function pg_fdw_resolve(). Ideally the functionality should be run by a background worker process frequently. The resolver looks at each entry and invokes the FDW routine to resolve the transaction. The FDW routine returns boolean status: true if the prepared transaction was resolved (committed/aborted), false otherwise. The resolution is as follows - 1. If foreign transaction status is COMMITTING or ABORTING, commits or aborts the prepared transaction resp through the FDW routine. If the transaction is successfully resolved, it removes the foreign transaction entry. 2. Else, it checks if the local transaction was committed or aborted, it update the foreign transaction status accordingly and takes the action according to above step 1. 3. The resolver doesn't touch entries created by in-progress local transactions. If server/backend crashes after it has regis
Re: [HACKERS] pg_basebackup -x/X doesn't play well with archive_mode & wal_keep_segments
On 2015-02-17 12:18:41 +0900, Fujii Masao wrote: > On Fri, Feb 13, 2015 at 9:18 AM, Andres Freund wrote: > > Somewhat uckily it's 9.2 only (9.3, 9.4 and master look correct, earlier > > releases don't have pg_receivexlog) > > Are you planning to back-patch the fix to 9.2? Yes, but I want to look through all versions, to make sure there's no other merge resolution mistakes lurking. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
