Re: [HACKERS] Enabling Checksums
On 26 November 2012 02:32, Jeff Davis pg...@j-davis.com wrote: Updated both patches. Changes: * Moved the changes to pageinspect into the TLI patch, because it makes more sense to be a part of that patch and it also reduces the size of the main checksums patch. * Fix off-by-one bug in checksum calculation * Replace VerificationInfo in the function names with Checksum, which is shorter. * Make the checksum algorithm process 4 bytes at a time and sum into a signed 64-bit int, which is faster than byte-at-a-time. Also, forbid zero in either byte of the checksum, because that seems like a good idea. I've done quite a bit of testing at this point, and everything seems fine to me. I've tested various kinds of errors (bytes being modified or zeroed at various places of the header and data areas, transposed pages) at 8192 and 32768 page sizes. I also looked at the distribution of checksums in various ways (group by checksum % prime for various primes, and not seeing any skew), and I didn't see any worrying patterns. I think the way forwards for this is... 1. Break out the changes around inCommit flag, since that is just uncontroversial refactoring. I can do that. That reduces the noise level in the patch and makes it easier to understand the meaningful changes. 2. Produce an SGML docs page that describes how this works, what the limitations and tradeoffs are. Reliability the WAL could use an extra section2 header called Checksums (wal.sgml). This is essential for users AND reviewers to ensure everybody has understood this (heck, I can't remember everything about this either...) 3. I think we need an explicit test of this feature (as you describe above), rather than manual testing. corruptiontester? 4. We need some general performance testing to show whether this is insane or not. But this looks in good shape for commit otherwise. -- Simon Riggs 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] WIP: index support for regexp search
On 02.12.2012 20:19, Tom Lane wrote: Alexander Korotkovaekorot...@gmail.com writes: Nice idea to delay expanding colors to characters! Obviously, we should delay expanding inly alphanumerical characters. Because non-alphanumberical characters influence graph structure. Trying to implement... Uh, why would that be? Colors are colors. The regexp machinery doesn't care whether they represent alphanumerics or not. (Or to be more precise, if there is a situation where it makes a difference, separate colors will have been created for each set of characters that need to be distinguished.) The regexp machinery doesn't care, but the trigrams that pg_trgm extracts only contain alphanumeric characters. So if by looking at the CNFA graph produced by the regexp machinery you conclude that any matching strings must contain three-letter sequences %oo and #oo, you can just club them together into oo trigram. I think you can run a pre-processing step to the colors, and merge colors that are equivalent as far as trigrams are considered. For example, if you have a color that contains only character '%', and another that contains character '#', you can treat them as the same hcolor. You might then be able to simplify the CNFA. Actually, it would be even better if you could apply the pre-processing to the regexp before the regexp machinery turns it into a CNFA. Not sure how easy it would be to do such pre-processing. BTW, why create the path matrix? You could check the check array of trigrams in the consistent function directly against the graph. Consistent should return true, iff there is a path through the graph following only arcs that contain trigrams present in the check array. Finding a path through a complex graph could be expensive, O(|E|), but if the path is complex, the path matrix would be large as well, and checking against a large matrix isn't exactly free either. It would allow you to avoid overflows caused by having too many paths through the graph. - 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] ALTER command reworks
Kohei KaiGai kai...@kaigai.gr.jp writes: The attached patch is a revised version. I think you fixed all the problems I could see with your patch. It applies cleanly (with some offsets), compiles cleanly (I have a custom Makefile with CUSTOM_COPT=-Werror) and passes regression tests. I'll go mark it as ready for commiter. Thanks! It follows the manner in ExecAlterObjectSchemaStmt(); that tries to check name duplication for object classes that support catcache with name-key. Elsewhere, it calls individual routines for each object class to solve object name and check namespace conflicts. For example, RenameFunction() is still called from ExecRenameStmt(), however, it looks up the given function name and checks namespace conflict only, then it just invokes AlterObjectRename_internal() in spite of system catalog updates by itself. I think that's much better this way. It allows to use this consolidated routine by object classes with special rule for namespace conflicts, such as collation. In addition, this design allowed to eliminate get_object_type() to pull OBJECT_* enum from class_id/object_id pair. Thanks for having done this refactoring. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TABLE ... NOREWRITE option
Noah Misch n...@leadboat.com writes: Acquiring the lock could still take an unpredictable amount of time. I think there's a new GUC brewing about setting the lock timeout separately from the statement timeout, right? being said, I share Tom's doubts. The DEBUG1 messages are a sorry excuse for a UI, but I'm not seeing a clear improvement in NOREWRITE. EXPLAIN ALTER TABLE would be the next step I guess. I discovered plenty of magic tricks when working on the rewriting support for that command, and I think exposing them in the EXPLAIN output would go a long way towards reducing some POLA violations. Ideally the EXPLAIN command would include names of new objects created by the command, such as constraints and indexes. My first thought is to add more detailed EXPLAIN support for DDL... Although that unfortunately broadens the scope of this a tiny bit. That would be ideal. +1 Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refactoring standby mode logic
On 29 November 2012 09:06, Heikki Linnakangas hlinnakan...@vmware.com wrote: The code that reads the WAL from the archive, from pg_xlog, and from a master server via walreceiver, is quite complicated. I'm talking about the WaitForWALToBecomeAvailable() function in xlog.c. I got frustrated with that while working on the switching timeline over streaming replication patch. Attached is a patch to refactor that logic into a more straightforward state machine. It's always been a kind of a state machine, but it's been hard to see, as the code wasn't explicitly written that way. Any objections? The only user-visible effect is that this slightly changes the order that recovery tries to read files from the archive, and pg_xlog, in the presence of multiple timelines. At the moment, if recovery fails to find a file on current timeline in the archive, it then tries to find it in pg_xlog. If it's not found there either, it checks if the file on next timeline exists in the archive, and then checks if exists in pg_xlog. For example, if we're currently recovering timeline 2, and target timeline is 4, and we're looking for WAL file A, the files are searched for in this order: 1. File 0004000A in archive 2. File 0004000A in pg_xlog 3. File 0003000A in archive 4. File 0003000A in pg_xlog 5. File 0002000A in archive 6. File 0002000A in pg_xlog With this patch, the order is: 1. File 0004000A in archive 2. File 0003000A in archive 3. File 0002000A in archive 4. File 0004000A in pg_xlog 5. File 0003000A in pg_xlog 6. File 0002000A in pg_xlog This change should have no effect in normal restore scenarios. It'd only make a difference if some files in the middle of the sequence of WAL files are missing from the archive, but have been copied to pg_xlog manually, and only if that file contains a timeline switch. Even then, I think I like the new order better; it's easier to explain if nothing else. Sorry, forgot to say fine by me. This probably helps the avoidance of shutdown checkpoints, since for that, we need to skip retrieving from archive once we're up. -- Simon Riggs 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] Refactoring standby mode logic
On 30.11.2012 13:11, Dimitri Fontaine wrote: Hi, Heikki Linnakangashlinnakan...@vmware.com writes: Attached is a patch to refactor that logic into a more straightforward state machine. It's always been a kind of a state machine, but it's been hard to see, as the code wasn't explicitly written that way. Any objections? On a quick glance over, looks good to me. Making that code simpler to read and reason about seems a good goal. Thanks. This change should have no effect in normal restore scenarios. It'd only make a difference if some files in the middle of the sequence of WAL files are missing from the archive, but have been copied to pg_xlog manually, and only if that file contains a timeline switch. Even then, I think I like the new order better; it's easier to explain if nothing else. I'm not understanding the sequence difference well enough to comment here, but I think some people are currently playing tricks in their failover scripts with moving files directly to the pg_xlog of the server to be promoted. That's still perfectly ok. It's only if you have a diverged timeline history, and you have files from one timeline in the archive and files from another in pg_xlog that you'll see a difference. But in such a split situation, it's quite arbitrary which timeline recovery will follow anyway, I don't think anyone can sanely rely on either behavior. Is it possible for your refactoring to keep the old sequence? Hmm, perhaps, but I think it would complicate the logic a bit. Doesn't seem worth it. Committed.. - 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] Proposal for Allow postgresql.conf values to be changed via SQL
On Saturday, December 01, 2012 10:00 PM Tom Lane wrote:
Amit Kapila amit.kap...@huawei.com writes:
On Saturday, December 01, 2012 1:30 AM Tom Lane wrote:
which cannot work if persistent could be a var_name, because bison
has to decide whether to reduce opt_persistent to PERSISTENT or empty
before it can see if there's anything after the var_name. So the fix
is to not use an opt_persistent production, but spell out both
alternatives:
RESET var_name
RESET PERSISTENT var_name
Now bison doesn't have to choose what to reduce until it can see the end
of the statement; that is, once it's scanned RESET and PERSISTENT, the
choice of whether to treat PERSISTENT as a var_name can be conditional
on whether the next token is a name or EOL.
With the above suggestion also, it's not able to resolve shift/reduce
errors.
However I have tried with below changes in gram.y, it works after below
change.
%leftPERSISTENT
VariableResetStmt:
RESET opt_persistent reset_common
{
VariableSetStmt *n = $3;
n-is_persistent = $2;
$$ = (Node *) n;
}
;
opt_persistent: PERSISTENT{ $$ = TRUE; }
| /*EMPTY*/%prec Op{ $$ = FALSE; }
;
I am not sure if there are any problems with above change.
Found one difference with the change is, the command reset persistent
execution results in different errors with/without change.
without change:
unrecognized configuration parameter persistent.
with change:
syntax error at or near ;
As per your yesterday's mail, it seems to me you are disinclined to have
RESET PERSISTENT syntax.
Can we conclude on that point? My only worry is user will have no way to
delete the entry from .auto.conf file.
Suggestions?
With Regards,
Amit Kapila.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH 11/14] Introduce wal decoding via catalog timetravel
Hi Steve, On 2012-12-02 21:52:08 -0500, Steve Singer wrote: On 12-11-14 08:17 PM, Andres Freund wrote: I am getting errors like the following when I try to use either your test_decoding plugin or my own (which does even less than yours) LOG: database system is ready to accept connections LOG: autovacuum launcher started WARNING: connecting to WARNING: Initiating logical rep LOG: computed new xmin: 773 LOG: start reading from 0/17F5D58, scrolled back to 0/17F4000 LOG: got new xmin 773 at 25124280 LOG: found initial snapshot (via running xacts). Done: 1 WARNING: reached consistent point, stopping! WARNING: Starting logical replication LOG: start reading from 0/17F5D58, scrolled back to 0/17F4000 LOG: found initial snapshot (via running xacts). Done: 1 FATAL: cannot read pg_class without having selected a database TRAP: FailedAssertion(!(SHMQueueEmpty((MyProc-myProcLocks[i]))), File: proc.c, Line: 759) This seems to be happening under the calls at reorderbuffer.c:832if (!SnapBuildHasCatalogChanges(NULL, xid, change-relnode)) Two things: 1) Which exact options are you using for pg_receivellog? Not -d replication by any chance? 2) Could you check that you really have a fully clean build? That has hit me in the past as well. 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] WIP: index support for regexp search
On Mon, Dec 3, 2012 at 2:05 PM, Heikki Linnakangas hlinnakan...@vmware.comwrote: On 02.12.2012 20:19, Tom Lane wrote: Alexander Korotkovaekorot...@gmail.com writes: Nice idea to delay expanding colors to characters! Obviously, we should delay expanding inly alphanumerical characters. Because non-alphanumberical characters influence graph structure. Trying to implement... Uh, why would that be? Colors are colors. The regexp machinery doesn't care whether they represent alphanumerics or not. (Or to be more precise, if there is a situation where it makes a difference, separate colors will have been created for each set of characters that need to be distinguished.) The regexp machinery doesn't care, but the trigrams that pg_trgm extracts only contain alphanumeric characters. So if by looking at the CNFA graph produced by the regexp machinery you conclude that any matching strings must contain three-letter sequences %oo and #oo, you can just club them together into oo trigram. I think you can run a pre-processing step to the colors, and merge colors that are equivalent as far as trigrams are considered. For example, if you have a color that contains only character '%', and another that contains character '#', you can treat them as the same hcolor. You might then be able to simplify the CNFA. Actually, it would be even better if you could apply the pre-processing to the regexp before the regexp machinery turns it into a CNFA. Not sure how easy it would be to do such pre-processing. Treating colors as same should be possible only for colors which has no alphanumeric characters, because colors are non-overlapping. However, this optimization could be significant in some cases. BTW, why create the path matrix? You could check the check array of trigrams in the consistent function directly against the graph. Consistent should return true, iff there is a path through the graph following only arcs that contain trigrams present in the check array. Finding a path through a complex graph could be expensive, O(|E|), but if the path is complex, the path matrix would be large as well, and checking against a large matrix isn't exactly free either. It would allow you to avoid overflows caused by having too many paths through the graph. Actually, I generally dislike path matrix for same reasons. But: 1) Output graphs could contain trigrams which are completely useless for search. For example, for regex /(abcdefgh)*ijk/ we need only ijk trigram while graph would contain much more.Path matrix is a method to get rid of all of them. 2) If we use color trigrams then we need some criteria for which color trigrams to expand into trigrams. Simultaneously, we shouldn't allow path from initial state to the final by unexpanded trigrams. It seems much harder to do with graph than with matrix. -- With best regards, Alexander Korotkov.
Re: [HACKERS] [PATCH 11/14] Introduce wal decoding via catalog timetravel
On 2012-12-03 13:22:12 +0100, Andres Freund wrote: Hi Steve, On 2012-12-02 21:52:08 -0500, Steve Singer wrote: On 12-11-14 08:17 PM, Andres Freund wrote: I am getting errors like the following when I try to use either your test_decoding plugin or my own (which does even less than yours) LOG: database system is ready to accept connections LOG: autovacuum launcher started WARNING: connecting to WARNING: Initiating logical rep LOG: computed new xmin: 773 LOG: start reading from 0/17F5D58, scrolled back to 0/17F4000 LOG: got new xmin 773 at 25124280 LOG: found initial snapshot (via running xacts). Done: 1 WARNING: reached consistent point, stopping! WARNING: Starting logical replication LOG: start reading from 0/17F5D58, scrolled back to 0/17F4000 LOG: found initial snapshot (via running xacts). Done: 1 FATAL: cannot read pg_class without having selected a database TRAP: FailedAssertion(!(SHMQueueEmpty((MyProc-myProcLocks[i]))), File: proc.c, Line: 759) This seems to be happening under the calls at reorderbuffer.c:832if (!SnapBuildHasCatalogChanges(NULL, xid, change-relnode)) Two things: 1) Which exact options are you using for pg_receivellog? Not -d replication by any chance? This seems to produce exactly that kind off error messages. I added some error checking around that. Pushed. Thanks! 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] Switching timeline over streaming replication
Is this patch available in version 9.2.1 ? Senthil -- View this message in context: http://postgresql.1045698.n5.nabble.com/Switching-timeline-over-streaming-replication-tp5723547p5734744.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.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] Enabling Checksums
On 3 December 2012 09:56, Simon Riggs si...@2ndquadrant.com wrote: I think the way forwards for this is... 1. Break out the changes around inCommit flag, since that is just uncontroversial refactoring. I can do that. That reduces the noise level in the patch and makes it easier to understand the meaningful changes. Done. -- Simon Riggs 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] Enabling Checksums
On 26 November 2012 02:32, Jeff Davis pg...@j-davis.com wrote: * Make the checksum algorithm process 4 bytes at a time and sum into a signed 64-bit int, which is faster than byte-at-a-time. Also, forbid zero in either byte of the checksum, because that seems like a good idea. Like that, especially the bit where we use the blocknumber as the seed for the checksum, so it will detect transposed pages. That's also a really neat way of encrypting the data for anybody that tries to access things via direct anonymous file access. -- Simon Riggs 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] Switching timeline over streaming replication
On 03.12.2012 14:21, senthilnathan wrote: Is this patch available in version 9.2.1 ? Nope, this is for 9.3. - 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] Refactoring standby mode logic
Heikki Linnakangas hlinnakan...@vmware.com writes: I'm not understanding the sequence difference well enough to comment here, but I think some people are currently playing tricks in their failover scripts with moving files directly to the pg_xlog of the server to be promoted. That's still perfectly ok. It's only if you have a diverged timeline history, and you have files from one timeline in the archive and files from another in pg_xlog that you'll see a difference. But in such a split situation, it's quite arbitrary which timeline recovery will follow anyway, I don't think anyone can sanely rely on either behavior. Fair enough. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Patch to compute Max LSN of Data Pages
I think we should expect provided path to be relative to current directory or may consider it to be relative to either one of Data or CWD. Because normally we expect path to be relative to CWD if some program is asking for path in command line. Please find the attached patch to make the path relative to CWD and check if the path is under data directory. Now the only point raised by Alvaro and you for this patch which is not yet addressed is : Hmm. I think I'd expect that if I give pg_computemaxlsn a number, it should consider that it is a relfilenode, and so it should get a list of all segments for all forks of it. So if I give 12345 it should get 12345, 12345.1 and so on, and also 12345_vm 12345_vm.1 and so on. However, if what I give it is a path, i.e. it contains a slash, I think it should only consider the specific file mentioned. In that light, I'm not sure that command line options chosen are the best set. I am just not sure whether we should handle this functionality and if we have to handle what is better way to provide it to user. Shall we provide new option -r or something for it. Opinions/Suggestions? IMHO, such functionality can be easily extendable in future. However I have no problem in implementing such functionality if you are of opinion that this is basic and it should go with first version of feature. With Regards, Amit Kapila. pg_computemaxlsn_v3.patch Description: pg_computemaxlsn_v3.patch -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.3] OAT_POST_ALTER object access hooks
On Tue, Nov 20, 2012 at 8:43 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
I'd like to have catalog/objectaccess.c to wrap-up invocation of hooks, rather
than doing all the stuffs with macros. It allows to use local variables,
unlike
macros; that has a risk of naming conflict with temporary variable for
ObjectAccessPostCreate.
So, how about to have a following definition for example?
void
InvokePostAlterHookArgs(Oid classId, Oid objectId, int subId,
Oid auxiliaryId, bool is_internal)
{
if (object_access_hook)
{
ObjectAccessPostAlterpa_arg;
memset(pa_arg, 0, sizeof(ObjectAccessPostAlter));
pa_arg.auxiliary_id = auxiliary_id;
pa_arg.is_internal = is_internal;
(*object_access_hook)(OAT_POST_ALTER,
classId, objectId, subId,
(void *) pa_arg);
}
}
and
#define InvokePostAlterHook(classId, objectId, subId) \
InvokePostAlterHookArgs(classId, objectId, subId, InvalidOid, false)
for most points to call.
This has the disadvantage of incurring the overhead of a function call
even if (as will typically be the case) there is no object access
hook. I still prefer having the if (object_access_hook) test in the
macro, though of course I have no problem with having the macro call
a function if it's set.
--
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] [v9.3] OAT_POST_ALTER object access hooks
On Sat, Dec 1, 2012 at 2:57 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: * Do we need OAT_POST_ALTER hook even if no fields were updated actually? In case when ALTER SET OWNER, it checks object's ownership only when current and new user-id is not same. Right now, I follow this manner on OAT_POST_ALTER invocation. However, I'm inclined to consider the hook should be invoked when no fields are actually updated also. (It allows extension-side to determine necessity of processing something.) I agree. I think it should always be called. * When tablespace of relation was changed, it seems to me the point to invoke OAT_POST_ALTER hook should be after ATRewriteTable(). However, it usually long time to rewrite whole the table if it already have large number of rows. I'm not 100% certain to put hook here, so this version does not support hook when tablespace changes. Well, if it's a post-alter hook, it should presumably happen as close to the end of processing as possible. But are you sure that's really what you want? I would think that for SE-Linux you'd be wanting to get control much earlier in the process. -- 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] Hot Standby Feedback should default to on in 9.3+
On Fri, Nov 30, 2012 at 6:41 PM, Andres Freund and...@2ndquadrant.com wrote: While we're talking about changing defaults, how about changing the default value of the recovery.conf parameter 'standby_mode' to on? Not sure about anybody else, but I never want it any other way. Hm. But only if there is a recovery.conf I guess? Yeah, wouldn't make much sense otherwise. -- 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] PQconninfo function for libpq
On Fri, Nov 30, 2012 at 1:02 AM, Magnus Hagander mag...@hagander.net wrote: In the interest of moving things along, I'll remove these for now at least, and commit the rest of the patch, so we can keep working on the basebacku part of things. I see you committed this - does this possibly provide infrastructure that could be used to lift the restriction imposed by the following commit? commit fe21fcaf8d91a71c15ff25276f9fa81e0cd1dba9 Author: Bruce Momjian br...@momjian.us Date: Wed Aug 15 19:04:52 2012 -0400 In psql, if the is no connection object, e.g. due to a server crash, require all parameters for \c, rather than using the defaults, which might be wrong. Because personally, I find the new behavior no end of annoying. -- 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] Review: Extra Daemons / bgworker
Markus Wanner wrote: On 11/28/2012 03:51 PM, Alvaro Herrera wrote: I remember your patchset. I didn't look at it for this round, for no particular reason. I did look at KaiGai's submission from two commitfests ago, and also at a patch from Simon which AFAIK was never published openly. Simon's patch merged the autovacuum code to make autovac workers behave like bgworkers as handled by his patch, just like you suggest. To me it didn't look all that good; I didn't have the guts for that, hence the separation. If later bgworkers are found to work well enough, we can port autovac workers to use this framework, but for now it seems to me that the conservative thing is to leave autovac untouched. Hm.. interesting to see how the same idea grows into different patches and gets refined until we end up with something considered committable. Do you remember anything in particular that didn't look good? Would you help reviewing a patch on top of bgworker-7 that changed autovacuum into a bgworker? I'm not really that interested in it currently ... and there are enough other patches to review. I would like bgworkers to mature a bit more and get some heavy real world testing before we change autovacuum. How are you envisioning that the requests would occur? Just like av_launcher does it now: set a flag in shared memory and signal the postmaster (PMSIGNAL_START_AUTOVAC_WORKER). I'm not sure how this works. What process is in charge of setting such a flag? In assign_maxconnections, et al, GetNumRegisteredBackgroundWorkers() is used in relation to MAX_BACKENDS or to calculate MaxBackends. That seems to leak the bgworkers that registered with neither BGWORKER_SHMEM_ACCESS nor BGWORKER_BACKEND_DATABASE_CONNECTION set. Or is there any reason to discount such extra daemon processes? No, I purposefully let those out, because those don't need a PGPROC. In fact that seems to me to be the whole point of non-shmem-connected workers: you can have as many as you like and they won't cause a backend-side impact. You can use such a worker to connect via libpq to the server, for example. Hm.. well, in that case, the shmem-connected ones are *not* counted. If I create and run an extensions that registers 100 shmem-connected bgworkers, I cannot connect to a system with max_connections=100 anymore, because bgworkers then occupy all of the connections, already. This is actually a very silly bug: it turns out that guc.c obtains the count of workers before workers have actually registered. So this necessitates some reshuffling. Or put another way: max_connections should always be the max number of *client* connections the DBA wants to allow. Completely agreed. -- Álvaro Herrerahttp://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] [PATCH 11/14] Introduce wal decoding via catalog timetravel
On 12-12-03 07:42 AM, Andres Freund wrote: Two things: 1) Which exact options are you using for pg_receivellog? Not -d replication by any chance? Yes that is exactly what I'md doing. Using a real database name instead makes this go away. Thanks This seems to produce exactly that kind off error messages. I added some error checking around that. Pushed. Thanks! Andres Freund -- Andres Freundhttp://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] [v9.3] OAT_POST_ALTER object access hooks
2012/12/3 Robert Haas robertmh...@gmail.com:
On Tue, Nov 20, 2012 at 8:43 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
I'd like to have catalog/objectaccess.c to wrap-up invocation of hooks,
rather
than doing all the stuffs with macros. It allows to use local variables,
unlike
macros; that has a risk of naming conflict with temporary variable for
ObjectAccessPostCreate.
So, how about to have a following definition for example?
void
InvokePostAlterHookArgs(Oid classId, Oid objectId, int subId,
Oid auxiliaryId, bool is_internal)
{
if (object_access_hook)
{
ObjectAccessPostAlterpa_arg;
memset(pa_arg, 0, sizeof(ObjectAccessPostAlter));
pa_arg.auxiliary_id = auxiliary_id;
pa_arg.is_internal = is_internal;
(*object_access_hook)(OAT_POST_ALTER,
classId, objectId, subId,
(void *) pa_arg);
}
}
and
#define InvokePostAlterHook(classId, objectId, subId) \
InvokePostAlterHookArgs(classId, objectId, subId, InvalidOid, false)
for most points to call.
This has the disadvantage of incurring the overhead of a function call
even if (as will typically be the case) there is no object access
hook. I still prefer having the if (object_access_hook) test in the
macro, though of course I have no problem with having the macro call
a function if it's set.
OK, I'll adjust the macro definitions to reduce empty function calls.
Thanks,
--
KaiGai Kohei kai...@kaigai.gr.jp
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH 11/14] Introduce wal decoding via catalog timetravel
On 2012-12-03 09:35:55 -0500, Steve Singer wrote: On 12-12-03 07:42 AM, Andres Freund wrote: Two things: 1) Which exact options are you using for pg_receivellog? Not -d replication by any chance? Yes that is exactly what I'md doing. Using a real database name instead makes this go away. Was using replication an accident or do you think it makes sense in some way? 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] [PATCH 11/14] Introduce wal decoding via catalog timetravel
On 12-12-03 09:48 AM, Andres Freund wrote: On 2012-12-03 09:35:55 -0500, Steve Singer wrote: On 12-12-03 07:42 AM, Andres Freund wrote: Yes that is exactly what I'md doing. Using a real database name instead makes this go away. Was using replication an accident or do you think it makes sense in some way? The 'replication' line in pg_hba.conf made me think that the database name had to be replication for walsender connections. Greetings, Andres Freund -- Andres Freundhttp://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] [v9.3] OAT_POST_ALTER object access hooks
2012/12/3 Robert Haas robertmh...@gmail.com: On Sat, Dec 1, 2012 at 2:57 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: * Do we need OAT_POST_ALTER hook even if no fields were updated actually? In case when ALTER SET OWNER, it checks object's ownership only when current and new user-id is not same. Right now, I follow this manner on OAT_POST_ALTER invocation. However, I'm inclined to consider the hook should be invoked when no fields are actually updated also. (It allows extension-side to determine necessity of processing something.) I agree. I think it should always be called. OK, I'll move the point to invoke hooks into outside of the if-block. * When tablespace of relation was changed, it seems to me the point to invoke OAT_POST_ALTER hook should be after ATRewriteTable(). However, it usually long time to rewrite whole the table if it already have large number of rows. I'm not 100% certain to put hook here, so this version does not support hook when tablespace changes. Well, if it's a post-alter hook, it should presumably happen as close to the end of processing as possible. But are you sure that's really what you want? I would think that for SE-Linux you'd be wanting to get control much earlier in the process. In my preference, an error shall be raised prior to whole of the table rewrite stuff; it can take exclusive table lock during possible TB or PB of disk-i/o. Even if sepgsql got control after all and raise an access control error, I'm not happy so much. :-( As we discussed before, it is hard to determine which attributes shall be informed to extension via object_access_hook, so the proposed post-alter hook (that allows to compare older and newer versions) works fine on 99% cases. However, I'm inclined to handle SET TABLESPACE as an exception of this scenario. For example, an idea is to define OAT_PREP_ALTER event additionally, but only invoked very limited cases that takes unignorable side-effects until system catalog updates. For consistency of hook, OAT_POST_ALTER event may also ought to be invoked just after catalog updates of pg_class-reltablespace, but is_internal=true. How about your opinion? Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] Review: Extra Daemons / bgworker
On 12/03/2012 03:28 PM, Alvaro Herrera wrote: I'm not really that interested in it currently ... and there are enough other patches to review. I would like bgworkers to mature a bit more and get some heavy real world testing before we change autovacuum. Understood. Just like av_launcher does it now: set a flag in shared memory and signal the postmaster (PMSIGNAL_START_AUTOVAC_WORKER). I'm not sure how this works. What process is in charge of setting such a flag? The only process that currently starts background workers ... ehm ... autovacuum workers is the autovacuum launcher. It uses the above Postmaster Signal in autovacuum.c:do_start_autovacuum_worker() to have the postmaster launch bg/autovac workers on demand. (And yes, this kind of is an exception to the rule that the postmaster must not rely on shared memory. However, these are just flags we write atomically, see pmsignal.[ch]) I'd like to extend that, so that other processes can request to start (pre-registered) background workers more dynamically. I'll wait for an update of your patch, though. This is actually a very silly bug: it turns out that guc.c obtains the count of workers before workers have actually registered. So this necessitates some reshuffling. Okay, thanks. Regards Markus Wanner -- 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] Proposal for Allow postgresql.conf values to be changed via SQL
On Sat, Dec 1, 2012 at 11:45 AM, Tom Lane t...@sss.pgh.pa.us wrote: I wrote: But even if we can't make that work, it's not grounds for reserving PERSISTENT. Instead I'd be inclined to forget about RESET PERSISTENT syntax and use, say, SET PERSISTENT var_name TO DEFAULT to mean that. (BTW, I wonder what behavior that syntax has now in your patch.) In fact, rereading this, I wonder why you think RESET PERSISTENT is a good idea even if there were no bison issues with it. We don't write RESET LOCAL or RESET SESSION, so it seems asymmetric to have RESET PERSISTENT. I think this feature is more analagous to ALTER DATABASE .. SET or ALTER ROLE .. SET. Which is, incidentally, another reason I don't like SET PERSISTENT as a proposed syntax. But even if we stick with that syntax, it feels weird to have an SQL command to put a line into postgresql.conf.auto and no syntax to take it back out again. We wouldn't allow a CREATE command without a corresponding DROP operation... -- 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] Proposal for Allow postgresql.conf values to be changed via SQL
On Mon, Dec 3, 2012 at 6:38 AM, Amit Kapila amit.kap...@huawei.com wrote:
On Saturday, December 01, 2012 10:00 PM Tom Lane wrote:
Amit Kapila amit.kap...@huawei.com writes:
On Saturday, December 01, 2012 1:30 AM Tom Lane wrote:
which cannot work if persistent could be a var_name, because bison
has to decide whether to reduce opt_persistent to PERSISTENT or empty
before it can see if there's anything after the var_name. So the fix
is to not use an opt_persistent production, but spell out both
alternatives:
RESET var_name
RESET PERSISTENT var_name
Now bison doesn't have to choose what to reduce until it can see the end
of the statement; that is, once it's scanned RESET and PERSISTENT, the
choice of whether to treat PERSISTENT as a var_name can be conditional
on whether the next token is a name or EOL.
With the above suggestion also, it's not able to resolve shift/reduce
errors.
However I have tried with below changes in gram.y, it works after below
change.
%leftPERSISTENT
VariableResetStmt:
RESET opt_persistent reset_common
{
VariableSetStmt *n = $3;
n-is_persistent = $2;
$$ = (Node *) n;
}
;
opt_persistent: PERSISTENT{ $$ = TRUE; }
| /*EMPTY*/%prec Op{ $$ = FALSE; }
;
I am not sure if there are any problems with above change.
We usually try to avoid operator precedence declarations. They
sometimes have unforeseen consequences.
Found one difference with the change is, the command reset persistent
execution results in different errors with/without change.
without change:
unrecognized configuration parameter persistent.
with change:
syntax error at or near ;
...but this in itself doesn't seem like a problem.
--
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] [v9.3] Row-Level Security
On Sun, Nov 25, 2012 at 03:20:28PM +0100, Kohei KaiGai wrote: However, UPDATE / DELETE support is not perfect right now. In case when we try to update / delete a table with inherited children and RETURNING clause was added, is loses right references to the pseudo columns, even though it works fine without inherited children. The attached patch fixed this known problem. This patch no longer applies to git master. Any chance of a rebase? Also, might this approach work for the catalog? The use case I have in mind is multi-tenancy, although one can imagine organizations where internal access controls might be required on it, too. Cheers, David. -- David Fetter da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- 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] Review: Extra Daemons / bgworker
On 3 December 2012 15:17, Markus Wanner mar...@bluegap.ch wrote: Just like av_launcher does it now: set a flag in shared memory and signal the postmaster (PMSIGNAL_START_AUTOVAC_WORKER). I'm not sure how this works. What process is in charge of setting such a flag? The only process that currently starts background workers ... ehm ... autovacuum workers is the autovacuum launcher. It uses the above Postmaster Signal in autovacuum.c:do_start_autovacuum_worker() to have the postmaster launch bg/autovac workers on demand. My understanding was that the patch keep autovac workers and background workers separate at this point. Is there anything to be gained *now* from merging those two concepts? I saw that as refactoring that can occur once we are happy it should take place, but isn't necessary. -- Simon Riggs 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] Proposal for Allow postgresql.conf values to be changed via SQL
Robert Haas robertmh...@gmail.com writes: On Sat, Dec 1, 2012 at 11:45 AM, Tom Lane t...@sss.pgh.pa.us wrote: But even if we can't make that work, it's not grounds for reserving PERSISTENT. Instead I'd be inclined to forget about RESET PERSISTENT syntax and use, say, SET PERSISTENT var_name TO DEFAULT to mean that. (BTW, I wonder what behavior that syntax has now in your patch.) In fact, rereading this, I wonder why you think RESET PERSISTENT is a good idea even if there were no bison issues with it. We don't write RESET LOCAL or RESET SESSION, so it seems asymmetric to have RESET PERSISTENT. I think this feature is more analagous to ALTER DATABASE .. SET or ALTER ROLE .. SET. Which is, incidentally, another reason I don't like SET PERSISTENT as a proposed syntax. But even if we stick with that syntax, it feels weird to have an SQL command to put a line into postgresql.conf.auto and no syntax to take it back out again. Neither of you have responded to the point about what SET PERSISTENT var_name TO DEFAULT will do, and whether it is or should be different from RESET PERSISTENT, and if not why we should put the latter into the grammar as well. 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] Proposal for Allow postgresql.conf values to be changed via SQL
Robert Haas robertmh...@gmail.com writes:
On Mon, Dec 3, 2012 at 6:38 AM, Amit Kapila amit.kap...@huawei.com wrote:
opt_persistent: PERSISTENT{ $$ = TRUE; }
| /*EMPTY*/%prec Op{ $$ = FALSE; }
;
I am not sure if there are any problems with above change.
We usually try to avoid operator precedence declarations. They
sometimes have unforeseen consequences.
Yes. This is not an improvement over factoring out opt_persistent as
I recommended previously.
Found one difference with the change is, the command reset persistent
execution results in different errors with/without change.
without change:
unrecognized configuration parameter persistent.
with change:
syntax error at or near ;
...but this in itself doesn't seem like a problem.
Indeed, this demonstrates why kluging the behavior this way isn't a good
solution. If PERSISTENT is an unreserved word, then you *should* get
the former error, because it's a perfectly valid interpretation of the
command. If you get the latter then PERSISTENT is not acting like an
unreserved word.
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] [v9.3] Row-Level Security
2012/12/3 David Fetter da...@fetter.org: On Sun, Nov 25, 2012 at 03:20:28PM +0100, Kohei KaiGai wrote: However, UPDATE / DELETE support is not perfect right now. In case when we try to update / delete a table with inherited children and RETURNING clause was added, is loses right references to the pseudo columns, even though it works fine without inherited children. The attached patch fixed this known problem. This patch no longer applies to git master. Any chance of a rebase? OK, I'll rebese it. Also, might this approach work for the catalog? The use case I have in mind is multi-tenancy, although one can imagine organizations where internal access controls might be required on it, too. If you intend to control behavior of DDL commands that internally takes access towards system catalog, RLS feature is not helpful. Please use sepgsql instead. :-) If you intend to control DML commands towards system catalogs, here is nothing special, so I expect it works as we are doing at user tables. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] Proposal for Allow postgresql.conf values to be changed via SQL
On 12/03/2012 10:32 AM, Tom Lane wrote:
Robert Haas robertmh...@gmail.com writes:
On Mon, Dec 3, 2012 at 6:38 AM, Amit Kapila amit.kap...@huawei.com wrote:
opt_persistent: PERSISTENT{ $$ = TRUE; }
| /*EMPTY*/%prec Op{ $$ = FALSE; }
;
I am not sure if there are any problems with above change.
We usually try to avoid operator precedence declarations. They
sometimes have unforeseen consequences.
Yes. This is not an improvement over factoring out opt_persistent as
I recommended previously.
This is by no means the first time this has come up. See
http://wiki.postgresql.org/wiki/Fixing_shift/reduce_conflicts_in_Bison
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.3] Row-Level Security
On 3 December 2012 15:36, Kohei KaiGai kai...@kaigai.gr.jp wrote: 2012/12/3 David Fetter da...@fetter.org: On Sun, Nov 25, 2012 at 03:20:28PM +0100, Kohei KaiGai wrote: However, UPDATE / DELETE support is not perfect right now. In case when we try to update / delete a table with inherited children and RETURNING clause was added, is loses right references to the pseudo columns, even though it works fine without inherited children. The attached patch fixed this known problem. This patch no longer applies to git master. Any chance of a rebase? OK, I'll rebese it. No chunk failures, its just fuzz. -- Simon Riggs 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] Review: Extra Daemons / bgworker
Markus Wanner wrote: On 12/03/2012 03:28 PM, Alvaro Herrera wrote: Just like av_launcher does it now: set a flag in shared memory and signal the postmaster (PMSIGNAL_START_AUTOVAC_WORKER). I'm not sure how this works. What process is in charge of setting such a flag? The only process that currently starts background workers ... ehm ... autovacuum workers is the autovacuum launcher. It uses the above Postmaster Signal in autovacuum.c:do_start_autovacuum_worker() to have the postmaster launch bg/autovac workers on demand. Oh, I understand that. My question was more about what mechanism are you envisioning for new bgworkers. What process would be in charge of sending such signals? Would it be a persistent bgworker which would decide to start up other bgworkers based on external conditions such as timing? And how would postmaster know which bgworker to start -- would it use the bgworker's name to find it in the registered workers list? The trouble with the above rough sketch is how does the coordinating bgworker communicate with postmaster. Autovacuum has a very, um, peculiar mechanism to make this work: avlauncher sends a signal (which has a hardcoded-in-core signal number) and postmaster knows to start a generic avworker; previously avlauncher has set things up in shared memory, and the generic avworker knows where to look to morph into the specific bgworker required. So postmaster never really looks at shared memory other than the signal number (which is the only use of shmem in postmaster that's acceptable, because it's been carefully coded to be robust). This doesn't work for generic modules because we don't have a hardcoded signal number; if two modules wanted to start up generic bgworkers, how would postmaster know which worker-main function to call? Now, maybe we can make this work in some robust fashion (robust meaning we don't put postmaster at risk, which is of utmost importance; and this in turn means don't trust anything in shared memory.) I don't say it's impossible; only that it needs some more thought and careful design. As posted, the feature is already useful and it'd be good to have it committed soon so that others can experiment with whatever sample bgworkers they see fit. That will give us more insight on other API refinements we might need. I'm going to disappear on paternity leave, most likely later this week, or early next week; I would like to commit this patch before that. When I'm back we can discuss other improvements. That will give us several weeks before the end of the 9.3 development period to get these in. Of course, other committers are welcome to improve the code in my absence. -- Álvaro Herrerahttp://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] Review: Extra Daemons / bgworker
Simon Riggs wrote: On 3 December 2012 15:17, Markus Wanner mar...@bluegap.ch wrote: The only process that currently starts background workers ... ehm ... autovacuum workers is the autovacuum launcher. It uses the above Postmaster Signal in autovacuum.c:do_start_autovacuum_worker() to have the postmaster launch bg/autovac workers on demand. My understanding was that the patch keep autovac workers and background workers separate at this point. That is correct. Is there anything to be gained *now* from merging those two concepts? I saw that as refactoring that can occur once we are happy it should take place, but isn't necessary. IMO it's a net loss in robustness of the autovac implementation. -- Álvaro Herrerahttp://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] Review: Extra Daemons / bgworker
On 12/03/2012 04:27 PM, Simon Riggs wrote: My understanding was that the patch keep autovac workers and background workers separate at this point. I agree to that, for this patch. However, that might not keep me from trying to (re-)sumbit some of the bgworker patches in my queue. :-) Regards Markus Wanner -- 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] Review: Extra Daemons / bgworker
On 12/03/2012 04:44 PM, Alvaro Herrera wrote: Simon Riggs wrote: Is there anything to be gained *now* from merging those two concepts? I saw that as refactoring that can occur once we are happy it should take place, but isn't necessary. IMO it's a net loss in robustness of the autovac implementation. Agreed. That's only one aspect of it, though. From the other point of view, it would be a proof of stability for the bgworker implementation if autovacuum worked on top of it. Regards Markus Wanner -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.3] Row-Level Security
On Mon, Dec 03, 2012 at 03:41:47PM +, Simon Riggs wrote: On 3 December 2012 15:36, Kohei KaiGai kai...@kaigai.gr.jp wrote: 2012/12/3 David Fetter da...@fetter.org: On Sun, Nov 25, 2012 at 03:20:28PM +0100, Kohei KaiGai wrote: However, UPDATE / DELETE support is not perfect right now. In case when we try to update / delete a table with inherited children and RETURNING clause was added, is loses right references to the pseudo columns, even though it works fine without inherited children. The attached patch fixed this known problem. This patch no longer applies to git master. Any chance of a rebase? OK, I'll rebese it. No chunk failures, its just fuzz. I must have done something wrong. I downloaded the patch from the web email archives, then ran git apply on it, and got this: $ git apply ../pgsql-v9.3-row-level-security.rw.v7.patch ../pgsql-v9.3-row-level-security.rw.v7.patch:806: space before tab in indent. * row-level security policy ../pgsql-v9.3-row-level-security.rw.v7.patch:1909: trailing whitespace. * would be given. Because the sub-query has security barrier flag, ../pgsql-v9.3-row-level-security.rw.v7.patch:2886: trailing whitespace. did | cid | dlevel | dauthor |dtitle ../pgsql-v9.3-row-level-security.rw.v7.patch:2899: trailing whitespace. cid | did | dlevel | dauthor |dtitle | cname ../pgsql-v9.3-row-level-security.rw.v7.patch:2918: trailing whitespace. did | cid | dlevel | dauthor |dtitle error: patch failed: src/backend/commands/copy.c:34 error: src/backend/commands/copy.c: patch does not apply What did I do wrong here? Cheers, David. -- David Fetter da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- 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] Review: Extra Daemons / bgworker
Alvaro, in general, please keep in mind that there are two aspects that I tend to mix and confuse: one is what's implemented and working for Postgres-R. The other this is how I envision (parts of it) to be merged back into Postgres, itself. Sorry if that causes confusion. On 12/03/2012 04:43 PM, Alvaro Herrera wrote: Oh, I understand that. My question was more about what mechanism are you envisioning for new bgworkers. What process would be in charge of sending such signals? Would it be a persistent bgworker which would decide to start up other bgworkers based on external conditions such as timing? And how would postmaster know which bgworker to start -- would it use the bgworker's name to find it in the registered workers list? Well, in the Postgres-R case, I've extended the autovacuum launcher to a more general purpose job scheduler, named coordinator. Lacking your bgworker patch, it is the only process that is able to launch background workers. Your work now allows extensions to register background workers. If I merge the two concepts, I can easily imagine allowing other (bgworker) processes to launch bgworkers. Another thing I can imagine is turning that coordinator into something that can schedule and trigger jobs registered by extensions (or even user defined ones). Think: cron daemon for SQL jobs in the background. (After all, that's pretty close to what the autovacuum launcher does, already.) The trouble with the above rough sketch is how does the coordinating bgworker communicate with postmaster. I know. I've gone through that pain. In Postgres-R, I've solved this with imessages (which was the primary reason for rejection of the bgworker patches back in 2010). The postmaster only needs to starts *a* background worker process. That process itself then has to figure out (by checking its imessage queue) what job it needs to perform. Or if you think in terms of bgworker types: what type of bgworker it needs to be. Autovacuum has a very, um, peculiar mechanism to make this work: avlauncher sends a signal (which has a hardcoded-in-core signal number) and postmaster knows to start a generic avworker; previously avlauncher has set things up in shared memory, and the generic avworker knows where to look to morph into the specific bgworker required. So postmaster never really looks at shared memory other than the signal number (which is the only use of shmem in postmaster that's acceptable, because it's been carefully coded to be robust). In Postgres-R, I've extended exactly that mechanism to work for other jobs that autovacuum. This doesn't work for generic modules because we don't have a hardcoded signal number; if two modules wanted to start up generic bgworkers, how would postmaster know which worker-main function to call? You've just described above how this works for autovacuum: the postmaster doesn't *need* to know. Leave it to the bgworker to determine what kind of task it needs to perform. As posted, the feature is already useful and it'd be good to have it committed soon so that others can experiment with whatever sample bgworkers they see fit. That will give us more insight on other API refinements we might need. I completely agree. I didn't ever intend to provide an alternative patch or hold you back. (Except for the extra daemon issue, where we disagree, but that's not a reason to keep this feature back). So please, go ahead and commit this feature (once the issues I've mentioned in the review are fixed). Please consider all of these plans or ideas in here (or in Postgres-R) as extending on your work, rather than competing against. I'm going to disappear on paternity leave, most likely later this week, or early next week; I would like to commit this patch before that. When I'm back we can discuss other improvements. That will give us several weeks before the end of the 9.3 development period to get these in. Of course, other committers are welcome to improve the code in my absence. Okay, thanks for sharing that. I'd certainly appreciate your inputs on further refinements for bgworkers and/or autovacuum. Regards Markus Wanner -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch for removng unused targets
Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes: Sorry for the delay. I've reviewed the patch. It was applied successfully, and it worked well for tests I did including the example you showed. I think it's worth the work, but I'm not sure you go about it in the right way. (I feel the patch decreases code readability more than it gives an advantage.) One thought here is that I don't particularly like adding a field like resorderbyonly to TargetEntry in the first place. That makes this optimization the business of the parser, which it should not be; and furthermore makes it incumbent on the rewriter, as well as anything else that manipulates parsetrees, to maintain the flag correctly while rearranging queries. It would be better if this were strictly the business of the planner. But having said that, I'm wondering (without having read the patch) why you need anything more than the existing resjunk field. 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] Materialized views WIP patch
Marko Tiikkaja wrote: Kevin Grittner kgri...@mail.com wrote: Marko Tiikkaja wrote: T2 sees an empty table As far as I know you are the first to notice this behavior. Thanks for pointing it out. I will take a look at the issue; I don't know whether it's something small I can address in this CF or whether it will need to be in the next CF, but I will fix it. Any news on this front? On a preliminary look, I think that it's because when the new heap is visible, it has been populated with rows containing an xmin too new to be visible to the transaction which was blocked. I'll see if I can't populate the new heap with tuples which are already frozen and hinted, which will not only fix this bug, but improve performance. I'll get back when I manage to get a better grasp of the code. The code looks relatively straightforward and good to my eyes. It passes my testing and looks to be changing all the necessary parts of the code. Thanks. I'll put together a new version of the patch based on feedback so far. I need to finish my testing of the fklocks patch and post a review first, so it will be a few days. Keep in mind that the current behavior of behaving like a regular view when the contents are invalid is not what I had in mind, that was an accidental effect of commenting out the body of the ExecCheckRelationsValid() function right before posting the patch because I noticed a regression. When I noticed current behavior, it struck me that someone might prefer it to the intended behavior of showing an error like this: ereport(ERROR, (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), errmsg(materialized view \%s\ has not been populated, get_rel_name(rte-relid)), errhint(Use the LOAD MATERIALIZED VIEW command.))); I mention it in case someone wants to argue for silently behaving as a regular view when the MV is not populated. FWIW, I'd prefer an error in this case, but I don't feel strongly about it. Me, too. Since nobody else has spoken up, that's what I'll do. I haven't heard anyone argue for keeping temporary materialized views, so those will be dropped from the next version of the patch. It sounds like most people prefer REFRESH to LOAD. If there is no further discussion of that, I will make that change in the next version of the patch; so if you want to argue against adding a new unreserved keyword for that, now is the time to say so. Also, should it be just?: REFRESH matview_name; or?: REFRESH MATERIALIZED VIEW matview_name; I will look at heap_inplace_update() where appropriate for the pg_class changes. On the issue of UHLOGGED MVs, I think it's pretty clear that the right way to handle this is to have something in the init fork of an unlogged MV that indicates that it is in an invalid state, which is checked when the MV is opened. I'm not sure of the best way to store that, but my first instinct would be to put it inot the special space of the initial page, which would then be removed after forcing the relisvalid flag to false. That feels pretty kludgy, but its the best idea I've had so far. Anyone else want to suggest a better way? I will add regression tests in the next version of the patch. While I agree that tracking one or more timestamps related to MV freshness is a good idea, that seems like material for a follow-on patch. I don't agree that never having been loaded is a form of stale, in spite of multiple people whom I hold in high regard expressing that opinion, so barring strong opposition I intend to keep the relisvalid column. I just don't feel comfortable about the fundamental correctness of the feature without it. If I'm reading things correctly, so far the opposition has been based on being lukewarm on the value of the column and wanting other features which might be harder with this column (i.e., UNLOGGED) or which are seen as alternatives to this column (i.e., a freshness timestamp) rather than actual opposition to throwing an error on an attempt to use an MV which has not been loaded with data. I will fix the commented-out test of relisvalid. I still haven't quite figured out what to do about the case Thom uncovered where the MV's SELECT was just selecting a literal. I haven't established what the most general case of that is, nor what a sensible behavior is. Any opinions or insights welcome. I didn't see any rebuttal to Tom's concerns about adding asymetrical COPY support, so I will leave that part as is. I will try to fill out the dependencies in pg_dump so that MVs which depend on other MVs will be dumped in the right order. I think that will probably be enough for the next version. -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [v9.3] Row-Level Security
David Fetter escribió: On Mon, Dec 03, 2012 at 03:41:47PM +, Simon Riggs wrote: On 3 December 2012 15:36, Kohei KaiGai kai...@kaigai.gr.jp wrote: 2012/12/3 David Fetter da...@fetter.org: On Sun, Nov 25, 2012 at 03:20:28PM +0100, Kohei KaiGai wrote: However, UPDATE / DELETE support is not perfect right now. In case when we try to update / delete a table with inherited children and RETURNING clause was added, is loses right references to the pseudo columns, even though it works fine without inherited children. The attached patch fixed this known problem. This patch no longer applies to git master. Any chance of a rebase? OK, I'll rebese it. No chunk failures, its just fuzz. I must have done something wrong. I downloaded the patch from the web email archives, then ran git apply on it, and got this: git apply is much stricter than other tools; if the patch requires a merge, it doesn't try. Try applying it with patch -p1 /path/to/patch (this is what I always use) -- Álvaro Herrerahttp://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] [v9.3] Row-Level Security
On 15 November 2012 21:07, Kohei KaiGai kai...@kaigai.gr.jp wrote: The attached patch is a revised version of row-level security feature. I've got time to review this patch, so I've added myself as a CF reviewer. Definitely looks very interesting, well done for getting it this far along. -- Simon Riggs 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] Tablespaces in the data directory
On Dec 3, 2012 2:55 AM, Andrew Dunstan and...@dunslane.net wrote: On 12/02/2012 07:50 PM, Magnus Hagander wrote: On Sat, Dec 1, 2012 at 6:56 PM, Tom Lane t...@sss.pgh.pa.us wrote: Magnus Hagander mag...@hagander.net writes: Someone just reported a problem when they had created a new tablespace inside the old data directory. I'm sure there can be other issues caused by this as well, but this is mainly a confusing scenario for people now. As there isn't (as far as I know at least) any actual *point* in creating a tablespace inside the main data directory, should we perhaps disallow this in CREATE TABLESPACE? Or at least throw a WARNING if one does it? It could be pretty hard to detect that in general (think symlinks and such). I guess if we're just trying to print a helpful warning, we don't have to worry about extreme corner cases. But what exactly do you have in mind --- complain about any relative path? Complain about absolute paths that have a prefix matching the DataDir? Oh, I hadn't thought quite so far as the implementation :) Was looking to see if there were going to be some major objections before I even started thinking about that. But for the implementation, I'd say any absolute path that have a prefix matching DataDir. Tablespaces cannot be created using relative paths, so we don't have to deal with that. I have been known to symlink a tablespace on a replica back to a directory in the datadir, while on the primary it points elsewhere. What exactly is the problem? That wouldn't be affected by this though, since it would only warn at create tablespace. I'd still consider it a bad idea in general to do that, since you're basically messing with the internal structure of the data directory. Why not just link it to some place outside the data directory? One obvious problem with it atm is that pg_basebackup breaks, in that it backs up your data twice, and throws warnings about things that aren't links if you actually out it inside pg_tblspc. /Magnus
[HACKERS] Visibility map page pinned for too long ?
I was looking at the code when the following tiny bit caught my attention.
In vacuumlazy.c, we release the pin on the final VM page at line number 972.
954 if (vacrelstats-num_dead_tuples 0)
955 {
956 /* Log cleanup info before we touch indexes */
957 vacuum_log_cleanup_info(onerel, vacrelstats);
958
959 /* Remove index entries */
960 for (i = 0; i nindexes; i++)
961 lazy_vacuum_index(Irel[i],
962 indstats[i],
963 vacrelstats);
964 /* Remove tuples from heap */
965 lazy_vacuum_heap(onerel, vacrelstats);
966 vacrelstats-num_index_scans++;
967 }
968
969 /* Release the pin on the visibility map page */
970 if (BufferIsValid(vmbuffer))
971 {
972 ReleaseBuffer(vmbuffer);
973 vmbuffer = InvalidBuffer;
974 }
So we are holding the pin right through the index vacuuming and the second
pass over the heap; both can take a very long time. We can and should
really be releasing the pin *before* those steps. In fact, it would be
appropriate to do it right after the preceding big for-loop.
While it may or may not matter from the performance or correctness
perspective, I think we should fix that.
Thanks,
Pavan
--
Pavan Deolasee
http://www.linkedin.com/in/pavandeolasee
Re: [HACKERS] Review: create extension default_full_version
Hi, Thanks for your very good review! Ibrar Ahmed ibrar.ah...@gmail.com writes: I looked at the discussion for this patch and the patch itself. Here are my comments and observations about the patch. What I got from the discussion is that the patch tries to implement a mechanism to install extension from series of SQL scripts from base/full version e.g. if a user wants to create an extension 1.1, system should run v1.0 script followed by 1.0--1.1 script. In that case we need to know about the base or full version which in the above case is v1.0. So the patch added a defualt_full_version option in extension control file. Exactly, that was an idea from Robert and I implemented it quite quickly. Too quickly as we can see from your testing report. Here are my comments about the patch * Note: Patch does not apply cleanly on latest code base. You probably need to re-base the code Done. The thing is that meanwhile another solution to the main problem has been found: drop support for installing hstore 1.0. Attached patch fixes the problem by reinstalling hstore--1.0.sql and re-enabling this version, and removing the hstore--1.1.sql file now that it's enough to just have hstore--1.0--1.1.sql to install directly (and by default) the newer version. I think we will have to decide about taking only the mechanism or both the mechanism and the actual change for the hstore contrib. * This is a user visible change so documentation change is required here. Added coverage of the new parameter. * Also, You need to update the comment, because this code is now handling default_full_version as well. /* * Determine the (unpackaged) version to update from, if any, and then * figure out what sequence of update scripts we need to apply. */ if ((d_old_version d_old_version-arg) || pcontrol-default_full_version) Done. I also fixed the bugs you reported here. Here's an edited version of the new (fixed) output: dim=# set client_min_messages to debug1; dim=# create extension hstore version '1.0'; DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--1.0.sql' WARNING: = is deprecated as an operator name CREATE EXTENSION dim=# create extension hstore version '1.1'; DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--1.0.sql' WARNING: = is deprecated as an operator name DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--1.0--1.1.sql' CREATE EXTENSION dim=# create extension hstore; DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--1.0.sql' WARNING: = is deprecated as an operator name DETAIL: This name may be disallowed altogether in future versions of PostgreSQL. DEBUG: execute_extension_script: '/Users/dim/pgsql/ddl/share/extension/hstore--1.0--1.1.sql' CREATE EXTENSION postgres=# CREATE EXTENSION hstore version '1.3' from '1.0'; WARNING: /usr/local/pgsql/share/extension/hstore--1.0--1.1.sql WARNING: /usr/local/pgsql/share/extension/hstore--1.1--1.2.sql WARNING: /usr/local/pgsql/share/extension/hstore--1.2--1.3.sql CREATE EXTENSION I liked your idea of extending the reporting about what files are used, but of course we can't keep that at the WARNING level, so I made that logging DEBUG1 in the attached patch. postgres=# CREATE EXTENSION hstore version '1.3' from '1.0'; Please try that case again, I believe it's fixed in the attached. - hstore regression is also failing. That's because it doesn't cope anymore with the operator = warning, and I left it this way because we have to decide about shipping hstore 1.0 once we have this patch in. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support *** a/contrib/hstore/Makefile --- b/contrib/hstore/Makefile *** *** 5,11 OBJS = hstore_io.o hstore_op.o hstore_gist.o hstore_gin.o hstore_compat.o \ crc32.o EXTENSION = hstore ! DATA = hstore--1.1.sql hstore--1.0--1.1.sql hstore--unpackaged--1.0.sql REGRESS = hstore --- 5,11 crc32.o EXTENSION = hstore ! DATA = hstore--1.0.sql hstore--1.0--1.1.sql hstore--unpackaged--1.0.sql REGRESS = hstore *** /dev/null --- b/contrib/hstore/hstore--1.0.sql *** *** 0 --- 1,530 + /* contrib/hstore/hstore--1.0.sql */ + + -- complain if script is sourced in psql, rather than via CREATE EXTENSION + \echo Use CREATE EXTENSION hstore to load this file. \quit + + CREATE TYPE hstore; + + CREATE FUNCTION hstore_in(cstring) + RETURNS hstore + AS 'MODULE_PATHNAME' + LANGUAGE C STRICT IMMUTABLE; + + CREATE FUNCTION hstore_out(hstore) + RETURNS cstring + AS 'MODULE_PATHNAME' + LANGUAGE C STRICT IMMUTABLE; + + CREATE FUNCTION hstore_recv(internal) + RETURNS hstore + AS 'MODULE_PATHNAME' + LANGUAGE C STRICT IMMUTABLE; + + CREATE FUNCTION hstore_send(hstore) + RETURNS bytea + AS 'MODULE_PATHNAME' + LANGUAGE C STRICT IMMUTABLE; + + CREATE TYPE
Re: [HACKERS] Tablespaces in the data directory
On 12/03/2012 12:33 PM, Magnus Hagander wrote: On Dec 3, 2012 2:55 AM, Andrew Dunstan and...@dunslane.net mailto:and...@dunslane.net wrote: On 12/02/2012 07:50 PM, Magnus Hagander wrote: On Sat, Dec 1, 2012 at 6:56 PM, Tom Lane t...@sss.pgh.pa.us mailto:t...@sss.pgh.pa.us wrote: Magnus Hagander mag...@hagander.net mailto:mag...@hagander.net writes: Someone just reported a problem when they had created a new tablespace inside the old data directory. I'm sure there can be other issues caused by this as well, but this is mainly a confusing scenario for people now. As there isn't (as far as I know at least) any actual *point* in creating a tablespace inside the main data directory, should we perhaps disallow this in CREATE TABLESPACE? Or at least throw a WARNING if one does it? It could be pretty hard to detect that in general (think symlinks and such). I guess if we're just trying to print a helpful warning, we don't have to worry about extreme corner cases. But what exactly do you have in mind --- complain about any relative path? Complain about absolute paths that have a prefix matching the DataDir? Oh, I hadn't thought quite so far as the implementation :) Was looking to see if there were going to be some major objections before I even started thinking about that. But for the implementation, I'd say any absolute path that have a prefix matching DataDir. Tablespaces cannot be created using relative paths, so we don't have to deal with that. I have been known to symlink a tablespace on a replica back to a directory in the datadir, while on the primary it points elsewhere. What exactly is the problem? That wouldn't be affected by this though, since it would only warn at create tablespace. I'd still consider it a bad idea in general to do that, since you're basically messing with the internal structure of the data directory. Why not just link it to some place outside the data directory? One obvious problem with it atm is that pg_basebackup breaks, in that it backs up your data twice, and throws warnings about things that aren't links if you actually out it inside pg_tblspc. Well, when I last did it I don't think there was such a thing as pg_basebackup :-) I think it would be reasonable for it to complain if it came across a PG_VERSION file in an unexpected location. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] visibilitymap_count() at the end of vacuum
I wonder if we really need to make another pass over the entire visibility map to count the number of all-visible pages at the end of the vacuum. The code that I'm looking at is in src/backend/commands/vacuumlazy.c: 247 new_rel_allvisible = visibilitymap_count(onerel); 248 if (new_rel_allvisible new_rel_pages) 249 new_rel_allvisible = new_rel_pages; We would have just scanned every bit of the visibility map and can remember information about the number of all-visible pages in vacrelstats, just like many other statistical information that we track and update the end of the vacuum. Sure, there might be some more updates to the VM, especially a few bits may get cleared while we are vacuuming the table, but that can happen even while we are recounting at the end. AFAICS we can deal with that much staleness of the data. If we agree that this is worth improving, I can write a patch to do so. Thanks, Pavan -- Pavan Deolasee http://www.linkedin.com/in/pavandeolasee
Re: [HACKERS] visibilitymap_count() at the end of vacuum
Hi, On 2012-12-03 23:44:36 +0530, Pavan Deolasee wrote: I wonder if we really need to make another pass over the entire visibility map to count the number of all-visible pages at the end of the vacuum. The code that I'm looking at is in src/backend/commands/vacuumlazy.c: 247 new_rel_allvisible = visibilitymap_count(onerel); 248 if (new_rel_allvisible new_rel_pages) 249 new_rel_allvisible = new_rel_pages; We would have just scanned every bit of the visibility map and can remember information about the number of all-visible pages in vacrelstats, just like many other statistical information that we track and update the end of the vacuum. Sure, there might be some more updates to the VM, especially a few bits may get cleared while we are vacuuming the table, but that can happen even while we are recounting at the end. AFAICS we can deal with that much staleness of the data. A full-table vacuum can take a *long* (as in days) time, so I think recounting makes sense. And normally the cost is pretty small, so I don't see a problem in this. Why change it? Andres -- 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] visibilitymap_count() at the end of vacuum
On Mon, Dec 3, 2012 at 11:50 PM, Andres Freund and...@2ndquadrant.comwrote: A full-table vacuum can take a *long* (as in days) time, so I think recounting makes sense. And normally the cost is pretty small, so I don't see a problem in this. Well, may be the cost is low. But it can still run into several hundred or thousand pages that are read into the buffer pool again. If there is indeed too much churn happening, an ANALYZE will kick in which will count the bits anyway or the next VACUUM will correct it (though it may become out dated again) Why change it? Why not ? As I said, we would have just counted the bits and will be doing it again which looks overkill unless someone really wants to argue that the staleness of the data is going to cause the planner to really start producing way too bad plans. But note that we have lived with the staleness of reltuples and relpages for so long. So I don't see why relallvisible needs any special treatment, just because its relatively easy to recount them. Thanks, Pavan -- Pavan Deolasee http://www.linkedin.com/in/pavandeolasee
Re: [HACKERS] Visibility map page pinned for too long ?
On 3 December 2012 17:37, Pavan Deolasee pavan.deola...@gmail.com wrote:
I was looking at the code when the following tiny bit caught my attention.
In vacuumlazy.c, we release the pin on the final VM page at line number 972.
954 if (vacrelstats-num_dead_tuples 0)
955 {
956 /* Log cleanup info before we touch indexes */
957 vacuum_log_cleanup_info(onerel, vacrelstats);
958
959 /* Remove index entries */
960 for (i = 0; i nindexes; i++)
961 lazy_vacuum_index(Irel[i],
962 indstats[i],
963 vacrelstats);
964 /* Remove tuples from heap */
965 lazy_vacuum_heap(onerel, vacrelstats);
966 vacrelstats-num_index_scans++;
967 }
968
969 /* Release the pin on the visibility map page */
970 if (BufferIsValid(vmbuffer))
971 {
972 ReleaseBuffer(vmbuffer);
973 vmbuffer = InvalidBuffer;
974 }
So we are holding the pin right through the index vacuuming and the second
pass over the heap; both can take a very long time. We can and should really
be releasing the pin *before* those steps. In fact, it would be appropriate
to do it right after the preceding big for-loop.
While it may or may not matter from the performance or correctness
perspective, I think we should fix that.
Yes, its a clear bug. Patched.
--
Simon Riggs 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] visibilitymap_count() at the end of vacuum
On 3 December 2012 18:20, Andres Freund and...@2ndquadrant.com wrote: Hi, On 2012-12-03 23:44:36 +0530, Pavan Deolasee wrote: I wonder if we really need to make another pass over the entire visibility map to count the number of all-visible pages at the end of the vacuum. The code that I'm looking at is in src/backend/commands/vacuumlazy.c: 247 new_rel_allvisible = visibilitymap_count(onerel); 248 if (new_rel_allvisible new_rel_pages) 249 new_rel_allvisible = new_rel_pages; We would have just scanned every bit of the visibility map and can remember information about the number of all-visible pages in vacrelstats, just like many other statistical information that we track and update the end of the vacuum. Sure, there might be some more updates to the VM, especially a few bits may get cleared while we are vacuuming the table, but that can happen even while we are recounting at the end. AFAICS we can deal with that much staleness of the data. A full-table vacuum can take a *long* (as in days) time, so I think recounting makes sense. And normally the cost is pretty small, so I don't see a problem in this. Why change it? There's another reason for doing it this way: if VACUUM sets everything as all visible, but during the VACUUM that state is quickly reset by others, it would be a mistake not to allow for that. We want a realistic value not a best possible case. -- Simon Riggs 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] why can't plpgsql return a row-expression?
Hi, Thanks for the review. Please see the updated patch. Hmm. We're running an I/O cast during do_tup_convert() now, and look up the required functions for each tuple. I think if we're going to support this operation at that level, we need to look up the necessary functions at convert_tuples_by_foo level, and then apply unconditionally if they've been set up. Done. TupleConversionMap struct should keep the array of functions oid's that needs to be applied. Though only for those cases where both attribute type id's do not match. This way no unnecessary casting will happen. Also, what are the implicancies for existing users of tupconvert? Do we want to apply casting during ANALYZE for example? AFAICS the patch shouldn't break any case that works today, but I don't see that there has been any analysis of this. I believe this part of the code should not impact existing users of tupconvert. As this part of code is relaxing a restriction upon which an error would have been generated. Though it could have a little impact on performance but should be only for cases where attribute type id's are different and are implicitly cast able. Besides existing users involve tupconvert in case of inheritance. And in that case an exact match is expected. Regards, --Asif return_rowtype.v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Add mode where contrib installcheck runs each module in a separa
Andrew Dunstan and...@dunslane.net writes: On 12/03/2012 01:45 PM, Alvaro Herrera wrote: $ LC_ALL=C make Makefile:15: invalid `override' directive Well, you seem to have more problems than just the database name. I see this too. Given that I want to wrap the back branches in a few minutes, please revert at least in the back branches. I can't wait for you to sort this out. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [COMMITTERS] pgsql: Add mode where contrib installcheck runs each module in a separa
On 12/03/2012 02:46 PM, Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: On 12/03/2012 01:45 PM, Alvaro Herrera wrote: $ LC_ALL=C make Makefile:15: invalid `override' directive Well, you seem to have more problems than just the database name. I see this too. Given that I want to wrap the back branches in a few minutes, please revert at least in the back branches. I can't wait for you to sort this out. OK. I reverted the lot. will do. cheers andrew -- 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] --single-transaction hack to pg_upgrade does not work
On Sat, Dec 1, 2012 at 03:41:15PM -0500, Andrew Dunstan wrote: On 12/01/2012 02:34 PM, Bruce Momjian wrote: On Sat, Dec 1, 2012 at 02:31:03PM -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2012-12-01 12:14:37 -0500, Tom Lane wrote: It could do with some comments ;-) Hehe, yes. Hopefully this version has enough of that. Hm, maybe too many --- I don't really think it's necessary for utility.c to provide a redundant explanation of what's happening. Committed with adjustments --- mainly, the TransactionIdIsCurrentTransactionId test was flat out wrong, because it would accept a parent transaction ID as well as a subcommitted subtransaction ID. We could safely allow the latter, but I don't think it's worth the trouble to add another xact.c test function. Thanks everyone. I can confirm that pg_upgrades make check now passes, so this should green the buildfarm. Again, I aplogize for the fire drill. I've added better logging of pg_upgrade testing to the buildfarm module: https://github.com/PGBuildFarm/client-code/commit/83834cceaea95ba42c03a1079a8c768782e32a6b example is at http://www.pgbuildfarm.org/cgi-bin/show_log.pl?nm=crakedt=2012-12-01%2017%3A44%3A03 This will be in the next buildfarm client release. Wow, that looks great. You even show the last few lines from the log files! -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] visibilitymap_count() at the end of vacuum
On Mon, Dec 3, 2012 at 1:36 PM, Pavan Deolasee pavan.deola...@gmail.com wrote: Well, may be the cost is low. But it can still run into several hundred or thousand pages that are read into the buffer pool again. If there is indeed too much churn happening, an ANALYZE will kick in which will count the bits anyway or the next VACUUM will correct it (though it may become out dated again) Several hundred pages? Each visibility map page covers 512 MB of heap pages. If you read several hundred of them, you're talking about a relation that is over 100GB in size. If you read several thousand, you're over a terabyte. There are probably a few people who have PostgreSQL relations that large, but not many. Also, if someone does have a 100GB relation, rereading 2MB of visibility map pages at the end probably isn't a significant part of the total cost. Even if 99.9% of the relation is all-visible and we skip reading those parts, the visibility map reads will still be only about 2% of the total read activity, and most of the time you won't be that lucky. -- 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] autovacuum truncate exclusive lock round two
Jan Wieck wrote: Attached is a new patch that addresses most of the points raised in discussion before. 1) Most of the configuration variables are derived from deadlock_timeout now. The check for conflicting lock request interval is deadlock_timeout/10, clamped to 10ms. The try to acquire exclusive lock interval is deadlock_timeout/20, also clamped to 10ms. The only GUC variable remaining is autovacuum_truncate_lock_try=2000ms with a range from 0 (just try once) to 2ms. If we're going to keep this GUC, we need docs for it. I'd like to point out that this is a significant change in functionality as without the config option for the check interval, there is no longer any possibility to disable the call to LockHasWaiters() and return to the original (deadlock code kills autovacuum) behavior. Arguably we could simplify the deadlock resolution code a little, but it seems like it is probably safer to leave it as a failsafe, at least for now. I did not touch the part about suppressing the stats and the ANALYZE step of auto vacuum+analyze. The situation is no different today. When the deadlock code kills autovacuum, stats aren't updated either. And this patch is meant to cause autovacuum to finish the truncate in a few minutes or hours, so that the situation fixes itself. Unless someone want to argue for more aggressive updating of the stats, I guess I'll yield to that argument. Besides the documentation, the only thing I could spot on this go-around was that this comment probably no longer has a reason for being since this is no longer conditional and what it's doing is obvious from the code: /* Initialize the starttime if we check for conflicting lock requests */ With docs and dropping the obsolete comment, I think this will be ready. -Kevin -- 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] autovacuum truncate exclusive lock round two
On 12/3/2012 5:42 PM, Kevin Grittner wrote: Jan Wieck wrote: Attached is a new patch that addresses most of the points raised in discussion before. 1) Most of the configuration variables are derived from deadlock_timeout now. The check for conflicting lock request interval is deadlock_timeout/10, clamped to 10ms. The try to acquire exclusive lock interval is deadlock_timeout/20, also clamped to 10ms. The only GUC variable remaining is autovacuum_truncate_lock_try=2000ms with a range from 0 (just try once) to 2ms. If we're going to keep this GUC, we need docs for it. Certainly. But since we're still debating which and how many GUC variables we want, I don't think doc-time has come yet. I'd like to point out that this is a significant change in functionality as without the config option for the check interval, there is no longer any possibility to disable the call to LockHasWaiters() and return to the original (deadlock code kills autovacuum) behavior. Arguably we could simplify the deadlock resolution code a little, but it seems like it is probably safer to leave it as a failsafe, at least for now. Thinking about it, I'm not really happy with removing the autovacuum_truncate_lock_check GUC at all. Fact is that the deadlock detection code and the configuration parameter for it should IMHO have nothing to do with all this in the first place. A properly implemented application does not deadlock. Someone running such a properly implemented application should be able to safely set deadlock_timeout to minutes without the slightest ill side effect, but with the benefit that the deadlock detection code itself does not add to the lock contention. The only reason one cannot do so today is because autovacuum's truncate phase could then freeze the application with an exclusive lock for that long. I believe the check interval needs to be decoupled from the deadlock_timeout again. This will leave us with 2 GUCs at least. Jan -- Anyone who trades liberty for security deserves neither liberty nor security. -- Benjamin Franklin -- 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] visibilitymap_count() at the end of vacuum
On Tue, Dec 4, 2012 at 3:16 AM, Robert Haas robertmh...@gmail.com wrote: Also, if someone does have a 100GB relation, rereading 2MB of visibility map pages at the end probably isn't a significant part of the total cost. Even if 99.9% of the relation is all-visible and we skip reading those parts, the visibility map reads will still be only about 2% of the total read activity, and most of the time you won't be that lucky. Hmm. I fully agree its a very small percentage of the total cost. But I still don't see why it should not be optimised, if possible. Of course, if not recounting at the end will generate bad query plans most of the time for most of the workloads or even a few workloads, then the minuscule cost will pay of. But nobody convincingly argued about that. Even if the current way is the right way, we should probably just add a comment there. I also noticed that we call vacuum_delay_point() after testing every visibility map bit in lazy_scan_heap() which looks overkill, but visibilitymap_count() runs to the end without even a single call to vacuum_delay_point(). Is that intended ? Or should we at least check for interrupts there ? Thanks, Pavan -- Pavan Deolasee http://www.linkedin.com/in/pavandeolasee
[HACKERS] Re: [COMMITTERS] pgsql: Add mode where contrib installcheck runs each module in a separa
On 12/03/2012 04:02 PM, Andrew Dunstan wrote: Looks like undefine is a new feature in gmake 3.82. I can reproduce this on my SL6 box which has 3.81. I'll have to come up with something other than ifdef, like ifneq ($(USE_MODULE_DB),) and use the override in dblink's Makefile to set it to the empty string. Here's a version that seems to work with pre 3.82 versions of gmake. cheers andrew diff --git a/contrib/dblink/Makefile b/contrib/dblink/Makefile index a27db88..32314a0 100644 --- a/contrib/dblink/Makefile +++ b/contrib/dblink/Makefile @@ -11,6 +11,9 @@ DATA = dblink--1.1.sql dblink--1.0--1.1.sql dblink--unpackaged--1.0.sql REGRESS = dblink +# the db name is hard-coded in the tests +override USE_MODULE_DB = + ifdef USE_PGXS PG_CONFIG = pg_config PGXS := $(shell $(PG_CONFIG) --pgxs) diff --git a/src/Makefile.global.in b/src/Makefile.global.in index e10eead..9cc14da 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -428,6 +428,15 @@ submake-libpgport: PL_TESTDB = pl_regression CONTRIB_TESTDB = contrib_regression +ifneq ($(MODULE_big),) + CONTRIB_TESTDB_MODULE = contrib_regression_$(MODULE_big) +else + ifneq ($(MODULES),) +CONTRIB_TESTDB_MODULE = contrib_regression_$(MODULES) + else +CONTRIB_TESTDB_MODULE = contrib_regression + endif +endif ifdef NO_LOCALE NOLOCALE += --no-locale diff --git a/src/makefiles/pgxs.mk b/src/makefiles/pgxs.mk index fd6473f..8acfe05 100644 --- a/src/makefiles/pgxs.mk +++ b/src/makefiles/pgxs.mk @@ -240,7 +240,11 @@ distclean maintainer-clean: clean ifdef REGRESS # Select database to use for running the tests -REGRESS_OPTS += --dbname=$(CONTRIB_TESTDB) +ifneq ($(USE_MODULE_DB),) + REGRESS_OPTS += --dbname=$(CONTRIB_TESTDB_MODULE) +else + REGRESS_OPTS += --dbname=$(CONTRIB_TESTDB) +endif # where to find psql for running the tests PSQLDIR = $(bindir) -- 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] json accessors
Andrew,
What about doing:
json_get(json, json)
returns json
where parameter #2 is a path expressed as JSON? For example,
json_get(personal_profile, '[ {contact_info {phone numbers {cell phones}
} } ]')
... would return whatever was in that heirarchical object, in this case
an array of cell phone numbers.
Or am I just reinventing jsonpath?
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Commits 8de72b and 5457a1 (COPY FREEZE)
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=8de72b66a2edcf12c812de0a73bd50b6b7d81d62 Part of that patch was reverted later: http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=5457a130d3a66db807d1e0ee2b8e829321809b83 I assume that refers to the discussion here: http://archives.postgresql.org/pgsql-hackers/2012-02/msg01177.php But that was quite a while ago, so is there a more recent discussion that prompted this commit now? I am a little confused about the case where setting HEAP_XMIN_COMMITTED when loading the table in the same transaction is wrong. There was some discussion about subtransactions, but those problems only seemed to appear when the CREATE and the INSERT/COPY happened in different subtransactions. So, I guess my question is, why the partial revert? Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER TABLE ... NOREWRITE option
EXPLAIN ALTER TABLE would be the next step I guess. I discovered plenty of magic tricks when working on the rewriting support for that command, and I think exposing them in the EXPLAIN output would go a long way towards reducing some POLA violations. I think this would be cool on its own merits. However, for a very large user group -- users with ORMs which do their own DDL migrations -- we could also use a way to log or WARN for table rewrites. Since the ORMs are not gonna do an EXPLAIN. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.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] ALTER TABLE ... NOREWRITE option
Josh Berkus j...@agliodbs.com writes: EXPLAIN ALTER TABLE would be the next step I guess. I discovered plenty of magic tricks when working on the rewriting support for that command, and I think exposing them in the EXPLAIN output would go a long way towards reducing some POLA violations. I think this would be cool on its own merits. However, for a very large user group -- users with ORMs which do their own DDL migrations -- we could also use a way to log or WARN for table rewrites. Since the ORMs are not gonna do an EXPLAIN. An ORM is also quite unlikely to pay attention to a warning, much less a postmaster log entry ... so your argument seems unfounded from here. 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] json accessors
On 12/03/2012 08:14 PM, Josh Berkus wrote:
Andrew,
What about doing:
json_get(json, json)
returns json
where parameter #2 is a path expressed as JSON? For example,
json_get(personal_profile, '[ {contact_info {phone numbers {cell phones}
} } ]')
... would return whatever was in that heirarchical object, in this case
an array of cell phone numbers.
Or am I just reinventing jsonpath?
Yes, you are, rather. It might be possible to do something like:
json_get(json, variadic text) = json
as long as it doesn't involve any testing beyond field name / array
index equivalence.
cheers
andrew
--
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] Tablespaces in the data directory
On Mon, Dec 3, 2012 at 02:38:20AM -, Greg Sabino Mullane wrote: -BEGIN PGP SIGNED MESSAGE- Hash: RIPEMD160 As there isn't (as far as I know at least) any actual *point* in creating a tablespace inside the main data directory, should we perhaps disallow this in CREATE TABLESPACE? Or at least throw a WARNING if one does it? Sure there is a point - emulating some other system. Could be replication, QA box, disaster recovery, etc. I'd be cool with a warning, but do not think we should disallow it. FYI, someone put their new cluster inside an existing old tablespace, and when they ran the script to delete their old install, their new install was deleted too. My answer was, Don't do that. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal for Allow postgresql.conf values to be changed via SQL
On Monday, December 03, 2012 8:59 PM Tom Lane wrote: Robert Haas robertmh...@gmail.com writes: On Sat, Dec 1, 2012 at 11:45 AM, Tom Lane t...@sss.pgh.pa.us wrote: But even if we can't make that work, it's not grounds for reserving PERSISTENT. Instead I'd be inclined to forget about RESET I think this feature is more analagous to ALTER DATABASE .. SET or ALTER ROLE .. SET. Which is, incidentally, another reason I don't like SET PERSISTENT as a proposed syntax. But even if we stick with that syntax, it feels weird to have an SQL command to put a line into postgresql.conf.auto and no syntax to take it back out again. Neither of you have responded to the point about what SET PERSISTENT var_name TO DEFAULT will do, and whether it is or should be different from RESET PERSISTENT, and if not why we should put the latter into the grammar as well. I have mentioned this in my previous mail but may be it has some problem http://archives.postgresql.org/pgsql-hackers/2012-12/msg00062.php The current behavior is 1. RESET PERSISTENT ... will delete the entry from postgresql.auto.conf 2. SET PERSISTENT... TO DEFAULT will update the entry value in postgresql.auto.conf to default value However we can even change SET PERSISTENT... TO DEFAULT to delete the entry and then we can avoid RESET PERSISTENT ... Opinions? With Regards, Amit Kapila. -- 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] Proposal for Allow postgresql.conf values to be changed via SQL
On Monday, December 03, 2012 8:50 PM Robert Haas wrote: On Sat, Dec 1, 2012 at 11:45 AM, Tom Lane t...@sss.pgh.pa.us wrote: I wrote: But even if we can't make that work, it's not grounds for reserving PERSISTENT. Instead I'd be inclined to forget about RESET PERSISTENT syntax and use, say, SET PERSISTENT var_name TO DEFAULT to mean that. (BTW, I wonder what behavior that syntax has now in your patch.) In fact, rereading this, I wonder why you think RESET PERSISTENT is a good idea even if there were no bison issues with it. We don't write RESET LOCAL or RESET SESSION, so it seems asymmetric to have RESET PERSISTENT. I think this feature is more analagous to ALTER DATABASE .. SET or ALTER ROLE .. SET. Which is, incidentally, another reason I don't like SET PERSISTENT as a proposed syntax. But even if we stick with that syntax, it feels weird to have an SQL command to put a line into postgresql.conf.auto and no syntax to take it back out again. We wouldn't allow a CREATE command without a corresponding DROP operation... Current implementation of SET PERSISTENT... TO DEFAULT will update the entry value in postgresql.auto.conf to default value. How about if make the behavior of SET PERSISTENT... TO DEFAULT such that it will delete the entry in postgresql.auto.conf? With Regards, Amit Kapila. -- 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] ALTER TABLE ... NOREWRITE option
On Mon, Dec 03, 2012 at 11:37:17AM +0100, Dimitri Fontaine wrote: Noah Misch n...@leadboat.com writes: Acquiring the lock could still take an unpredictable amount of time. I think there's a new GUC brewing about setting the lock timeout separately from the statement timeout, right? Yes. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Reducing pg_dumpall errors
I recently applied the attached patch to prevent recreation of the current database user in the dump file when in binary upgrade mode. This was necessary because pg_upgrade will fail on any error from a pg_dumpall restore. It would be nice of we could do the same for non-binary-upgrade pg_dumpall dumps, but I assume we can't because the restore might be performed by a super user who is different from the dump user, right? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + commit db00d837c17cebf3769fd3b6655812e2d3776f5d Author: Bruce Momjian br...@momjian.us Date: Mon Dec 3 19:43:02 2012 -0500 In pg_upgrade, fix bug where no users were dumped in pg_dumpall binary-upgrade mode; instead only skip dumping the current user. This bug was introduced in during the removal of split_old_dump(). Bug discovered during local testing. diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c new file mode 100644 index aa4fcbb..088106f *** a/src/bin/pg_dump/pg_dumpall.c --- b/src/bin/pg_dump/pg_dumpall.c *** dumpRoles(PGconn *conn) *** 642,648 i_rolpassword, i_rolvaliduntil, i_rolreplication, ! i_rolcomment; int i; /* note: rolconfig is dumped later */ --- 642,649 i_rolpassword, i_rolvaliduntil, i_rolreplication, ! i_rolcomment, ! i_is_current_user; int i; /* note: rolconfig is dumped later */ *** dumpRoles(PGconn *conn) *** 652,658 rolcreaterole, rolcreatedb, rolcanlogin, rolconnlimit, rolpassword, rolvaliduntil, rolreplication, ! pg_catalog.shobj_description(oid, 'pg_authid') as rolcomment FROM pg_authid ORDER BY 2); else if (server_version = 80200) --- 653,660 rolcreaterole, rolcreatedb, rolcanlogin, rolconnlimit, rolpassword, rolvaliduntil, rolreplication, ! pg_catalog.shobj_description(oid, 'pg_authid') as rolcomment, ! rolname = current_user AS is_current_user FROM pg_authid ORDER BY 2); else if (server_version = 80200) *** dumpRoles(PGconn *conn) *** 661,667 rolcreaterole, rolcreatedb, rolcanlogin, rolconnlimit, rolpassword, rolvaliduntil, false as rolreplication, ! pg_catalog.shobj_description(oid, 'pg_authid') as rolcomment FROM pg_authid ORDER BY 2); else if (server_version = 80100) --- 663,670 rolcreaterole, rolcreatedb, rolcanlogin, rolconnlimit, rolpassword, rolvaliduntil, false as rolreplication, ! pg_catalog.shobj_description(oid, 'pg_authid') as rolcomment, ! rolname = current_user AS is_current_user FROM pg_authid ORDER BY 2); else if (server_version = 80100) *** dumpRoles(PGconn *conn) *** 670,676 rolcreaterole, rolcreatedb, rolcanlogin, rolconnlimit, rolpassword, rolvaliduntil, false as rolreplication, ! null as rolcomment FROM pg_authid ORDER BY 2); else --- 673,680 rolcreaterole, rolcreatedb, rolcanlogin, rolconnlimit, rolpassword, rolvaliduntil, false as rolreplication, ! null as rolcomment, ! rolname = current_user AS is_current_user FROM pg_authid ORDER BY 2); else *** dumpRoles(PGconn *conn) *** 685,691 passwd as rolpassword, valuntil as rolvaliduntil, false as rolreplication, ! null as rolcomment FROM pg_shadow UNION ALL SELECT 0, groname as rolname, --- 689,696 passwd as rolpassword, valuntil as rolvaliduntil, false as rolreplication, ! null as rolcomment, ! rolname = current_user AS is_current_user FROM pg_shadow UNION ALL SELECT 0, groname as rolname, *** dumpRoles(PGconn *conn) *** 698,704 null::text as rolpassword, null::abstime as rolvaliduntil, false as rolreplication, ! null as rolcomment FROM pg_group WHERE NOT EXISTS (SELECT 1 FROM pg_shadow WHERE usename = groname) --- 703,709 null::text as rolpassword, null::abstime as rolvaliduntil, false as rolreplication, ! null as rolcomment, false FROM pg_group WHERE NOT EXISTS (SELECT 1 FROM pg_shadow WHERE usename = groname) *** dumpRoles(PGconn *conn) *** 718,723 --- 723,729 i_rolvaliduntil = PQfnumber(res, rolvaliduntil); i_rolreplication = PQfnumber(res, rolreplication); i_rolcomment = PQfnumber(res, rolcomment); +
[HACKERS] Forgotten argument description in header of index_create
Hi all, While reading some index-related code, I found that the description of the argument is_internal of index_create in index.c has been forgotten in commit f4c4335. Correction patch attached. Thanks, -- Michael Paquier http://michael.otacoo.com 20121204_index_create_header.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] Tablespaces in the data directory
On Dec 3, 2012, at 12:33, Magnus Hagander wrote: On Dec 3, 2012 2:55 AM, Andrew Dunstan and...@dunslane.net wrote: On 12/02/2012 07:50 PM, Magnus Hagander wrote: On Sat, Dec 1, 2012 at 6:56 PM, Tom Lane t...@sss.pgh.pa.us wrote: Magnus Hagander mag...@hagander.net writes: Someone just reported a problem when they had created a new tablespace inside the old data directory. I'm sure there can be other issues caused by this as well, but this is mainly a confusing scenario for people now. As there isn't (as far as I know at least) any actual *point* in creating a tablespace inside the main data directory, should we perhaps disallow this in CREATE TABLESPACE? Or at least throw a WARNING if one does it? It could be pretty hard to detect that in general (think symlinks and such). I guess if we're just trying to print a helpful warning, we don't have to worry about extreme corner cases. But what exactly do you have in mind --- complain about any relative path? Complain about absolute paths that have a prefix matching the DataDir? Oh, I hadn't thought quite so far as the implementation :) Was looking to see if there were going to be some major objections before I even started thinking about that. But for the implementation, I'd say any absolute path that have a prefix matching DataDir. Tablespaces cannot be created using relative paths, so we don't have to deal with that. I have been known to symlink a tablespace on a replica back to a directory in the datadir, while on the primary it points elsewhere. What exactly is the problem? That wouldn't be affected by this though, since it would only warn at create tablespace. I'd still consider it a bad idea in general to do that, since you're basically messing with the internal structure of the data directory. Why not just link it to some place outside the data directory? One reason is that subsequent copies of the data directory then also includes the tablespace data. Saves one step when setting up a standby. Michael Glaesemann grzm seespotcode net -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Use of fsync; was Re: [HACKERS] Pg_upgrade speed for many tables
Applied. --- On Fri, Nov 30, 2012 at 10:43:29PM -0500, Bruce Momjian wrote: On Mon, Nov 26, 2012 at 02:43:19PM -0500, Bruce Momjian wrote: In any event, I think the documentation should caution that the upgrade should not be deemed to be a success until after a system-wide sync has been done. Even if we use the link rather than copy method, are we sure that that is safe if the directories recording those links have not been fsynced? OK, the above is something I have been thinking about, and obviously you have too. If you change fsync from off to on in a cluster, and restart it, there is no guarantee that the dirty pages you read from the kernel are actually on disk, because Postgres doesn't know they are dirty. They probably will be pushed to disk by the kernel in less than one minute, but still, it doesn't seem reliable. Should this be documented in the fsync section? Again, another reason not to use fsync=off, though your example of the file copy is a good one. As you stated, this is a problem with the file copy/link, independent of how Postgres handles the files. We can tell people to use 'sync' as root on Unix, but what about Windows? I'm pretty sure someone mentioned the way to do that on Windows in this list in the last few months, but I can't seem to find it. I thought it was the initdb fsync thread. Yep, the code is already in initdb to fsync a directory --- we just need a way for pg_upgrade to access it. I have developed the attached patch that does this. It basically adds an --sync-only option to initdb, then turns off all durability in pg_upgrade and has pg_upgrade run initdb --sync-only; this give us another nice speedup! -- SSD -- magnetic --- gitpatchgitpatch 1 11.11 11.11 11.10 11.13 1000 20.57 19.89 20.72 19.30 2000 28.02 25.81 28.50 27.53 4000 42.00 43.59 46.71 46.84 8000 89.66 74.16 89.10 73.67 16000 157.66 135.98 159.97 153.48 32000 316.24 296.90 334.74 308.59 64000 814.97 715.53 797.34 727.94 (I am very happy with these times. Thanks to Jeff Janes for his suggestions.) I have also added documentation to the 'fsync' configuration variable warning about dirty buffers and recommending flushing them to disk before the cluster is crash-recovery safe. I consider this patch ready for 9.3 application (meaning it is not a prototype). -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + diff --git a/contrib/pg_upgrade/pg_upgrade.c b/contrib/pg_upgrade/pg_upgrade.c new file mode 100644 index c12f15b..63df529 *** a/contrib/pg_upgrade/pg_upgrade.c --- b/contrib/pg_upgrade/pg_upgrade.c *** main(int argc, char **argv) *** 150,155 --- 150,161 new_cluster.pgdata); check_ok(); + prep_status(Sync data directory to disk); + exec_prog(UTILITY_LOG_FILE, NULL, true, + \%s/initdb\ --sync-only \%s\, new_cluster.bindir, + new_cluster.pgdata); + check_ok(); + create_script_for_cluster_analyze(analyze_script_file_name); create_script_for_old_cluster_deletion(deletion_script_file_name); diff --git a/contrib/pg_upgrade/server.c b/contrib/pg_upgrade/server.c new file mode 100644 index 49d4c8f..05d8cc0 *** a/contrib/pg_upgrade/server.c --- b/contrib/pg_upgrade/server.c *** start_postmaster(ClusterInfo *cluster) *** 209,217 * a gap of 20 from the current xid counter, so autovacuum will * not touch them. * ! * synchronous_commit=off improves object creation speed, and we only ! * modify the new cluster, so only use it there. If there is a crash, ! * the new cluster has to be recreated anyway. */ snprintf(cmd, sizeof(cmd), \%s/pg_ctl\ -w -l \%s\ -D \%s\ -o \-p %d%s%s%s%s\ start, --- 209,217 * a gap of 20 from the current xid counter, so autovacuum will * not touch them. * ! * Turn off durability requirements to improve object creation speed, and ! * we only modify the new cluster, so only use it there. If there is a ! * crash, the new cluster has to be recreated anyway. */ snprintf(cmd, sizeof(cmd), \%s/pg_ctl\ -w -l \%s\ -D \%s\ -o \-p %d%s%s%s%s\ start, *** start_postmaster(ClusterInfo *cluster) *** 219,225 (cluster-controldata.cat_ver =
Re: [HACKERS] In pg_upgrade, remove 'set -x' from test script.
Why was this change made? -- 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] initdb.c::main() too large
On Fri, Nov 30, 2012 at 06:06:39PM -0500, Andrew Dunstan wrote:
On 11/30/2012 04:45 PM, Bruce Momjian wrote:
On Thu, Nov 29, 2012 at 11:12:23PM -0500, Bruce Momjian wrote:
In looking to add an fsync-only option to initdb, I found its main()
function to be 743 lines long, and very hard to understand.
The attached patch moves much of that code into separate functions,
which will make initdb.c easier to understand, and easier to add an
fsync-only option. The original initdb.c author, Andrew Dunstan, has
accepted the restructuring, in principle.
Applied.
Sorry I didn't have time to review this before it was applied.
A few minor nitpicks:
* process() is a fairly uninformative function name, not sure what I'd
call it, but something more descriptive.
* the setup_signals_and_umask() call and possibly the final message
section of process() would be better placed back in main() IMNSHO.
* the large statements for setting up the datadir and the xlogdir
should be factored out of process() into their own functions, I
think. That would make it much more readable.
Done with the attached patch. I kept the signals in their own function,
but moved the umask() call out --- I was not happy mixing those either.
--
Bruce Momjian br...@momjian.ushttp://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
commit 26374f2a0fc02b76a91b7565e908dbae99a3b5f9
Author: Bruce Momjian br...@momjian.us
Date: Mon Dec 3 23:22:56 2012 -0500
In initdb.c, rename some newly created functions, and move the directory
creation and xlog symlink creation to separate functions.
Per suggestions from Andrew Dunstan.
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
new file mode 100644
index 8c0a9f4..d44281b
*** a/src/bin/initdb/initdb.c
--- b/src/bin/initdb/initdb.c
*** void setup_pgdata(void);
*** 255,263
void setup_bin_paths(const char *argv0);
void setup_data_file_paths(void);
void setup_locale_encoding(void);
! void setup_signals_and_umask(void);
void setup_text_search(void);
! void process(const char *argv0);
#ifdef WIN32
--- 255,265
void setup_bin_paths(const char *argv0);
void setup_data_file_paths(void);
void setup_locale_encoding(void);
! void setup_signals(void);
void setup_text_search(void);
! void create_data_directory(void);
! void create_xlog_symlink(void);
! void initialize_data_directory(void);
#ifdef WIN32
*** setup_text_search(void)
*** 3164,3170
void
! setup_signals_and_umask(void)
{
/* some of these are not valid on Windows */
#ifdef SIGHUP
--- 3166,3172
void
! setup_signals(void)
{
/* some of these are not valid on Windows */
#ifdef SIGHUP
*** setup_signals_and_umask(void)
*** 3184,3202
#ifdef SIGPIPE
pqsignal(SIGPIPE, SIG_IGN);
#endif
-
- umask(S_IRWXG | S_IRWXO);
}
void
! process(const char *argv0)
{
- int i;
- char bin_dir[MAXPGPATH];
-
- setup_signals_and_umask();
-
switch (pg_check_dir(pg_data))
{
case 0:
--- 3186,3197
#ifdef SIGPIPE
pqsignal(SIGPIPE, SIG_IGN);
#endif
}
void
! create_data_directory(void)
{
switch (pg_check_dir(pg_data))
{
case 0:
*** process(const char *argv0)
*** 3249,3255
--- 3244,3255
progname, pg_data, strerror(errno));
exit_nicely();
}
+ }
+
+ void
+ create_xlog_symlink(void)
+ {
/* Create transaction log symlink, if required */
if (strcmp(xlog_dir, ) != 0)
{
*** process(const char *argv0)
*** 3336,3341
--- 3336,3356
exit_nicely();
#endif
}
+ }
+
+
+ void
+ initialize_data_directory(void)
+ {
+ int i;
+
+ setup_signals();
+
+ umask(S_IRWXG | S_IRWXO);
+
+ create_data_directory();
+
+ create_xlog_symlink();
/* Create required subdirectories */
printf(_(creating subdirectories ... ));
*** process(const char *argv0)
*** 3404,3422
if (authwarning != NULL)
fprintf(stderr, %s, authwarning);
-
- /* Get directory specification used to start this executable */
- strlcpy(bin_dir, argv0, sizeof(bin_dir));
- get_parent_directory(bin_dir);
-
- printf(_(\nSuccess. You can now start the database server using:\n\n
- %s%s%spostgres%s -D %s%s%s\n
- or\n
- %s%s%spg_ctl%s -D %s%s%s -l logfile start\n\n),
- QUOTE_PATH, bin_dir, (strlen(bin_dir) 0) ? DIR_SEP : , QUOTE_PATH,
- QUOTE_PATH, pgdata_native, QUOTE_PATH,
- QUOTE_PATH, bin_dir, (strlen(bin_dir) 0) ? DIR_SEP : , QUOTE_PATH,
- QUOTE_PATH, pgdata_native, QUOTE_PATH);
}
--- 3419,3424
*** main(int argc, char *argv[])
*** 3459,3464
--- 3461,3467
int c;
int option_index;
char *effective_user;
+ char bin_dir[MAXPGPATH];
progname = get_progname(argv[0]);
set_pglocale_pgservice(argv[0],
Re: [HACKERS] In pg_upgrade, remove 'set -x' from test script.
On Mon, Dec 3, 2012 at 11:24:08PM -0500, Peter Eisentraut wrote: Why was this change made? I asked Andrew and he had no idea why a 'set -x' would be in the script. I ran the script and saw the commands being echoed. Was that intentional? If so, I can add it back, but it would be good to add a comment as to why it was being used, because Andrew and I had no idea, and thought it was a mistake. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] support for LDAP URLs
On Mon, 2012-11-26 at 18:15 -0300, Alvaro Herrera wrote: Should we be referencing RFC 4516 instead? True. I'm not very fond of the way this entry is worded: Good point. I've rewritten it a little bit. -- 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] In pg_upgrade, remove 'set -x' from test script.
On Mon, 2012-12-03 at 23:26 -0500, Bruce Momjian wrote: On Mon, Dec 3, 2012 at 11:24:08PM -0500, Peter Eisentraut wrote: Why was this change made? I asked Andrew and he had no idea why a 'set -x' would be in the script. I ran the script and saw the commands being echoed. Was that intentional? yes If so, I can add it back, please but it would be good to add a comment as to why it was being used, because Andrew and I had no idea, and thought it was a mistake. Well, it is so you can see what's being run in case you see failures or odd output. It's usually fairly self-evident what set -x does, but feel free to comment it. -- 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] In pg_upgrade, remove 'set -x' from test script.
On Mon, Dec 3, 2012 at 11:38:03PM -0500, Peter Eisentraut wrote: On Mon, 2012-12-03 at 23:26 -0500, Bruce Momjian wrote: On Mon, Dec 3, 2012 at 11:24:08PM -0500, Peter Eisentraut wrote: Why was this change made? I asked Andrew and he had no idea why a 'set -x' would be in the script. I ran the script and saw the commands being echoed. Was that intentional? yes If so, I can add it back, please but it would be good to add a comment as to why it was being used, because Andrew and I had no idea, and thought it was a mistake. Well, it is so you can see what's being run in case you see failures or odd output. It's usually fairly self-evident what set -x does, but feel free to comment it. OK, added, with a comment. When the script ends, the set -x is a little verbose for my liking. It makes it look like something is wrong --- perhaps we should set +x there. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] pg_ping utility
On Sun, Dec 2, 2012 at 5:56 AM, Phil Sorber p...@omniti.com wrote: Here is the updated patch. I renamed it, but using v5 to stay consistent. After looking at this patch, I found the following problems: - There are a couple of whitespaces still in the code, particularly at the end of those lines + const char *pguser = NULL; + const char *pgdbname = NULL; - When describing the values that are returned by pg_isready, it is awkward to refer to the returning values as plain integers as those values are part of an enum. You should add references to PQPING_OK, PQPING_REJECT, PQPING_NO_RESPONSE and PQPING_NO_ATTEMPT instead. Also add to reference links in the docs redirecting to them, with things of the type xref linkend=libpq-pqpingparams-pqping-ok or related. - Same thing with this example: + para +Standard Usage: +screen + prompt$/prompt userinputpg_isready/userinput + prompt$/prompt userinputecho $?/userinput + computeroutput0/computeroutput +/screen + /para For the time being PQPING_OK returns 0 because it is on top of the enum PGPing, but this might change if for a reason or another the order of outputs is changed. Once those things are fixed, I think this will be ready for committer review as everybody here seem to agree with your approach. -- Michael Paquier http://michael.otacoo.com
Re: [HACKERS] Patch for removng unused targets
From: Tom Lane [mailto:t...@sss.pgh.pa.us] Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes: Sorry for the delay. I've reviewed the patch. It was applied successfully, and it worked well for tests I did including the example you showed. I think it's worth the work, but I'm not sure you go about it in the right way. (I feel the patch decreases code readability more than it gives an advantage.) One thought here is that I don't particularly like adding a field like resorderbyonly to TargetEntry in the first place. That makes this optimization the business of the parser, which it should not be; and furthermore makes it incumbent on the rewriter, as well as anything else that manipulates parsetrees, to maintain the flag correctly while rearranging queries. It would be better if this were strictly the business of the planner. Okay. I would like to investigate a planner-based approach that would not require the resorderbyonly field. Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Master fails to build without ldap headers
Hi all While doing some setup I've noticed that master (at least) appears to fail to build if LDAP headers aren't installed. gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -I../../../src/include -D_GNU_SOURCE -c -o hba.o hba.c hba.c: In function 'parse_hba_auth_opt': hba.c:1388: error: 'LDAP_SCOPE_SUBTREE' undeclared (first use in this function) hba.c:1388: error: (Each undeclared identifier is reported only once hba.c:1388: error: for each function it appears in.) hba.c:1451: error: 'LDAPURLDesc' undeclared (first use in this function) hba.c:1451: error: 'urldata' undeclared (first use in this function) hba.c:1452: warning: ISO C90 forbids mixed declarations and code hba.c:1452: warning: unused variable 'rc' It looks like this was broken by: commit aa2fec0a18e4d23272c78916ef318078c920611a Author: Peter Eisentraut pete...@gmx.net Date: Mon Dec 3 23:29:56 2012 -0500 Add support for LDAP URLs Allow specifying LDAP authentication parameters as RFC 4516 LDAP URLs. -- Craig Ringer 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] ALTER TABLE ... NOREWRITE option
On 12/02/2012 03:07 AM, Noah Misch wrote: On Sat, Dec 01, 2012 at 07:34:51PM +0100, Andres Freund wrote: On 2012-12-01 18:27:08 +, Simon Riggs wrote: On 1 December 2012 16:38, Tom Lane t...@sss.pgh.pa.us wrote: Simon Riggs si...@2ndquadrant.com writes: It's hard to know whether your tables will be locked for long periods when implementing DDL changes. The NOREWRITE option would cause an ERROR if the table would be rewritten by the command. This would allow testing to highlight long running statements before code hits production. I'm not thrilled about inventing YA keyword for this. If you have a problem with that sort of scenario, why aren't you testing your DDL on a test server before you do it on production? That's the point. You run it on a test server first, and you can conclusively see that it will/will not run for a long time on production server. Acquiring the lock could still take an unpredictable amount of time. Greg Sabine Mullane wrote an interesting blog about a way of solving the problem in userspace. I currently recommend using the DEBUG1 messages for this purpose: [local] test=# set client_min_messages = debug1; SET [local] test=# create table t (c int8 primary key, c1 text); DEBUG: building index pg_toast_109381_index on table pg_toast_109381 DEBUG: CREATE TABLE / PRIMARY KEY will create implicit index t_pkey for table t DEBUG: building index t_pkey on table t CREATE TABLE [local] test=# alter table t alter c type int4; DEBUG: building index pg_toast_109391_index on table pg_toast_109391 DEBUG: rewriting table t DEBUG: building index t_pkey on table t ALTER TABLE [local] test=# alter table t alter c type oid; DEBUG: building index t_pkey on table t ALTER TABLE Observe that some changes rewrite the table and all indexes, while others skip rewriting the table but rebuild one or more indexes. I've threatened to optimize type changes like (base type) - (domain with CHECK constraint) by merely scanning the table for violations. If we do add syntax such as you have proposed, I recommend using a different name and defining it to reject any operation with complexity O(n) or worse relative to table size. That being said, I share Tom's doubts. The DEBUG1 messages are a sorry excuse for a UI, but I'm not seeing a clear improvement in NOREWRITE. Or even more to the point, you can always cancel the statement once you realize it's taking too long. Which means you have to watch it, which is not always possible. There's statement_timeout. I think the point was in catching the rewrite behaviour in testing. for statement_timeout to kick in you may need to have both production size and production load which is not always easy to achieve in testing. My first thought is to add more detailed EXPLAIN support for DDL... Although that unfortunately broadens the scope of this a tiny bit. That would be ideal. It would also be nice to add a dry run support, which switches to EXPLAIN for all SQL instead of executing. SET explain_only TO TRUE; Hannu -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Bug in buildfarm client
Hello all,
the extension modules (TestUpgrade etc.) in the buildfarm client do not
use the make command override defined in the config file, instead
hardcoding command lines using make. This fails where make is not GNU
make.
Patch attached, which fixes the problem on jaguarundi.
--
Christian
*** /tmp/qvZogO_FileTextArrayFDW.pm 2012-12-04 08:31:46.878698514 +0100
--- PGBuild/Modules/FileTextArrayFDW.pm 2012-12-04 08:29:35.653265999 +0100
***
*** 119,125
print main::time_str(), building $MODULE\n if $verbose;
! my $cmd = PATH=../inst:$ENV{PATH} make USE_PGXS=1;
my @makeout = `cd $self-{where} $cmd 21`;
--- 119,125
print main::time_str(), building $MODULE\n if $verbose;
! my $cmd = PATH=../inst:$ENV{PATH} $self-{bfconf}-{make} USE_PGXS=1;
my @makeout = `cd $self-{where} $cmd 21`;
***
*** 136,142
print main::time_str(), installing $MODULE\n if $verbose;
! my $cmd = PATH=../inst:$ENV{PATH} make USE_PGXS=1 install;
my @log = `cd $self-{where} $cmd 21`;
--- 136,142
print main::time_str(), installing $MODULE\n if $verbose;
! my $cmd = PATH=../inst:$ENV{PATH} $self-{bfconf}-{make} USE_PGXS=1
install;
my @log = `cd $self-{where} $cmd 21`;
***
*** 161,167
print main::time_str(), install-checking $MODULE\n if $verbose;
! my $cmd = PATH=../inst:$ENV{PATH} make USE_PGXS=1 installcheck;
my @log = `cd $self-{where} $cmd 21`;
--- 161,167
print main::time_str(), install-checking $MODULE\n if $verbose;
! my $cmd = PATH=../inst:$ENV{PATH} $self-{bfconf}-{make} USE_PGXS=1
installcheck;
my @log = `cd $self-{where} $cmd 21`;
*** /tmp/3XAamk_TestUpgrade.pm 2012-12-04 08:31:46.886698485 +0100
--- PGBuild/Modules/TestUpgrade.pm 2012-12-04 08:28:38.007735375 +0100
***
*** 67,73
}
else
{
! my $cmd = cd $self-{pgsql}/contrib/pg_upgrade make check;
@checklog = `$cmd 21`;
}
--- 67,73
}
else
{
! my $cmd = cd $self-{pgsql}/contrib/pg_upgrade
$self-{bfconf}-{make} check;
@checklog = `$cmd 21`;
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
